Fix python dest #160

paskino · 2018-04-11T10:53:58Z

The PYTHON_DEST variable was always overwritten to `${CMAKE_INSTALL_PREFIX}/python'

KrisThielemans · 2018-04-17T07:21:17Z

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()

does that work? from the docs I understood it wouldn't with a variable

KrisThielemans · 2018-04-17T08:16:49Z

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

KrisThielemans · 2018-04-17T08:17:28Z

CMakeLists.txt

+  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

- depends on SyneRBI/SIRF#160 - fixes #107

casperdcl

LGTM; no-brainer

KrisThielemans · 2018-04-17T22:11:21Z

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.

casperdcl · 2018-04-18T02:05:42Z

Even if we did use the gadgetron python things, we'd use their framework to install it - not ours.

paskino · 2018-04-18T08:22:05Z

@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?

KrisThielemans · 2018-04-18T09:59:08Z

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.

casperdcl · 2018-04-18T10:34:52Z

I think *_(BINARY|LIBRARY_INCLUDE)_DIR are all for where to put obtain already present files from findPackage() etc, while *_(INSTALL|DEST|SITE|PACKAGE|PREFIX)_DIR should be for new things to install. Personally I'd vote for prefix because that matches configure logic

casperdcl · 2018-04-18T10:37:07Z

Shouldn't SIRF devs be using a superbuild script cmake.sh with all the flags they need (including -DSIRF_TAG=myLocalFeatureBranch)?

paskino · 2018-04-18T11:29:33Z

@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.

casperdcl · 2018-04-18T12:14:31Z

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.

casperdcl · 2018-04-18T13:43:04Z

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: SyneRBI/SIRF-SuperBuild#91

KrisThielemans · 2018-04-19T14:15:50Z

@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).

- depends on SyneRBI/SIRF#160 - fixes #107

casperdcl · 2018-04-26T08:32:56Z

can we rebase this (removing non-surviving commits) and merge?

KrisThielemans · 2018-04-26T08:50:20Z

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)

KrisThielemans · 2018-04-27T10:12:28Z

name for cached variable: PYTHON_DEST_DIR, MATLAB_DEST_DIR

- depends on #160 - fixes #107

#160 (comment) #160 (comment)

- depends on SyneRBI/SIRF#160 - fixes #107

paskino · 2018-04-30T17:32:25Z

@casperdcl I guess so, and that might be the reason that travis fails?

…to fix_python_dest

paskino · 2018-05-01T05:30:26Z

No, the reason travis failed is that the CMakeLists.txt had syntax error. Should be fixed now.

- depends on #160 - fixes #107

KrisThielemans

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).

KrisThielemans · 2018-05-02T15:07:04Z

.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?

yes, I use 1 thread when I debug, otherwise I don't understand what's gone wrong. will put it back to 2

KrisThielemans · 2018-05-02T15:07:39Z

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"?

damn you're right! #160 (comment)

KrisThielemans · 2018-05-02T15:08:10Z

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

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...

KrisThielemans · 2018-05-02T15:08:55Z

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..."

KrisThielemans · 2018-05-02T15:09:34Z

CMakeLists.txt

+    set(MATLAB_DEST "${CMAKE_INSTALL_PREFIX}/matlab")
+  endif()
+  message(STATUS "Matlab libraries found")
+  message(STATUS "Location of Matlab libraries: " ${MATLAB_DEST})


same here obviously

#160 (comment) some clearer message to user build with 2 threads.

- depends on #160 - fixes #107

- related to SyneRBI/SIRF#160 - fixes (partially) #107

paskino added 5 commits April 10, 2018 13:22

PYTHON_DEST was always overwritten

17676c6

debug message

1ec22f6

if NOT does not apply to variables

ed63cb9

CMake if does not want dereferencing

a4f7bd8

removed messages

403c7a8

paskino requested review from KrisThielemans, bathomas, casperdcl and rijobro April 11, 2018 10:53

paskino mentioned this pull request Apr 11, 2018

Fix python dest SyneRBI/SIRF-SuperBuild#106

Closed

KrisThielemans mentioned this pull request Apr 17, 2018

don't override external PYTHON_DEST #161

Closed

KrisThielemans reviewed Apr 17, 2018

View reviewed changes

paskino added 2 commits April 17, 2018 13:45

added message

e463b9f

use if(NOT PYTHON_DEST)

396bba7

casperdcl added a commit to SyneRBI/SIRF-SuperBuild that referenced this pull request Apr 17, 2018

backward-compatible setup.py

1e42fb6

- depends on SyneRBI/SIRF#160 - fixes #107

casperdcl mentioned this pull request Apr 17, 2018

expose python strategy SyneRBI/SIRF-SuperBuild#109

Merged

12 tasks

casperdcl approved these changes Apr 17, 2018

View reviewed changes

casperdcl added a commit to SyneRBI/SIRF-SuperBuild that referenced this pull request Apr 19, 2018

backward-compatible setup.py

011a83a

- depends on SyneRBI/SIRF#160 - fixes #107

casperdcl mentioned this pull request Apr 26, 2018

Conda SyneRBI/SIRF-SuperBuild#112

Draft

paskino self-assigned this Apr 27, 2018

casperdcl added a commit that referenced this pull request Apr 27, 2018

backward-compatible setup.py

5bd39b2

- depends on #160 - fixes #107

casperdcl mentioned this pull request Apr 27, 2018

sirf python packaging #163

Merged

12 tasks

added PYTHON_DEST_DIR and MATLAB_DEST_DIR cached vars

ea68ad0

#160 (comment) #160 (comment)

casperdcl added a commit to SyneRBI/SIRF-SuperBuild that referenced this pull request Apr 27, 2018

backward-compatible setup.py

d7e9058

- depends on SyneRBI/SIRF#160 - fixes #107

paskino added 3 commits April 30, 2018 18:46

make with one thread to see what goes wrong with travis

0ed3bb6

closed if

ef26a0c

Merge branch 'fix_python_dest' of https://github.com/CCPPETMR/SIRF in…

94e5543

…to fix_python_dest

paskino added 2 commits May 1, 2018 05:50

closed if for MATLAB_DEST

a0f4a5b

fix cache variable set

77b01af

casperdcl added a commit that referenced this pull request May 1, 2018

backward-compatible setup.py

e9654d0

- depends on #160 - fixes #107

KrisThielemans requested changes May 2, 2018

View reviewed changes

paskino added 5 commits May 3, 2018 09:01

change PYTHON_INSTALL_DIR to PYTHON_DEST_DIR

3061ddc

#160 (comment) some clearer message to user build with 2 threads.

add explanation on PYTHON_DEST_DIR

a917055

fix typo

0261609

fix another typo

cc357d5

added info on PYTHON_DEST_DIR and MATLAB_DEST_DIR

688adfa

casperdcl merged commit 241b4e2 into master May 3, 2018

casperdcl deleted the fix_python_dest branch May 3, 2018 13:36

casperdcl added a commit that referenced this pull request May 3, 2018

backward-compatible setup.py

01da234

- depends on #160 - fixes #107

casperdcl restored the fix_python_dest branch May 3, 2018 13:44

casperdcl added a commit to SyneRBI/SIRF-SuperBuild that referenced this pull request May 3, 2018

use SIRF's setup.py logic, including PYTHON_DEST_DIR

bc64088

- related to SyneRBI/SIRF#160 - fixes (partially) #107

casperdcl deleted the fix_python_dest branch May 3, 2018 15:45

KrisThielemans mentioned this pull request Jan 7, 2022

cmake caches PYTHON_DEST after first run of cmake-gui UCL/STIR#980

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix python dest #160

Fix python dest #160

paskino commented Apr 11, 2018

KrisThielemans Apr 17, 2018

paskino Apr 17, 2018 •

edited

Loading

KrisThielemans commented Apr 17, 2018

KrisThielemans Apr 17, 2018

casperdcl left a comment

KrisThielemans commented Apr 17, 2018

casperdcl commented Apr 18, 2018

paskino commented Apr 18, 2018

KrisThielemans commented Apr 18, 2018

casperdcl commented Apr 18, 2018 •

edited

Loading

casperdcl commented Apr 18, 2018

paskino commented Apr 18, 2018

casperdcl commented Apr 18, 2018

casperdcl commented Apr 18, 2018

KrisThielemans commented Apr 19, 2018

casperdcl commented Apr 26, 2018

KrisThielemans commented Apr 26, 2018

KrisThielemans commented Apr 27, 2018

paskino commented Apr 30, 2018

paskino commented May 1, 2018

KrisThielemans left a comment

KrisThielemans May 2, 2018

paskino May 2, 2018

KrisThielemans May 2, 2018

paskino May 3, 2018

KrisThielemans May 2, 2018

KrisThielemans May 3, 2018

KrisThielemans May 2, 2018

KrisThielemans May 2, 2018

		message(STATUS "Python libraries found")
		message(STATUS "Location of Python modules: " ${PYTHON_DEST})

Fix python dest #160

Fix python dest #160

Conversation

paskino commented Apr 11, 2018

Choose a reason for hiding this comment

paskino Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

KrisThielemans commented Apr 17, 2018

Choose a reason for hiding this comment

casperdcl left a comment

Choose a reason for hiding this comment

KrisThielemans commented Apr 17, 2018

casperdcl commented Apr 18, 2018

paskino commented Apr 18, 2018

KrisThielemans commented Apr 18, 2018

casperdcl commented Apr 18, 2018 • edited Loading

casperdcl commented Apr 18, 2018

paskino commented Apr 18, 2018

casperdcl commented Apr 18, 2018

casperdcl commented Apr 18, 2018

KrisThielemans commented Apr 19, 2018

casperdcl commented Apr 26, 2018

KrisThielemans commented Apr 26, 2018

KrisThielemans commented Apr 27, 2018

paskino commented Apr 30, 2018

paskino commented May 1, 2018

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paskino Apr 17, 2018 •

edited

Loading

casperdcl commented Apr 18, 2018 •

edited

Loading