Casper da Costa-Luis

unread,

Jan 19, 2018, 12:44:38 PM1/19/18

to CCPPETMR/SIRF, Subscribed

fix travis for osx builds

You can view, comment on, or merge this pull request online at:

https://github.com/CCPPETMR/SIRF/pull/79

Commit Summary

osx wip

File Changes

M .travis.yml (12)

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Richard Brown

unread,

Jan 29, 2018, 5:28:55 AM1/29/18

to CCPPETMR/SIRF, Subscribed

@casperdcl:
The original problem for OSX was that it couldn't open rpaths to dylibs, so compilation would complete, but tests would fail. I managed to replicate this problem on my own OSX.

Using the Cmake argument, CMAKE_INSTALL_NAME_DIR=${CMAKE_INSTALL_PREFIX}/lib in SuperBuild.cmake and then passing this to all subsequent CMAKE_ARGS in the necessary External_*.cmake files, I managed to install all shared libraries with absolute paths on my own machine.

However, Travis is still failing. The error is Fatal Python error: PyThreadState_Get: no current thread, and a quick google tells me that this happens when libraries are compiled against one version of python, but run with another. I know you added the python$PYMVER command, so I'm at a loss of what to do. Thoughts?

As far as I can tell from the Travis log, I have no way of telling which library was compiled against the different version of python (if that is the issue...). The branch is here: OSX_abs_paths.

Casper da Costa-Luis

unread,

Jan 29, 2018, 6:04:16 AM1/29/18

to CCPPETMR/SIRF, Subscribed

Thanks for looking into this. It confirms what I had said during the earlier tcons. You can see the mismatched bin amd lib versions (2.7.10 and 2.7.14 from what I remember) in the cmake output on travis. What I had requested is of anyone knows of a way of installing matching bin & lib versions on osx. AFAIK travis doesn't have matched versions. I'm tempted to just go with anaconda...

Casper da Costa-Luis

unread,

Jan 29, 2018, 6:09:37 AM1/29/18

to CCPPETMR/SIRF, Subscribed

Edit: yes just looked at your log, it shows 2.7.14 (bin) and 2.7.10 (lib). Don't recall ever having issues with rpaths/dylibs.

Kris Thielemans

unread,

Jan 29, 2018, 6:20:53 AM1/29/18

to CCPPETMR/SIRF, Subscribed

we've briefly looked at this. we're still using python-config to fix PYTHON_LIBRARY presumably. That'll create conflicts.

Richard Brown

unread,

Jan 30, 2018, 9:09:05 AM1/30/18

to CCPPETMR/SIRF, Subscribed

Hi all,

I've been trying to set one specific version of python. I want to get that to work before going to the general case of letting the user select their python version.

I set EXTRA_BUILD_FLAGS to include PTHON_EXECUTABLE, PYTHON_LIBRARY and PYTHON_INCLUDE_DIR which point towards bin/python2.7, lib/python2.7.dylib and include/python2.7, respectively, in /System/Library/Frameworks/Python.framework/Versions/2.7/.

I also created a variable, PYTHON_EXECUTABLE, so that instead of calling python$MPYVER -m pip, PYTHON_EXECUTABLE -m pip can be used instead.

However, I'm now getting a strange error during the ctests saying: ImportError: No module named matplotlib. This is odd because earlier in the same log, during the installation of matplotlib, there is the following message: Requirement already satisfied: matplotlib in /System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python

I've been trying to remedy this by messing around with pip install to no avail. Any ideas?

The branch I'm working on is here.

Kris Thielemans

unread,

Jan 30, 2018, 4:00:58 PM1/30/18

to CCPPETMR/SIRF, Subscribed

Checked the latest travis log. It seems that @rijobro got passed the python/matplotlib problems with the hard-wired path (great, good starting point!). However, now we have

ImportError: dlopen(/Users/travis/build/CCPPETMR/SIRF-SuperBuild/INSTALL/python/_pygadgetron.so, 2): Library not loaded: libismrmrd.1.3.dylib

  Referenced from: /Users/travis/build/CCPPETMR/SIRF-SuperBuild/INSTALL/python/_pygadgetron.so

  Reason: unsafe use of relative rpath libismrmrd.1.3.dylib in /Users/travis/build/CCPPETMR/SIRF-SuperBuild/INSTALL/python/_pygadgetron.so with restricted binary

This is essentially the same problem as CCPPETMR/SIRF-SuperBuild#63. I believe this needs a SIRF modification to the CMakeLists.txt as in https://stackoverflow.com/questions/35377704/dlopen-error-unsafe-use-of-relative-rpath-on-os-x-el-capitan (although it seems to me that CMAKE_BUILD_WITH_INSTALL_RPATH would normally be kept to FALSE). Note also that that post and the CMake wiki set CMAKE_INSTALL_RPATH twice (I would keep only the 2nd one).

Presumably we would then need a similar modification in ISMRMRD and Gadgetron and STIR... One way to achieve this would be to set these in the SuperBuild.cmake and use mark_as_superbuild for all the relevant variables.

(Didn't we have this conversation already?)

Richard Brown

unread,

Jan 31, 2018, 5:16:03 AM1/31/18

to CCPPETMR/SIRF, Subscribed

Unfortunately, options such as CMAKE_BUILD_WITH_INSTALL_RPATH and CMAKE_INSTALL_RPATH are depreciated on OSX and no longer have an effect.

I managed to avoid the unsafe use of relative rpath error with CMAKE_INSTALL_NAME_DIR=${CMAKE_INSTALL_PREFIX}/lib, which installs the shared libraries with absolute paths. I put this as an advanced option called SHARED_LIBS_ABS_PATH in SuperBuild.cmake for OSX users (default is OFF). I then passed CMAKE_INSTALL_NAME_DIR to all the CMAKE_ARGS of the necessary External_*.cmake files.

Brew's installation of Boost 1.65 uses rpaths, so caused errors. This can be avoided either by compiling our own version (with SHARED_LIBS_ABS_PATH=ON) or using brew upgrade boost (this gets version 1.66, which uses absolute paths).

Lastly, we had problems of mismatching python versions. I got around this by hard-wiring paths to PYTHON_EXECUTABLE, PYTHON_LIBRARY and PYTHON_INCLUDE_DIR. I then replaced python$MPYVER with PYTHON_EXECUTABLE to ensure everything was installed with the same version of python. This needs some work to make more flexible (or we can just leave it for Travis as it seems to work on real Macs).

Kris Thielemans

unread,

Feb 2, 2018, 2:07:05 AM2/2/18

to CCPPETMR/SIRF, Subscribed

@rijobro, can you merge your SuperBuild changes here to SIRF as well?

Richard Brown

unread,

Feb 2, 2018, 7:01:57 AM2/2/18

to CCPPETMR/SIRF, Subscribed

Done, and have pull requested. Travis currently fails because SIRF clones the master SuperBuild which does not have this fix yet.

codecov[bot]

unread,

Feb 2, 2018, 1:26:19 PM2/2/18

to CCPPETMR/SIRF, Subscribed

Codecov Report

Merging #79 into master will increase coverage by 0.97%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   54.54%   55.51%   +0.97%     
==========================================
  Files           2        2              
  Lines        1674     1632      -42     
==========================================
- Hits          913      906       -7     
+ Misses        761      726      -35

Impacted Files	Coverage Δ
src/xSTIR/pSTIR/pSTIR.py	`61.63% <0%> (+2.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54afc3e...66fbf45. Read the comment docs.

codecov[bot]

unread,

Feb 2, 2018, 1:42:36 PM2/2/18

to CCPPETMR/SIRF, Subscribed

Codecov Report

Merging #79 into master will increase coverage by 0.97%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   54.54%   55.51%   +0.97%     
==========================================
  Files           2        2              
  Lines        1674     1632      -42     
==========================================
- Hits          913      906       -7     
+ Misses        761      726      -35

Impacted Files	Coverage Δ
src/xSTIR/pSTIR/pSTIR.py	`61.63% <0%> (+2.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54afc3e...66fbf45. Read the comment docs.

—

Casper da Costa-Luis

unread,

Feb 2, 2018, 1:43:54 PM2/2/18

to CCPPETMR/SIRF, Push

@casperdcl pushed 1 commit.

2419547 more tests

—
You are receiving this because you are subscribed to this thread.

View it on GitHub or mute the thread.

Kris Thielemans

unread,

Feb 4, 2018, 3:37:05 PM2/4/18

to CCPPETMR/SIRF, Subscribed

@KrisThielemans commented on this pull request.

In .travis.yml:

> @@ -143,12 +181,12 @@ before_install:
  - export BUILD_FLAGS="$BUILD_FLAGS -DPYVER=$PYMVER -DSIRF_URL=$PWD -DSIRF_TAG=$TRAVIS_COMMIT"
  # get SuperBuild
  - cd ..
- - git clone https://github.com/CCPPETMR/SIRF-SuperBuild --recursive -b master
+ - git clone https://github.com/CCPPETMR/SIRF-SuperBuild --recursive -b OSX_abs_paths

do we want this? we then will have to remember to remove it later.

Casper da Costa-Luis

unread,

Feb 4, 2018, 6:30:32 PM2/4/18

to CCPPETMR/SIRF, Subscribed

@casperdcl commented on this pull request.

In .travis.yml:

> @@ -143,12 +181,12 @@ before_install:
  - export BUILD_FLAGS="$BUILD_FLAGS -DPYVER=$PYMVER -DSIRF_URL=$PWD -DSIRF_TAG=$TRAVIS_COMMIT"
  # get SuperBuild
  - cd ..
- - git clone https://github.com/CCPPETMR/SIRF-SuperBuild --recursive -b master
+ - git clone https://github.com/CCPPETMR/SIRF-SuperBuild --recursive -b OSX_abs_paths

yes, have it in my local merge, will push when approved

Kris Thielemans

unread,

Feb 5, 2018, 1:48:33 AM2/5/18

to CCPPETMR/SIRF, Subscribed

@KrisThielemans approved this pull request.

Casper da Costa-Luis

unread,

Feb 5, 2018, 1:58:33 PM2/5/18

to CCPPETMR/SIRF, Subscribed

Closed #79 via 6579f3c.

Reply all

Reply to author

Forward

[CCPPETMR/SIRF] CI: OSX (#79)

Casper da Costa-Luis

You can view, comment on, or merge this pull request online at:

Commit Summary

File Changes

Patch Links:

Richard Brown

Casper da Costa-Luis

Casper da Costa-Luis

Kris Thielemans

Richard Brown

Kris Thielemans

Richard Brown

Kris Thielemans

Richard Brown

codecov[bot]

Codecov Report

codecov[bot]

Codecov Report

Casper da Costa-Luis

Kris Thielemans

Casper da Costa-Luis

Kris Thielemans

Casper da Costa-Luis