This is an archive of the discontinued LLVM Phabricator instance.

Teach CMake to find versions of Python != 2.7
ClosedPublic

Authored by zturner on Oct 2 2015, 5:54 PM.

Details

Reviewers
zturner
enlight
Summary

Hi Vadim,

Would you mind taking a look at this? My CMake-fu is only level 5, so if you have any suggestions on how to improve this, let me know.

Note that I'm claiming with this revision that LLDB itself works with Python versions other than 2.7. Only that CMake can set all the variables appropriately if you point it to a different version of CMake

Diff Detail

Event Timeline

zturner updated this revision to Diff 36422.Oct 2 2015, 5:54 PM
zturner retitled this revision from to Teach CMake to find versions of Python != 2.7.
zturner updated this object.
zturner added a reviewer: enlight.
zturner added a subscriber: lldb-commits.
enlight added inline comments.Oct 3 2015, 12:19 AM
cmake/modules/LLDBConfig.cmake
109

PYTHON_LIBRARY is used in this scope later on, so it needs to be explicitly set for both this scope and the parent scope, like so:

set (PYTHON_LIBRARY $<$<CONFIG:Debug>:${PYTHON_DEBUG_LIB}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_LIB}>)
set (PYTHON_LIBRARY ${PYTHON_LIBRARY} PARENT_SCOPE)
112–114

I think this belongs outside this function, along with include_directories below.

117

This command will never be executed because PYTHON_LIBRARY is only set in the parent scope. However, I'd prefer include_directories() wasn't here at all, let the caller of find_python_libs_windows do that instead so that the behavior is more similar to the built-in FindPythonLibs module (which means PYTHON_INCLUDE_DIRS should be made available in the parent scope, note the extra S).

131–133

Please move these into find_python_libs_windows(), as I'll need access to the debug/release paths in order to avoid printing out the generator expressions.

cmake/modules/LLDBStandalone.cmake
51–53

The comment no longer seems accurate.

enlight added inline comments.Oct 3 2015, 12:39 AM
cmake/modules/LLDBConfig.cmake
56

This will only set LLDB_DISABLE_PYTHON inside the function, you probably want to do set (LLDB_DISABLE_PYTHON 1 PARENT_SCOPE), or set (LLDB_DISABLE_PYTHON 1 CACHE INTERNAL "") instead.

73

Ditto.

zturner updated this revision to Diff 36544.Oct 5 2015, 1:31 PM

Updated from Vadim's comments.

enlight added inline comments.Oct 5 2015, 9:35 PM
cmake/modules/LLDBConfig.cmake
113

You missed :P

140

Can we make this if (PYTHON_INCLUDE_DIRS) instead?

enlight edited edge metadata.Oct 8 2015, 5:59 AM

Hi Zach,

Can you close this issue please?

zturner accepted this revision.Oct 8 2015, 9:30 AM
zturner added a reviewer: zturner.

In the future, u sually the reviewer (you in this case) does this by hitting Accept in Phabricator. I can do it this time though, no worries

This revision is now accepted and ready to land.Oct 8 2015, 9:30 AM
enlight accepted this revision.Oct 13 2015, 9:53 AM
enlight edited edge metadata.
zturner closed this revision.Oct 15 2015, 1:49 PM