[CCPPETMR/SIRF] Fix python dest (#160)
Edoardo Pasca
The PYTHON_DEST variable was always overwritten to `${CMAKE_INSTALL_PREFIX}/python'
You can view, comment on, or merge this pull request online at:
https://github.com/CCPPETMR/SIRF/pull/160
Commit Summary
- PYTHON_DEST was always overwritten
- debug message
- if NOT does not apply to variables
- CMake if does not want dereferencing
- removed messages
File Changes
- M CMakeLists.txt (5)
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.
Kris Thielemans
@KrisThielemans commented on this pull request.
In CMakeLists.txt:
> @@ -75,7 +75,10 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
- set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+ if (PYTHON_DEST)
clearer to say
if (NOT PYTHON_DEST) set(...) endif()
Kris Thielemans
I understand that this will do the following: if PYTHON_DEST not set by the user, it will use the current CMAKE_INSTALL_PREFIX to construct the path, else it will use the user's value. Neat.
This circumvents the original problem creating it as a cached variable which we fixed in #77. Does it also work when the user creates PYTHON_DEST only at the 2nd run of CMake? (I think so).
It is of course somewhat non-CMake-like as the unsuspecting user will never see the possibility of using this variable (as it won't appear in the GUI for instance). I guess we could/should document it. A possibly better alternative would be to do this trick with another cached variable, e.g. PYTHON_INSTALL_DIR. Something like this maybe?
if (BUILD_PYTHON) set(PYTHON_INSTALL_DIR "" CACHE ....) if (PYTHON_INSTALL_DIR) set(PYTHON_DEST "${PYTHON_INSTALL_DIR }") else()
set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
endif()This should work due to the rules for if(). It is somewhat confusing for the user though.
Opinions?
Clearly, we'd have to do the same for MATLAB_DEST
Kris Thielemans
> @@ -75,7 +75,10 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
- set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+ if (PYTHON_DEST)
+ else()
+ set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+ endif()
message(STATUS "Python libraries found")
let's add the install location to the message
Edoardo Pasca
@paskino commented on this pull request.
In CMakeLists.txt:
> @@ -75,7 +75,10 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
- set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+ if (PYTHON_DEST)
does that work? from the docs I had the impression it wouldn't with a variable
Edoardo Pasca
@paskino pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Casper da Costa-Luis
@casperdcl approved this pull request.
LGTM; no-brainer
Kris Thielemans
If we're (re)allowing setting the destination, I'd rather get this right now (especially as we'll do this for SIRF, SIRF-SuperBuild and STIR). I'm therefore leaning towards the cached variable. We could call that PYTHON_DEST of course, but it'd need more code changes and possibly PYTHON_INSTALL_DIR is more standard?
PS: I checked that Gadgetron doesn't allow to set this. They have an internal variable for it and install in ${CMAKE_INSTALL_PREFIX}/share/gadgetron/python , see here. However, this isn't a major problem for us as we don't build/use the python gadgets.
Casper da Costa-Luis
Even if we did use the gadgetron python things, we'd use their framework to install it - not ours.
Edoardo Pasca
@KrisThielemans do you intend to have a cached variable only in the SuperBuild, or also in SIRF/STIR?
To me it makes sense to have a cached variable only in the SuperBuild. Possibly in STIR, which is independent on SIRF.
Who is going to install SIRF without the SuperBuild?
Kris Thielemans
Who is going to install SIRF without the SuperBuild?
most SIRF developers I think. It's not so easy to set up the SuperBuild such that it picks up your local copy where you're editing (and not occasionally override things).
Let's therefore put it in SIRF and SIRF-SuperBuild and STIR.
Before going ahead, can you just do a check that with your current set-up, changing the CMAKE_INSTALL_PREFIX (while not having set PYTHON_DEST) does indeed do what we want?
Clearly, we'd want it for matlab as well.
Just had a conversation about naming of the cached variables with @bathomas. It's not so easy to choose a name that is immediately clear to people (Matlab_INSTALL_DIR could just as well be understood as "where is your Matlab", which is actually a variable Matlab_ROOT_DIR). Feel free to put some suggestions here and let's vote on Friday.
Casper da Costa-Luis
I think *_(BINARY|LIBRARY_INCLUDE)_DIR are all for where to put obtain already present files from findPackage() etc, while *_(INSTALL|PACKAGE|PREFIX)_DIR should be for new things to install. Personally I'd vote for prefix because that matches configure logic
Casper da Costa-Luis
Shouldn't SIRF devs be using a superbuild script cmake.sh with all the flags they need (including -DSIRF_TAG=myLocalFeatureBranch)?
Edoardo Pasca
@casperdcl I'm actually thinking that that would be the preferred way. However, the SuperBuild will always overwrite the sources directory, so you'll always loose your changes.
A good enhancement would be to select whether to download SIRF or not during a SuperBuild.
Casper da Costa-Luis
I've only ever used the SuperBuild for all my dev work and never had a problem with overwriting. Maybe I've been doing things a bit differently from everyone else? May want to add this to the discussion on Fri.
Casper da Costa-Luis
Incidentally for those of you who have VMs and don't (can't) use docker directly, you can always use docker in your VM. This is what many windows devs in industry do https://blog.docker.com/2016/04/containers-and-vms-together/
Perhaps we could consider shipping a docker image(s) and a VM with the image, and drop the current VM? related: CCPPETMR/SIRF-SuperBuild#91
Kris Thielemans
@casperdcl, the potential problem is when using the SuperBuild, it will normally extract a revision of SIRF, so if you edit, the next run of the superbuild might get the revision again, silently removing your edits (git can be crazy). So you do have to use a SIRF_TAG, but that cannot be local, unless you sit the SIRF_URL to your local copy. etc. etc. (Would be good to document this).
Regarding shipping with docker. I have no clue if you can run interactive scripts then on the VM (without a bunch of trickery). It'd be a somewhat bad user-experience if they have to write docker run python ... and can only use the scripts. I know you can seth the DISPLAY etc and presumably run spyder from docker, but it starts to give me headache. (You can see I haven't used docker yet). If it turns out to be easy, well, that'd be an option. (although I am warry of creating another layer where things can go wrong).
Casper da Costa-Luis
So you do have to use a SIRF_TAG, but that cannot be local, unless you sit the SIRF_URL to your local copy. etc. etc. (Would be good to document this).
I disagree, I've never touched the SIRF_URL and it works fine.
SIRF-SuperBuild (master)$ cmake -DDEVEL_BUILD=ON . SIRF-SuperBuild (master)$ make -j SIRF-SuperBuild (master)$ pushd sources/SIRF SIRF-SuperBuild/sources/SIRF (master)$ git checkout -b suchfun SIRF-SuperBuild/sources/SIRF (suchfun)$ touch whuut SIRF-SuperBuild/sources/SIRF (suchfun)$ git add whuut SIRF-SuperBuild/sources/SIRF (suchfun)$ git commit -m "will not die" SIRF-SuperBuild/sources/SIRF (suchfun)$ popd SIRF-SuperBuild (master)$ cmake -DSIRF_TAG=suchfun . SIRF-SuperBuild (master)$ make -j SIRF-SuperBuild (master)$ pushd sources/SIRF SIRF-SuperBuild/sources/SIRF (suchfun)$ echo again >> whuut SIRF-SuperBuild/sources/SIRF (suchfun)$ git commit -am "still will not die" SIRF-SuperBuild/sources/SIRF (suchfun)$ popd SIRF-SuperBuild (master)$ cmake . SIRF-SuperBuild (master)$ make -j SIRF-SuperBuild (master)$ cat sources/SIRF/whuut again # proves cmake SuperBuild doesn't reset SIRF
Regarding how hard it is to get docker working on a linux machine ($DISPLAY, permission etc) I did all the hard work there so it's all automated. After installing docker CE (such as https://docs.docker.com/install/linux/docker-ce/ubuntu/#install-docker-ce) and docker-compose,
~$ git clone https://github.com/CCPPETMR/SIRF-SuperBuild && cd SIRF-SuperBuild/docker SIRF-SuperBuild/docker (master)$ docker-compose \ -f docker-compose.yml -f docker-compose.nix.yml up --no-start sirf
Then it's just
~$ docker start -ai sirfevery time you want to play with SIRF.
~$ docker start -ai sirf (pyvenv) sirf:~$ python /opt/SIRF-SuperBuild/sources/SIRF/examples/Python/PET/osem_reconstruction.py
Kris Thielemans
thanks @casperdcl for the explicit instructions for developers. will want to add something like that to the doc. comments/questions:
- I think the creation of the branch together with
cmake -DSIRF_TAG=suchfun .is essential. if you forget it, boom. - I'm not 100% if you have to commit before every build.
For docker, that sounds pretty good. I suppose you could launch spyder or some other GUI from there as well (if installed on the docker image). Still wondering about the extra layer though. For discussion...
Casper da Costa-Luis
- docker: added TL;DR version on https://github.com/CCPPETMR/SIRF/wiki/SIRF-SuperBuild-on-Docker#tldr
- developer example with superbuild: where to add?
- no, commits not required to build updates. also,
cmakenot required to be re-run unless there's a file creation/deletion. onlymakealways needs to be re-run on code changes.
Casper da Costa-Luis
Incidentally, I never use spyder but the only way I could get it working on my docker image was installing via conda-forge:
curl -OL https://repo.continuum.io/miniconda/Miniconda2-latest-Linux-x86_64.sh bash Miniconda2-latest-Linux-x86_64.sh conda update conda conda update -c conda-forge --all conda install -c conda-forge spyder
Casper da Costa-Luis
can we rebase this (removing non-surviving commits) and merge?
Kris Thielemans
things to fix first:
- use the cached variable strategy shown above
- rename variable (let's discuss this Friday)
- duplicate for matlab (or can be separate PR)
Kris Thielemans
name for cached variable: PYTHON_DEST_DIR, MATLAB_DEST_DIR
Edoardo Pasca
@paskino pushed 1 commit.
- ea68ad0 added PYTHON_DEST_DIR and MATLAB_DEST_DIR cached vars
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
@casperdcl I guess so, and that might be the reason that travis fails?
Edoardo Pasca
@paskino pushed 1 commit.
- 0ed3bb6 make with one thread to see what goes wrong with travis
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
@paskino pushed 2 commits.
- ef26a0c closed if
- 94e5543 Merge branch 'fix_python_dest' of https://github.com/CCPPETMR/SIRF into fix_python_dest
Edoardo Pasca
No, the reason travis failed is that the CMakeLists.txt had syntax error. Should be fixed now.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Edoardo Pasca
@paskino pushed 1 commit.
- a0f4a5b closed if for MATLAB_DEST
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
Please also add this in CHANGES.md. I guess also in installation instructions (if they're on the wiki, we'd have to wait until the release I guess).
In .travis.yml:
> @@ -63,7 +63,7 @@ env: global: - BUILD_FLAGS="-DCMAKE_BUILD_TYPE=Release" # don't use too many threads - may crash - - MAKEFLAGS="-j 2" + - MAKEFLAGS="-j 1"
I don't mind, but is this intentional?
In CMakeLists.txt:
> @@ -75,18 +75,31 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
- set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+ set(PYTHON_INSTALL_DIR "" CACHE PATH "Directory of the SIRF Python modules")
I seem to recall we chose PYTHON_DEST_DIR to avoid confusion with "my files" vs "system files"?
In CMakeLists.txt:
> @@ -75,18 +75,31 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
add a comment saying why we do this
In CMakeLists.txt:
> message(STATUS "Python libraries found")
+ message(STATUS "Location of Python modules: " ${PYTHON_DEST})
confusing text. maybe "SIRF Python modules will be installed in..."
In CMakeLists.txt:
> if (BUILD_MATLAB)
matlab_get_mex_suffix("${Matlab_ROOT_DIR}" MATLAB_MEX_EXT)
- set(MATLAB_DEST "${CMAKE_INSTALL_PREFIX}/matlab")
- message(STATUS "MATLAB libraries found")
+ set(MATLAB_INSTALL_DIR "" CACHE PATH "Directory of the SIRF Matlab libraries")
+ if (MATLAB_INSTALL_DIR)
+ set(MATLAB_DEST "${MATLAB_INSTALL_DIR }")
+ else()
+ set(MATLAB_DEST "${CMAKE_INSTALL_PREFIX}/matlab")
+ endif()
+ message(STATUS "Matlab libraries found")
+ message(STATUS "Location of Matlab libraries: " ${MATLAB_DEST})
same here obviously
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Edoardo Pasca
@paskino commented on this pull request.
In .travis.yml:
> @@ -63,7 +63,7 @@ env: global: - BUILD_FLAGS="-DCMAKE_BUILD_TYPE=Release" # don't use too many threads - may crash - - MAKEFLAGS="-j 2" + - MAKEFLAGS="-j 1"
yes, I use 1 thread when I debug, otherwise I don't understand what's gone wrong. will put it back to 2
Edoardo Pasca
@paskino commented on this pull request.
In CMakeLists.txt:
> @@ -75,18 +75,31 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
- set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+ set(PYTHON_INSTALL_DIR "" CACHE PATH "Directory of the SIRF Python modules")
damn you're right! #160 (comment)
Edoardo Pasca
@paskino pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In CMakeLists.txt:
> @@ -75,18 +75,31 @@ find_package(PythonInterp)
find_package(PythonLibs)
set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
if (BUILD_PYTHON)
thanks. I'd add something like "If PYTHON_DEST_DIR is not set, we will install in ${CMAKE_INSTALL_PREFIX}/python". I know, obvious maybe...
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 0261609 fix typo
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Casper da Costa-Luis
Merged #160.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.