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 PythonLibs handling #874

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Conversation

kislinsk
Copy link
Contributor

@kislinsk kislinsk commented Aug 5, 2019

  1. The FindPythonLibs module reads PYTHON_LIBRARY as a hint but it also sets the same variable when successful. In the latter case, the variable may be set to a list containing target_link_libraries
    keywords like "optimized" and "debug" instead of a single filepath. When read as a hint, though, FindPythonLibs only understands a single filepath. Repeated unguarded calls to the module may result in a faulty value of PYTHON_LIBRARY like "optimized;optimized;python3.lib;debug;debug;python3d.lib". Such values entail errors when the linker tries to link against the keywords "optimized" or "debug".
  2. PYTHON_LIBRARY may be a mixed list of keywords and filepaths and should not be used directly in get_filename_component()
  3. Same for passing PYTHON_LIBRARY to PythonQt.
  4. The documented output variable of FindPythonLibs is PYTHON_LIBRARIES instead of PYTHON_LIBRARY, which is an old relict from pre CMake 3.0 (minimal required CMake version of CTK).

@pieper pieper requested a review from jcfr August 5, 2019 12:06
@kislinsk
Copy link
Contributor Author

kislinsk commented Aug 5, 2019

Fixed a wrong iterator name I introduced in a last-minute change.

@kislinsk
Copy link
Contributor Author

kislinsk commented Aug 5, 2019

This PR still is not complete, sorry. I just run into a debug linker problem on Windows with CTKScriptingPythonCore for the first time. Eventhough python37_d.lib is set as link dependency something wants to draw in python37.lib. Investigating...

@kislinsk
Copy link
Contributor Author

kislinsk commented Aug 5, 2019

This PR still is not complete, sorry. I just run into a debug linker problem on Windows with CTKScriptingPythonCore for the first time. Eventhough python37_d.lib is set as link dependency something wants to draw in python37.lib. Investigating...

In fact, when linking against python37.lib instead of python37d.lib, everything works fine.

@lassoan
Copy link
Member

lassoan commented Aug 5, 2019

Thank you for working on this, I don't fully understand the details but it's great if you fix such inconsistencies.

Note that we often need to use release-mode Python binaries in applications built in debug-mode because third-party binary packages are only available for release-mode Python. Please make sure that this will remain possible.

@kislinsk
Copy link
Contributor Author

kislinsk commented Aug 5, 2019

Note that we often need to use release-mode Python binaries in applications built in debug-mode because third-party binary packages are only available for release-mode Python. Please make sure that this will remain possible.

No worries, last fix in the pull request is actually right what you describe: prefer using release-mode Python also in debug configuration as debug-mode Python has many implications. Seems to be the adopted standard in other Projects like Qt and Boost as well.

jcfr and others added 2 commits August 21, 2019 10:02
The FindPythonLibs module reads PYTHON_LIBRARY as a hint but it
also sets the same variable when successful. In the latter case,
the variable may be set to a list containing target_link_libraries
keywords like "optimized" and "debug" instead of a single filepath.
When read as a hint, though, FindPythonLibs only understands a
single filepath. Repeated unguarded calls to the module may result
in a faulty value of PYTHON_LIBRARY like:

  "optimized;optimized;optimized;python3.lib;debug;debug;python3d.lib"

Such values entail errors when the linker tries to link against
the keywords "optimized" or "debug".

This commit implements the following changes:

* Fix usage of FindPythonLibs result variables

  The documented output variable of FindPythonLibs is
  PYTHON_LIBRARIES instead of PYTHON_LIBRARY, which is an old
  relict from pre CMake 3.0 (minimal required CMake version of CTK).

* Fix linking to Python on Windows in Debug configuration

  Linking to a debug version of Python has many implications and is
  generally quirky enough that many famous projects like Boost and
  Qt decided to link against the release version of Python instead.

  Handling of PYTHON_LIBRARIES/PYTHON_LIBRARY was refactored into
  ctkFunctionExtractOptimizedLibrary.

  For non-MSVC/non-multiconfig build systems it is basically a NO-OP
  and does not change behavior.

Signed-off-by: Stefan Dinkelacker <s.dinkelacker@dkfz-heidelberg.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants