[Cmake] Use the modern way to find Python when possible

[JDevlieghere]

Summary

CMake has a much improved way to find the Python interpreter and libraries which guarantees that the interpreter and libraries are the same. However, this functionality is only available in CMake 3.12 and later. This patch changes the CMake logic to use that without bumping the minimum CMake version.

Question for people familiar with CMake on Windows: do you think that FindPython could replace the current logic in find_python_libs_windows? So we could do something like:

if(NOT CMAKE_VERSION VERSION_LESS "3.12")
  # Use FindPython
else()
  if (Windows) 
   # use find_python_libs_windows
  else()
   # use PythonInterp / PythonLibs
  endif()
endif()

[amccarth]

An aside ...

I'm still trying to get back to a buildable state the earlier changes, like the one that tries to enforce version consistency between the libs and the interpreter. I'm currently bisecting to figure out what I hope is the final blocker.

For me, the find_package(PythonInterp) call was always finding the older interpreter (2.7) even though everything pointed to the newer one (3.6). That tripped the version compatibility check that was recently added. The algorithm used by find_package isn't well documented (as far as I can tell), so I couldn't even tell you whether it was doing the wrong thing. I can tell you that the version check seemed unnecessary, as lldb built and tested fine when I locally removed that check, despite the fact that the versions didn't match.

The only way I was able to make things work again was to completely remove Python 2.7 from my machine. Oddly, the uninstaller also took make with it, so then I couldn't run lldb tests. With make re-installed, I now have a some crashes during tests to the Python allocator being called without the GIL being held. I'm currently bisecting to figure out which change caused that. This crash is especially painful, as it leaves an invisible Python process running and holding files open, which breaks subsequent builds.

To your question ...

Without understanding how either version of find_package finds Python, it's hard to say whether it solves the problems outlined in Zach's comment at the top of find_python_libs. Zach's first two points don't apply anymore, as we're well past MSVC 2015. But the third one, regarding a 32-bit CMake being able to find a 64-bit Python, seems like it would still be a problem.

[JDevlieghere]

In D64881#1590204, @amccarth wrote:

An aside ...

I'm still trying to get back to a buildable state the earlier changes, like the one that tries to enforce version consistency between the libs and the interpreter. I'm currently bisecting to figure out what I hope is the final blocker.

Maybe we should just revert D64443? This is what broke my setup, and led me to introduce to the consistency check in the first place, which merely diagnoses a real problem.

For me, the find_package(PythonInterp) call was always finding the older interpreter (2.7) even though everything pointed to the newer one (3.6). That tripped the version compatibility check that was recently added. The algorithm used by find_package isn't well documented (as far as I can tell), so I couldn't even tell you whether it was doing the wrong thing. I can tell you that the version check seemed unnecessary, as lldb built and tested fine when I locally removed that check, despite the fact that the versions didn't match.

This is interesting, for me on macOS, it was the other way around. It would always find the 3.7 interpreter because of the LLVM change and never find the corresponding python libs for 3.7 in LLDB. I had to remove every trace of the Python 3 interpreter, for it to only find the 2.7 one. Anyway, I think we all agree (CMake developers included) that the old way of doing things is inherently broken. That's why I want to start adopting the new FindPython.

The only way I was able to make things work again was to completely remove Python 2.7 from my machine. Oddly, the uninstaller also took make with it, so then I couldn't run lldb tests. With make re-installed, I now have a some crashes during tests to the Python allocator being called without the GIL being held. I'm currently bisecting to figure out which change caused that. This crash is especially painful, as it leaves an invisible Python process running and holding files open, which breaks subsequent builds.

To your question ...

Without understanding how either version of find_package finds Python, it's hard to say whether it solves the problems outlined in Zach's comment at the top of find_python_libs. Zach's first two points don't apply anymore, as we're well past MSVC 2015. But the third one, regarding a 32-bit CMake being able to find a 64-bit Python, seems like it would still be a problem.

Thanks, I guess we'd just need to try it then.

[stella.stamenova]

Re: Windows CMake: I am not sure, I'd have to do look into it further.

On a separate note: I haven't looked into it yet, but we have a setup to run the LLDB tests on Ubuntu and that started failing somewhat recently with some Swig and Python errors. For example:

Unable to load lldb extension module.  Possible reasons for this include:
  1) LLDB was built with LLDB_DISABLE_PYTHON=1
  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to
     the version of Python that LLDB built and linked against, and PYTHONPATH
     should contain the Lib directory for the same python distro, as well as the
     location of LLDB's site-packages folder.
  3) A different version of Python than that which was built against is exported in
     the system's PATH environment variable, causing conflicts.
  4) The executable '/home/e2admin/vstsdrive/_work/29/b/bin/lldb' could not be found.  Please check 
     that it exists and is executable.
Traceback (most recent call last):
  File "/home/e2admin/vstsdrive/_work/29/s/lldb/test/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/home/e2admin/vstsdrive/_work/29/s/lldb/packages/Python/lldbsuite/test/dotest.py", line 1268, in run_suite
    import lldb
ImportError: No module named 'lldb'

I am not sure how this change is going to affect that - better or worse?

[JDevlieghere]

In D64881#1590273, @stella.stamenova wrote:

Re: Windows CMake: I am not sure, I'd have to do look into it further.

On a separate note: I haven't looked into it yet, but we have a setup to run the LLDB tests on Ubuntu and that started failing somewhat recently with some Swig and Python errors. For example:

Unable to load lldb extension module.  Possible reasons for this include:
  1) LLDB was built with LLDB_DISABLE_PYTHON=1
  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to
     the version of Python that LLDB built and linked against, and PYTHONPATH
     should contain the Lib directory for the same python distro, as well as the
     location of LLDB's site-packages folder.
  3) A different version of Python than that which was built against is exported in
     the system's PATH environment variable, causing conflicts.
  4) The executable '/home/e2admin/vstsdrive/_work/29/b/bin/lldb' could not be found.  Please check 
     that it exists and is executable.
Traceback (most recent call last):
  File "/home/e2admin/vstsdrive/_work/29/s/lldb/test/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/home/e2admin/vstsdrive/_work/29/s/lldb/packages/Python/lldbsuite/test/dotest.py", line 1268, in run_suite
    import lldb
ImportError: No module named 'lldb'

I am not sure how this change is going to affect that - better or worse?

Yes, this should solve (3). However, this should've already been caught at configure-time by my change in https://reviews.llvm.org/rL366243.

[amccarth]

In D64881#1590252, @JDevlieghere wrote:

In D64881#1590204, @amccarth wrote:

An aside ...

I'm still trying to get back to a buildable state the earlier changes, like the one that tries to enforce version consistency between the libs and the interpreter. I'm currently bisecting to figure out what I hope is the final blocker.

Maybe we should just revert D64443? This is what broke my setup, and led me to introduce to the consistency check in the first place, which merely diagnoses a real problem.

It wasn't a real problem on Windows, where we've been using Python 3.x for LLDB for a long time (predating the patch you propose rolling back).

For me, the find_package(PythonInterp) call was always finding the older interpreter (2.7) even though everything pointed to the newer one (3.6). That tripped the version compatibility check that was recently added. The algorithm used by find_package isn't well documented (as far as I can tell), so I couldn't even tell you whether it was doing the wrong thing. I can tell you that the version check seemed unnecessary, as lldb built and tested fine when I locally removed that check, despite the fact that the versions didn't match.

This is interesting, for me on macOS, it was the other way around. It would always find the 3.7 interpreter because of the LLVM change and never find the corresponding python libs for 3.7 in LLDB. I had to remove every trace of the Python 3 interpreter, for it to only find the 2.7 one. Anyway, I think we all agree (CMake developers included) that the old way of doing things is inherently broken. That's why I want to start adopting the new FindPython.

The only way I was able to make things work again was to completely remove Python 2.7 from my machine. Oddly, the uninstaller also took make with it, so then I couldn't run lldb tests. With make re-installed, I now have a some crashes during tests to the Python allocator being called without the GIL being held. I'm currently bisecting to figure out which change caused that. This crash is especially painful, as it leaves an invisible Python process running and holding files open, which breaks subsequent builds.

To your question ...

Without understanding how either version of find_package finds Python, it's hard to say whether it solves the problems outlined in Zach's comment at the top of find_python_libs. Zach's first two points don't apply anymore, as we're well past MSVC 2015. But the third one, regarding a 32-bit CMake being able to find a 64-bit Python, seems like it would still be a problem.

Thanks, I guess we'd just need to try it then.

Testing the matrix of 32/64, 64/32, Python 2/3, debug/release, seems like a lot of work. I'd be pretty surprised to learn that CMake did extra, Windows-specific work necessary to make find_package do the right thing in the face of registry and/or filesystem redirection.

[mgorny]

In D64881#1590330, @amccarth wrote:

Testing the matrix of 32/64, 64/32, Python 2/3, debug/release, seems like a lot of work. I'd be pretty surprised to learn that CMake did extra, Windows-specific work necessary to make find_package do the right thing in the face of registry and/or filesystem redirection.

Well, apparently it has *some* support for registry (see: Python_FIND_REGISTRY).

[mgorny]

That said, I ended up having mixed feelings about this.

On the plus side, it definitely forces matching Python version for me. For example, without this patch changing PYTHON_EXECUTABLE resulted in mismatch. If I wanted to change Python version, I had to either update all vars manually or remove them from cache.

