Skip to content

added feedback after failing command with trap and added info about l…#131

Merged
aothms merged 1 commit into
IfcOpenShell:masterfrom
krichter722:build-all_feedback
Nov 13, 2016
Merged

added feedback after failing command with trap and added info about l…#131
aothms merged 1 commit into
IfcOpenShell:masterfrom
krichter722:build-all_feedback

Conversation

@krichter722

Copy link
Copy Markdown

…ogging file printed to stdout

@aothms

aothms commented Sep 10, 2016

Copy link
Copy Markdown
Member

Thanks, this would be a welcome addition. What OS, shell did you test this on? What kind of errors are supposed to trigger the trap? For me the ERR trap does not work. I introduced some errors manually but the message does not get printed. I read about some differences between set -e vs set -E here [1] but also that didnt make the trap work as expected to me.

[1] http://stackoverflow.com/questions/35800082/bash-how-to-trap-err-when-using-set-e

@krichter722

Copy link
Copy Markdown
Author

I tested on Ubuntu 16.04, but one has to use set -E instead of set -e - I'm sure that the bash anti-quality insurance vetoed a simpler system as not complicated enough.

With set -E a failure causes the function to fail, but not the script which renders set -E quite useless imo. Since there's also no way to handle information transparently for the user in the trap (only the line number is available in LINENO which is strictly speaking developer and not user information). Can we get rid of set -whatever and handle failures properly? In bash this is hell this is overkill because commands aren't handled well, so do you mind if I port this into python?

@krichter722

Copy link
Copy Markdown
Author

I started with the python port (see push in this PR), but it's a lot of work to port - a pity that such a deprecated technology as a shell script has been used in the first place.

@aothms

aothms commented Sep 11, 2016

Copy link
Copy Markdown
Member

Ok, thanks for spearheading this. I must say I am quite in favour of this. The script organically grew a bit out of hand.

I still plan to add some small things, e.g different CMake generators to setup a user with a working development environment. Now its mostly oriented towards building release builds.

Surely this will be more maintainable in a proper scripting language. Let me know if there is something where I can pitch in.

@krichter722

Copy link
Copy Markdown
Author

