Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix/simplify Python CMake arguments #473

Merged
merged 2 commits into from
Jan 18, 2021

Conversation

KrisThielemans
Copy link
Member

  • Created a variable PYTHONLIBS_CMAKE_ARGS that contains all settings to be used.

  • No longer pass PYTHON_INCLUDE_DIR from ...DIRS, nor PYTHON_LIBRARY from ..LIBRARIES, but from the variable itself. This caused problems when there are multiple files, and in particular debug and release Python libraries present.

Fixes #472

- Created a variable PYTHONLIBS_CMAKE_ARGS that contains all
settings to be used.

- No longer pass PYTHON_INCLUDE_DIR from ...DIRS, nor
PYTHON_LIBRARY from ..LIBRARIES, but from the variable
itself. This caused problems when
there are multiple files, and in particular debug and release
Python libraries present.

Fixes SyneRBI#472
@KrisThielemans KrisThielemans added this to the v2.3 milestone Jan 15, 2021
@KrisThielemans KrisThielemans linked an issue Jan 15, 2021 that may be closed by this pull request
@KrisThielemans KrisThielemans linked an issue Jan 15, 2021 that may be closed by this pull request
SuperBuild.cmake Outdated
@@ -232,8 +247,11 @@ option(BUILD_pet_rd_tools "Build pet_rd_tools" OFF)
option(BUILD_CIL "Build CCPi CIL Modules and ASTRA engine" OFF)
option(BUILD_CIL_LITE "Build CCPi CIL Modules" OFF)
option(BUILD_NIFTYREG "Build NIFTYREG" ON)
option(BUILD_SIRF_Contribs "Build SIRF-Contribs" ON)

if (BUILD_PYTHON)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's no guarantee that the contributions will always be limited to python.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's true, but at the moment it does

INSTALL_COMMAND ${CMAKE_COMMAND} -E copy_directory ${${proj}_SOURCE_DIR}/src/Python/sirf/ ${PYTHON_DEST}/sirf

which fails if PYTHON_DOEST isn't set.

Maybe a better fix would be to fix that line

Copy link
Contributor

@rijobro rijobro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't tested it obviously, but looks good to me!

@KrisThielemans
Copy link
Member Author

@rijobro I have now used a different strategy for SIRF-contribs

@KrisThielemans KrisThielemans merged commit cfecb01 into SyneRBI:master Jan 18, 2021
@KrisThielemans KrisThielemans deleted the FixPython branch January 18, 2021 08:12
@AnderBiguri
Copy link
Contributor

Sorry I didn't test this before, but I get:

CMake Error at SuperBuild.cmake:146 (if):
  if given arguments:

    "EXISTS" "optimized" "C:/Users/Ander/AppData/Local/Programs/Python/Python38/libs/python38.lib" "debug" "C:/Users/Ander/AppData/Local/Programs/Python/Python38/libs/python38_d.lib"

  Unknown arguments specified
Call Stack (most recent call first):
  CMakeLists.txt:37 (include)

@KrisThielemans
Copy link
Member Author

bugger. I guess I was too optimistic. I believe this is due to yet another bug in FindHDF5.cmake (it shouldn't set PYTHON_LIBRARY like this, but apparently does). Could you try by putting some quotes around it?

if (EXISTS "${PYTHON_LIBRARY}")

also for the include files.

@AnderBiguri
Copy link
Contributor

AnderBiguri added a commit to AnderBiguri/SIRF-SuperBuild that referenced this pull request Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants