[Lldb-commits] [PATCH] D13404: Teach CMake to find versions of Python != 2.7

[Vadim Macagon via lldb-commits]

enlight added inline comments.

================
Comment at: cmake/modules/LLDBConfig.cmake:109
@@ +108,3 @@
+  set (PYTHON_EXECUTABLE $<$<CONFIG:Debug>:${PYTHON_DEBUG_EXE}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_EXE}> PARENT_SCOPE)
+  set (PYTHON_LIBRARY $<$<CONFIG:Debug>:${PYTHON_DEBUG_LIB}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_LIB}> PARENT_SCOPE)
+  set (PYTHON_DLL $<$<CONFIG:Debug>:${PYTHON_DEBUG_DLL}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_DLL}> PARENT_SCOPE)
----------------
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)
```


================
Comment at: cmake/modules/LLDBConfig.cmake:112-114
@@ +111,5 @@
+
+  if (NOT LLDB_RELOCATABLE_PYTHON)
+    add_definitions( -DLLDB_PYTHON_HOME="${PYTHON_HOME}" )
+  endif()
+
----------------
I think this belongs outside this function, along with `include_directories` below.

================
Comment at: cmake/modules/LLDBConfig.cmake:117
@@ +116,3 @@
+  if (PYTHON_LIBRARY)
+    include_directories(${PYTHON_INCLUDE_DIR})
+  endif()
----------------
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 [[ https://cmake.org/cmake/help/v2.8.12/cmake.html#module:FindPythonLibs | FindPythonLibs ]] module (which means `PYTHON_INCLUDE_DIRS` should be made available in the parent scope, note the extra `S`).

================
Comment at: cmake/modules/LLDBConfig.cmake:131-133
@@ -104,1 +130,5 @@
+    find_python_libs_windows()
+    message("-- Found PythonExecutable: ${PYTHON_EXECUTABLE}")
+    message("-- Found PythonLibs: ${PYTHON_LIBRARY}")
+    message("-- Found PythonDLL: ${PYTHON_DLL}")
   else()
----------------
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.

================
Comment at: cmake/modules/LLDBStandalone.cmake:51-53
@@ -50,5 +50,5 @@
 
   # Verify that we can find a Python 2 interpreter.  Python 3 is unsupported.
   if (PYTHON_EXECUTABLE STREQUAL "")
-    set(Python_ADDITIONAL_VERSIONS 2.7 2.6 2.5)
+    set(Python_ADDITIONAL_VERSIONS 3.5 3.4 3.3 3.2 3.1 3.0 2.7 2.6 2.5)
     include(FindPythonInterp)
----------------
The comment no longer seems accurate.


http://reviews.llvm.org/D13404