I wrote a long comment yesterday explaining in detail everything that's wrong - no idea how anyone can produce a software development platform that makes it possible to loose a comment, no matter whether it's posted or not. So you only get the short version:

  • line xy: unclear why declare is used and how to transform it into python
  • building of swig fails for unknown reasons (see https://travis-ci.org/krichter722/IfcOpenShell/builds/159311269 for details)
  • download of cmake fails randomly with both curl and wget - wget seems more reliable
  • If I build locally the build of swig doesn't fail, but the python wrappers do - no idea why from staring at the screen. Although it's a bit more work to manage the build directory in variables and pass the cwd argument to subprocess.x it's much more resistant to future trouble than actually changing the current working directory.

…feedback

for the user at all (shell scripts systematically are working against providing
any useful user feedback, so the port is the only option)
@aothms

aothms commented Sep 13, 2016

Copy link
Copy Markdown
Member

Thanks. I will have a more detailed look soon.

unclear why declare is used

Well, unfortunately I cannot precisely recall the reason either. It was some work-around for something trivial. The idea is that there is some trivial CMake file to concatenate certain flags into an env var (e.g. CXX_FLAGS from the CMake variable CMAKE_CXX_FLAGS_RELEASE if the user specified Release for its BUILD_CFG). I will have a look to reproduce it in the python script.

building of swig fails for unknown reasons

I will have a look

download of cmake fails randomly

Never experienced this, maybe a temporary issue somewhere?

If I build locally the build of swig doesn't fail, but the python wrappers do

I will have a look. Using the cwd kwarg of subprocess.Popen sounds like a good idea.

@aothms

aothms commented Sep 13, 2016

Copy link
Copy Markdown
Member

With the following changes I was able to build everything successfully. There were a couple of wrong cwds being set. The issue with swig was that apparently bash should not be used to run ../configure. No idea why that would fail, took me some time to get to that, as I ruled it out as an unlikely cause initially. Other than that I think it worked really well. Benefit of Python is immediately clear in tracking down the errors, so thanks for that. Have a look at the changes below, probably easiest if you add them to the PR.

diff --git a/nix/build-all.py b/nix/build-all.py
old mode 100644
new mode 100755
index afb4252..e34499c
--- a/nix/build-all.py
+++ b/nix/build-all.py
@@ -40,6 +40,7 @@ import subprocess as sp
 import shutil
 import time
 import tarfile
+import multiprocessing

 logger = logging.getLogger(__name__)
 logger.setLevel(logging.INFO)
@@ -104,7 +105,7 @@ def get_os():
 try:
     IFCOS_NUM_BUILD_PROCS = os.environ["IFCOS_NUM_BUILD_PROCS"]
 except KeyError:
-    IFCOS_NUM_BUILD_PROCS=int(sp.check_output([bash, "-c", "sysctl -n hw.ncpu 2> /dev/null || cat /proc/cpuinfo | grep processor | wc -l"]))+1
+    IFCOS_NUM_BUILD_PROCS=multiprocessing.cpu_count() + 1
     os.environ["IFCOS_NUM_BUILD_PROCS"]=str(IFCOS_NUM_BUILD_PROCS)

 try:
@@ -188,11 +189,19 @@ if not os.path.exists(LOG_FILE):
 logger.info("using command log file '%s'" % (LOG_FILE,))

 def __check_call__(cmds, cwd=None):
-    logger.info("running command '%s' in directory '%s'" % (cmds, cwd))
+    logger.info("running command '%s' in directory '%s'" % (" ".join(cmds), cwd))
     log_file_handle = open(LOG_FILE, "a")
-    sp.check_call(cmds, cwd=cwd, stdout=log_file_handle, stderr=log_file_handle)
+    proc = sp.Popen(cmds, cwd=cwd, stdout=log_file_handle, stderr=sp.PIPE)
+    _, stderr = proc.communicate()
+    log_file_handle.write(stderr)
     log_file_handle.close()

+    if proc.returncode != 0:
+        print "-" * 70
+        print stderr
+        print "-" * 70
+        raise Exception("Command `%s` returned exit code %d" % (" ".join(cmds), proc.returncode))
+
 def __check_output__(cmds, cwd=None):
     """Wraps `subprocess.check_output` and logs the command being executed,
     sets up logging `stderr` to `LOG_FILE` (in append mode) and strips the
@@ -219,9 +228,8 @@ def run_autoconf(arg1, configure_args, cwd):
     configure_path = os.path.realpath(os.path.join(cwd, "..", "configure"))
     if not os.path.exists(configure_path):
         __check_call__([bash, "./autogen.sh"], cwd=os.path.realpath(os.path.join(cwd, ".."))) # only run autogen.sh in the directory it is located and use cwd to achieve that in order to not mess up things
-    __check_call__([bash, configure_path
-        #"../configure"
-        ]+configure_args+["--prefix=%s" % (os.path.realpath("%s/install/%s" % (DEPS_DIR, arg1)),)], cwd=cwd)
+    # Using `sh` over `bash` fixes issues with building swig 
+    __check_call__(["/bin/sh", "../configure"]+configure_args+["--prefix=%s" % (os.path.realpath("%s/install/%s" % (DEPS_DIR, arg1)),)], cwd=cwd)

 def run_cmake(arg1, cmake_args, cmake_dir=None, cwd=None):
     if cmake_dir is None:
@@ -454,14 +462,14 @@ run_cmake("", cmake_args=["-DBOOST_ROOT=%s/install/boost-%s" % (DEPS_DIR, BOOST_

 logger.info("\rBuilding executables...   ")

-__check_call__([make, "-j%s" % (IFCOS_NUM_BUILD_PROCS,)], cwd=CMAKE_DIR)
+__check_call__([make, "-j%s" % (IFCOS_NUM_BUILD_PROCS,)], cwd=executables_dir)

 if get_os() == "Darwin":
     STRIP_OPTION="-x"
 else:
     STRIP_OPTION="-s"

-__check_call__([strip, STRIP_OPTION, "IfcConvert", "IfcGeomServer"], cwd=CMAKE_DIR)
+__check_call__([strip, STRIP_OPTION, "IfcConvert", "IfcGeomServer"], cwd=executables_dir)

 # On OSX the actual Python library is not linked against.
 ADDITIONAL_ARGS=""
@@ -499,10 +507,10 @@ for PYTHON_VERSION in PYTHON_VERSIONS:

     logger.info("\rBuilding python %s wrapper...   " % (PYTHON_VERSION,))

-    __check_call__([make, "-j%s" % (IFCOS_NUM_BUILD_PROCS,), "_ifcopenshell_wrapper"], cwd=CMAKE_DIR)
+    __check_call__([make, "-j%s" % (IFCOS_NUM_BUILD_PROCS,), "_ifcopenshell_wrapper"], cwd=python_dir)

     if get_os() != "Darwin":
         # TODO: This symbol name depends on the Python version?
-        __check_call__([strip, "-s", "-K", "PyInit__ifcopenshell_wrapper", "ifcwrap/_ifcopenshell_wrapper.so"])
+        __check_call__([strip, "-s", "-K", "PyInit__ifcopenshell_wrapper", "ifcwrap/_ifcopenshell_wrapper.so"], cwd=python_dir)

 logger.info("\rBuilt IfcOpenShell...\n\n")

@aothms aothms mentioned this pull request Nov 6, 2016
@aothms aothms merged commit 72d8b53 into IfcOpenShell:master Nov 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants