[CMake] Loosen Python version check and ignore patch version

[JDevlieghere]

Summary

Some versions of macOS report a different patch version for the system provided interpreter and libraries.

[sgraenitz]

Interesting way to get from version to list! :)

Cosmetics: It looks like PYTHON_VERSION_STRING and PYTHONLIBS_VERSION_STRING are global and all others are local variables? Using lowercase names for locals makes it easier to see. As names are case-sensitive, it would also avoid the collision with the actual PYTHON_MAJOR/MINOR.

lldb/cmake/modules/LLDBConfig.cmake

205 ↗

(On Diff #211559)

I think CMake would not shadow, but overwrite them in directory scope (which is root here, so everywhere except the cache).

[JDevlieghere]

Use lowercase variables for locals.

[labath]

lldb/cmake/modules/LLDBConfig.cmake

203–204 ↗

(On Diff #211606)

If this is the only reason, then maybe we could fix find_python_libs_windows to set the relevant variables instead (?)

[JDevlieghere]

lldb/cmake/modules/LLDBConfig.cmake

203–204 ↗

(On Diff #211606)

FindPythonLibs only sets PYTHONLIBS_VERSION_STRING, so we'd still have that to parse the major and minor version of that manually. It was a coin flip between keeping the parsing together and updating find_python_libs_windows. I'm happy to do either.

[labath]

lldb/cmake/modules/LLDBConfig.cmake

203–204 ↗

(On Diff #211606)

Yea, I see what you mean. I think I'd go with changing find_python_libs_windows just to minimize the difference between the windows and non-windows branches of the python-finding code (and possible side-effects of those differences), but it's not a big deal either way...

[JDevlieghere]

Export the same variables in find_python_libs_windows as FindPythonInterp and FindPythonLibs.

[JDevlieghere]

@amccarth I think this might explain the issues you were having last week. Without this patch, find_python_libs_windows is not setting the expected version variables. As a result, my check was complaining about a mismatch between the Python interpreter that LLVM found, and the libs that LLDB found, while LLDB was actually using a different Python interpreter, found by find_python_libs_windows.

lldb/cmake/modules/LLDBConfig.cmake

203–204 ↗

(On Diff #211606)

Sure, sounds good!

[sgraenitz]

LGTM

[labath]

Thanks.

BTW, I tried this on windows, and it didn't blow up in my face, I think it should be relatively safe. I couldn't reproduce the problems that @amccarth was experiencing, so I don't know if it fixes that problem. It would be good to know whether it does, as we could cherry-pick this to the release branch in that case.

[amccarth]

In D65230#1602370, @labath wrote:

Thanks.

BTW, I tried this on windows, and it didn't blow up in my face, I think it should be relatively safe. I couldn't reproduce the problems that @amccarth was experiencing, so I don't know if it fixes that problem. It would be good to know whether it does, as we could cherry-pick this to the release branch in that case.

Sorry for the slow reply. I've been in a conference most of the week.

Since my problem tends to be a "just me" problem that doesn't affect the bots or apparently anyone else, I don't think a cherrypick is necessary.

By having only one version of Python on my machine, I was able to work around that problem. Unfortunately, I've been stuck on another problem since making that change (specifically, all dotest.py tests report "Python memory allocator called without holding the GIL" and crash). This, too, seems to be a "just me" thing, and I'm continuing to work on that this morning. Until I get that figured out, I don't think I want to perturb my environment further by re-installing a second version of Python. I suspect the other patch to eliminate multiprocessing from dotest.py might solve the GIL problem as well, but I want to know more first.

[amccarth]

Just closing the loop. Regarding the "Python memory allocator called without holding the GIL" bugs: this is not a "just me" problem. It's for anyone who enables debug mode. This appears to be a test infrastructure problem, but it might be harmless. If I run in "release" mode, I get the same behavior as the Windows build bot.

lldb/trunk/cmake/modules/LLDBConfig.cmake

function(find_python_libs_windows)
 
   if(EXISTS "${PYTHON_INCLUDE_DIR}/patchlevel.h")
     file(STRINGS "${PYTHON_INCLUDE_DIR}/patchlevel.h" python_version_str
          REGEX "^#define[ \t]+PY_VERSION[ \t]+\"[^\"]+\"")
     string(REGEX REPLACE "^#define[ \t]+PY_VERSION[ \t]+\"([^\"+]+)[+]?\".*" "\\1"
          PYTHONLIBS_VERSION_STRING "${python_version_str}")
     message(STATUS "Found Python library version ${PYTHONLIBS_VERSION_STRING}")
     string(REGEX REPLACE "([0-9]+)[.]([0-9]+)[.][0-9]+" "python\\1\\2" PYTHONLIBS_BASE_NAME "${PYTHONLIBS_VERSION_STRING}")
-    set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}" PARENT_SCOPE)
     unset(python_version_str)
   else()
     message(WARNING "Unable to find ${PYTHON_INCLUDE_DIR}/patchlevel.h, Python installation is corrupt.")
     message(WARNING "Python support will be disabled for this build.")
     set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
     return()
   endif()
 
function(find_python_libs_windows)
   endforeach()
 
   if (NOT PYTHON_EXE OR NOT PYTHON_LIB OR NOT PYTHON_DLL)
     message(WARNING "Unable to find all Python components.  Python support will be disabled for this build.")
     set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
     return()
   endif()
 
-  set (PYTHON_EXECUTABLE ${PYTHON_EXE} PARENT_SCOPE)
-  set (PYTHON_LIBRARY ${PYTHON_LIB} PARENT_SCOPE)
-  set (PYTHON_DLL ${PYTHON_DLL} PARENT_SCOPE)
-  set (PYTHON_INCLUDE_DIR ${PYTHON_INCLUDE_DIR} PARENT_SCOPE)
+  # Find the version of the Python interpreter.
+  execute_process(COMMAND "${PYTHON_EXE}" -c
+                  "import sys; sys.stdout.write(';'.join([str(x) for x in sys.version_info[:3]]))"
+                  OUTPUT_VARIABLE PYTHON_VERSION_OUTPUT
+                  RESULT_VARIABLE PYTHON_VERSION_RESULT
+                  ERROR_QUIET)
+  if(NOT PYTHON_VERSION_RESULT)
+    string(REPLACE ";" "." PYTHON_VERSION_STRING "${PYTHON_VERSION_OUTPUT}")
+    list(GET PYTHON_VERSION_OUTPUT 0 PYTHON_VERSION_MAJOR)
+    list(GET PYTHON_VERSION_OUTPUT 1 PYTHON_VERSION_MINOR)
+    list(GET PYTHON_VERSION_OUTPUT 2 PYTHON_VERSION_PATCH)
+  endif()
+
+  # Set the same variables as FindPythonInterp and FindPythonLibs.
+  set(PYTHON_EXECUTABLE ${PYTHON_EXE} PARENT_SCOPE)
+  set(PYTHON_LIBRARY ${PYTHON_LIB} PARENT_SCOPE)
+  set(PYTHON_DLL ${PYTHON_DLL} PARENT_SCOPE)
+  set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}" PARENT_SCOPE)
+  set(PYTHON_VERSION_STRING ${PYTHON_VERSION_STRING} PARENT_SCOPE)
+  set(PYTHON_VERSION_MAJOR ${PYTHON_VERSION_MAJOR} PARENT_SCOPE)
+  set(PYTHON_VERSION_MINOR ${PYTHON_VERSION_MINOR} PARENT_SCOPE)
+  set(PYTHON_VERSION_PATCH ${PYTHON_VERSION_PATCH} PARENT_SCOPE)
 
-  message(STATUS "LLDB Found PythonExecutable: ${PYTHON_EXE}")
-  message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIB}")
+  message(STATUS "LLDB Found PythonExecutable: ${PYTHON_EXECUTABLE}")
+  message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIBRARY}")
   message(STATUS "LLDB Found PythonDLL: ${PYTHON_DLL}")
   message(STATUS "LLDB Found PythonIncludeDirs: ${PYTHON_INCLUDE_DIR}")
 endfunction(find_python_libs_windows)
 
 if (NOT LLDB_DISABLE_PYTHON)
   if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
     find_python_libs_windows()
 
     if (NOT LLDB_RELOCATABLE_PYTHON)
       file(TO_CMAKE_PATH "${PYTHON_HOME}" LLDB_PYTHON_HOME)
       add_definitions( -DLLDB_PYTHON_HOME="${LLDB_PYTHON_HOME}" )
     endif()
   else()
     find_package(PythonInterp REQUIRED)
     find_package(PythonLibs REQUIRED)
   endif()
 
-  if (NOT PYTHON_VERSION_STRING VERSION_EQUAL PYTHONLIBS_VERSION_STRING AND
-      NOT CMAKE_CROSSCOMPILING)
-    message(FATAL_ERROR "Found incompatible Python interpreter (${PYTHON_VERSION_STRING}) and Python libraries (${PYTHONLIBS_VERSION_STRING})")
+  if (NOT CMAKE_CROSSCOMPILING)
+    string(REPLACE "." ";" pythonlibs_version_list ${PYTHONLIBS_VERSION_STRING})
+    list(GET pythonlibs_version_list 0 pythonlibs_major)
+    list(GET pythonlibs_version_list 1 pythonlibs_minor)
+
+    # Ignore the patch version. Some versions of macOS report a different patch
+    # version for the system provided interpreter and libraries.
+    if (NOT PYTHON_VERSION_MAJOR VERSION_EQUAL pythonlibs_major OR
+        NOT PYTHON_VERSION_MINOR VERSION_EQUAL pythonlibs_minor)
+      message(FATAL_ERROR "Found incompatible Python interpreter (${python_major}.${python_minor})"
+                          " and Python libraries (${pythonlibs_major}.${pythonlibs_minor})")
+    endif()
   endif()
 
   if (PYTHON_INCLUDE_DIR)
     include_directories(${PYTHON_INCLUDE_DIR})
   endif()
 endif()
 
 if (LLDB_DISABLE_PYTHON)