[PATCH] D78762: build: use `find_package(Python3)` if available

Mon Apr 27 09:06:36 PDT 2020

JDevlieghere added a comment.

In D78762#2002390 <https://reviews.llvm.org/D78762#2002390>, @compnerd wrote:

> @JDevlieghere I think that adding another mechanism for finding the python executable is not the right approach.  You already have the variables that you are talking about, you just need to specify it in triplicate if you want compatibility across all the versions, but specifying `-DPython3_EXECUTABLE=` gives you the control over the executable for python3.  If you want to use python2, `-DPython3_EXECUTABLE=` and `-DPython2_EXECUTABLE=` will ensure that python2 is always used.  If you are using an older CMake, specifying `-DPython3_EXECUTABLE=`, `-DPython2_EXECUTABLE=` and `-DPYTHON_EXECUTABLE=` will ensure that the specified version is used.  I'm not sure what the scenario is that you believe will be made more difficult with this approach.

It only //works// because you're setting Python3_EXECUTABLE to Python2_EXECUTABLE.

  set(Python3_EXECUTABLE ${Python2_EXECUTABLE})

This will be completely opaque to lldb and we'll have to struggle again to reverse engineer which Python is used, leaving us in the same situation as today. How are we going to choose between Python 2 and Python 3 based on that variable? What I envision is to find Python once, like we do with other dependencies in the LLVM project, and then use that (interpreter, libraries, include paths) in LLDB. This has to work for in-tree-builds as well as out-of-tree builds (I'm not a fan but hey they're still supported).

In D78762#2005407 <https://reviews.llvm.org/D78762#2005407>, @ldionne wrote:

> In D78762#2002305 <https://reviews.llvm.org/D78762#2002305>, @JDevlieghere wrote:
>
> > My suggestion is to have 4 new CMake variable that abstract over this: `LLVM_PYTHON_EXECUTABLE`, `LLVM_PYTHON_LIBRARIES`, `LLVM_PYTHON_INCLUDE_DIRS` and finally `LLVM_PYTHON_HINT`.  We can then populate the first three depending on what machinery we want to use, i.e. the old CMake way of finding Python (until we bump the requirement across llvm), as well as the new `FindPython3` and `FindPython2`. Similarly for the `HINT` variable, having our own abstraction means we don't clobber any of the variables used internally by CMake.
> >
> > What do you think?
>
>
> This is not better IMHO since it assumes that all subprojects are using the `LLVM_meow` variable to refer to the Python executable.

This patch is already changing the variable. How is this different/worse from using `Python3_EXECUTABLE` and having it **maybe** point to a Python 2 interpreter?

================
Comment at: clang/CMakeLists.txt:154
+        message(WARNING "Python3 not found, using python2 as a fallback")
+        find_package(Python3 COMPONENTS Interpreter REQUIRED)
+        if(Python2_VERSION VERSION_LESS 2.7)
----------------
Python2?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78762/new/

https://reviews.llvm.org/D78762