On the minus side, I can't seem to find a way to force a particular interpreter. Overriding Python_EXECUTABLE just doesn't work, and it always forces 3.7 for me. The logic is rather complex, and I'm not sure if it's possible at all. Maybe we should wait for the code to mature a bit.

[mgorny]

Filed https://gitlab.kitware.com/cmake/cmake/issues/19492 for that.

[labath]

I haven't been keeping up with all the python cmake patches, but I'm not sure this will help the situation, as it will add another dimension to the configuration matrix. If we're still going to support cmake<3.12, then we're going to need to make the other branch of this work anyway... Maybe once cmake 3.12 gets a bit older (widely available), we could just switch to the other method and say that cmake>=3.12 is required if you're going to build lldb...

[JDevlieghere]

Alright, it appears that the consensus is that this isn't desirable for now.

From our side, r366447 fixed all the issues we were experiencing, so I'm abandoning this.

[hans]

In D64881#1590204, @amccarth wrote:

An aside ...

I'm still trying to get back to a buildable state the earlier changes, like the one that tries to enforce version consistency between the libs and the interpreter. I'm currently bisecting to figure out what I hope is the final blocker.

Do you have the revisions for these changes?

I built successfully at r363781, but now I can't build anymore.

I'm configuring with:

cmake -GNinja %cmake_flags% -DPYTHON_HOME=C:\Users\hwennborg\AppData\Local\Programs\Python\Python36-32

And get the error:

CMake Error at tools/lldb/cmake/modules/LLDBConfig.cmake:204 (message):

Found incompatible Python interpreter (2.7.15) and Python libraries (3.6.4)

Uninstalling Python 2.7 is not an option for me.

[labath]

I think setting -DPYTHON_EXECUTABLE=C:\Users\hwennborg\AppData\Local\Programs\Python\Python36-32\python.exe (or something) should be enough to get cmake to select python3 for the interpreter as well.

As for the version mismatch, the one of the reasons why the mismatched version did not work on unix is because some of our python build scripts assume that lldb will be running the same version of python as they are and derive python site-packages dir based on that. This is bad practice because of cross-compiling, but it may not be an issue on windows because there the site-packages dir does not include the python version (lib/site-packages vs lib/pythonX.Y/site-packages). So it's possible we can skip the version mismatch check on windows.

[hans]

In D64881#1596920, @labath wrote:

I think setting -DPYTHON_EXECUTABLE=C:\Users\hwennborg\AppData\Local\Programs\Python\Python36-32\python.exe (or something) should be enough to get cmake to select python3 for the interpreter as well.

Cool, that works. Thanks!

lldb/cmake/modules/LLDBConfig.cmake

function(find_python_libs_windows)
   set (PYTHON_INCLUDE_DIR ${PYTHON_INCLUDE_DIR} PARENT_SCOPE)
 
   message(STATUS "LLDB Found PythonExecutable: ${PYTHON_EXE}")
   message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIB}")
   message(STATUS "LLDB Found PythonDLL: ${PYTHON_DLL}")
   message(STATUS "LLDB Found PythonIncludeDirs: ${PYTHON_INCLUDE_DIR}")
 endfunction(find_python_libs_windows)
 
+function(find_python_libs)
+  if(NOT CMAKE_VERSION VERSION_LESS "3.12")
+    find_package(Python COMPONENTS Interpreter Development)
+    if(Python_FOUND)
+      set(PYTHON_EXECUTABLE          ${Python_EXECUTABLE})
+      set(PYTHON_LIBRARY             ${Python_LIBRARIES})
+      set(PYTHON_INCLUDE_DIR         ${Python_INCLUDE_DIRS})
+      set(PYTHON_VERSION_STRING      ${Python_VERSION})
+      set(PYTHONLIBS_VERSION_STRING  ${Python_VERSION})
+
+      set(PYTHON_EXECUTABLE          ${PYTHON_EXECUTABLE} PARENT_SCOPE)
+      set(PYTHON_LIBRARY             ${PYTHON_LIBRARY} PARENT_SCOPE)
+      set(PYTHON_INCLUDE_DIR         ${PYTHON_INCLUDE_DIR} PARENT_SCOPE)
+      set(PYTHON_VERSION_STRING      ${PYTHON_VERSION_STRING} PARENT_SCOPE)
+      set(PYTHONLIBS_VERSION_STRING  ${PYTHONLIBS_VERSION_STRING} PARENT_SCOPE)
+    endif()
+  else()
+    find_package(PythonInterp REQUIRED)
+    find_package(PythonLibs REQUIRED)
+  endif()
+endfunction(find_python_libs)
+
 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)
+    find_python_libs()
   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})")
   endif()
 
   if (PYTHON_INCLUDE_DIR)