[CMake] Require python 3.6 if enabling LLVM test targets

[ctetreau]

Summary

The lit test suite uses python 3.6 features. Rather than a strange
python syntax error upon running the lit tests, we will require the
correct version in CMake.

[yln]

LGTM, thanks! (I got bitten by this.)

[JDevlieghere]

However, the project claims to require 3.6 or greater, and 3.6 features are being used.

Can you link to where it says that? I'm not opposed to making 3.6 the minimally required version, but at least for LLDB we don't have that requirement and we have documentation mentioning Python 3.5.

[yln]

In D95635#2528851, @JDevlieghere wrote:

However, the project claims to require 3.6 or greater, and 3.6 features are being used.

Can you link to where it says that? I'm not opposed to making 3.6 the minimally required version, but at least for LLDB we don't have that requirement and we have documentation mentioning Python 3.5.

For LLVM: https://llvm.org/docs/GettingStarted.html#software

[ctetreau]

In D95635#2528851, @JDevlieghere wrote:

However, the project claims to require 3.6 or greater, and 3.6 features are being used.

Can you link to where it says that? I'm not opposed to making 3.6 the minimally required version, but at least for LLDB we don't have that requirement and we have documentation mentioning Python 3.5.

In D95635#2528883, @yln wrote:

In D95635#2528851, @JDevlieghere wrote:

However, the project claims to require 3.6 or greater, and 3.6 features are being used.

Can you link to where it says that? I'm not opposed to making 3.6 the minimally required version, but at least for LLDB we don't have that requirement and we have documentation mentioning Python 3.5.

For LLVM: https://llvm.org/docs/GettingStarted.html#software

This is where I saw it. I got bit by @yln's cleanup patch. I can reduce the minimum version for LLDB if that's the officially required version, but llvm at the very least advertises 3.6

[JDevlieghere]

In D95635#2529027, @ctetreau wrote:

In D95635#2528851, @JDevlieghere wrote:

However, the project claims to require 3.6 or greater, and 3.6 features are being used.

Can you link to where it says that? I'm not opposed to making 3.6 the minimally required version, but at least for LLDB we don't have that requirement and we have documentation mentioning Python 3.5.

In D95635#2528883, @yln wrote:

In D95635#2528851, @JDevlieghere wrote:

However, the project claims to require 3.6 or greater, and 3.6 features are being used.

Can you link to where it says that? I'm not opposed to making 3.6 the minimally required version, but at least for LLDB we don't have that requirement and we have documentation mentioning Python 3.5.

For LLVM: https://llvm.org/docs/GettingStarted.html#software

This is where I saw it. I got bit by @yln's cleanup patch. I can reduce the minimum version for LLDB if that's the officially required version, but llvm at the very least advertises 3.6

That page says "known to work" which is not exactly the same a minimally supported. In a unified build there's also no way to say LLVM requires Python 3.6 but LLDB only requires Python 3.5 because we use the same variables and we need to keep that in sync as to not run the test suite under one Python version while linking against another.

I see the patch got reverted because not all the bots have Python 3.6. What's the lowest Python version that they're running? Is there a reason 3.6 should be the minimally supported version?

Again, I'm not opposed to this, but it warrants a bit more discussion.

[ctetreau]

@JDevlieghere I personally have no skin in this game. This is actually quite an obnoxious development for me since my linux machine is stuck on an old version and Python 3.5 is the newest version in the repos. I sent a message to llvm-dev so hopefully this will get hashed out there. You are right though, that the linked page does not actually say "minimum requirement". I just assumed that since code was going in unchallenged that requires it, that it must be true.

[MaskRay]

https://lists.llvm.org/pipermail/llvm-dev/2021-January/148229.html "Python version requirement" for the version discussion.

[JDevlieghere]

Thanks for brining this up on the mailing list, I think that's a good place to discuss!

[yln]

My apologies for misrepresenting the meaning of the docs and stirring up this confusion. I did not read the docs carefully enough.

Replying on the mailing list ...

clang/CMakeLists.txt

1

Note that we already have a precedent for requiring the stated version with CMake.

[ctetreau]

I plan to revive this patch. I will try to rig it to only require 3.6 or above is testing is enabled using LLVM_INCLUDE_TESTS. Hopefully if any build bots haven't been fixed by now, they will not be setting this variable to ON (since they clearly aren't running tests)

[ctetreau]

Only require if adding test suite targets

[ctetreau]

Ping

[serge-sans-paille]

would it make sense to syndicate the minimal version in a single variable, say LLVM_MINIMAL_PYTHON_VERSION, and use it in several place instead of hard-coding the value across multiple files?

[ctetreau]

set a var for the minimum bound

[ctetreau]

In D95635#2619804, @serge-sans-paille wrote:

would it make sense to syndicate the minimal version in a single variable, say LLVM_MINIMAL_PYTHON_VERSION, and use it in several place instead of hard-coding the value across multiple files?

Done. That lets us just do the branch in one place as well.

[serge-sans-paille]

LGTM, thanks for doing this!

[gargaroff]

This still breaks for us. The find_package(Python3 ...) from Tooling/CMakeLists.txt does not look for the minimum version and overrides the version that was already found through llvm/CMakeLists.txt. I opened D99715 to fix this and added the original reviewers of this patch.

clang/CMakeLists.txt

 cmake_minimum_required(VERSION 3.13.4)
 
 # If we are not building as a part of LLVM, build Clang as an
 # standalone project, using LLVM as an external library:
 if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
   project(Clang)
 
   set(CMAKE_CXX_STANDARD 14 CACHE STRING "C++ standard to conform to")
   set(CMAKE_CXX_STANDARD_REQUIRED YES)
if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
   include_directories("${LLVM_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
   link_directories("${LLVM_LIBRARY_DIR}")
 
   set( CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin )
   set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )
   set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )
 
   if(LLVM_INCLUDE_TESTS)
-    find_package(Python3 REQUIRED COMPONENTS Interpreter)
+    find_package(Python3 3.6 REQUIRED COMPONENTS Interpreter)
 
     # Check prebuilt llvm/utils.
     if(EXISTS ${LLVM_TOOLS_BINARY_DIR}/FileCheck${CMAKE_EXECUTABLE_SUFFIX}
         AND EXISTS ${LLVM_TOOLS_BINARY_DIR}/count${CMAKE_EXECUTABLE_SUFFIX}
         AND EXISTS ${LLVM_TOOLS_BINARY_DIR}/not${CMAKE_EXECUTABLE_SUFFIX})
       set(LLVM_UTILS_PROVIDED ON)
     endif()
 

lld/CMakeLists.txt

if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
 
   include(AddLLVM)
   include(TableGen)
   include(HandleLLVMOptions)
   include(CheckAtomic)
 
   if(LLVM_INCLUDE_TESTS)
-    find_package(Python3 REQUIRED COMPONENTS Interpreter)
+    find_package(Python3 3.6 REQUIRED COMPONENTS Interpreter)
 
     # Check prebuilt llvm/utils.
     if(EXISTS ${LLVM_TOOLS_BINARY_DIR}/FileCheck${CMAKE_EXECUTABLE_SUFFIX}
         AND EXISTS ${LLVM_TOOLS_BINARY_DIR}/not${CMAKE_EXECUTABLE_SUFFIX})
       set(LLVM_UTILS_PROVIDED ON)
     endif()
 
     if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py)

llvm/CMakeLists.txt

 endif()
 option(LLVM_ENABLE_PLUGINS "Enable plugin support" ${LLVM_ENABLE_PLUGINS_default})
 
 set(ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER FALSE CACHE BOOL
   "Enable the experimental new pass manager by default.")
 
 include(HandleLLVMOptions)
 
+if (LLVM_INCLUDE_TESTS)
+  # Test suite uses features of 3.6
+  find_package(Python3 3.6 REQUIRED COMPONENTS Interpreter)
+else()
   find_package(Python3 REQUIRED COMPONENTS Interpreter)
+endif()
 
 ######
 
 # Configure all of the various header file fragments LLVM uses which depend on
 # configuration variables.
 set(LLVM_ENUM_TARGETS "")
 set(LLVM_ENUM_ASM_PRINTERS "")
 set(LLVM_ENUM_ASM_PARSERS "")

mlir/CMakeLists.txt

 
 set(MLIR_BINDINGS_PYTHON_ENABLED 0 CACHE BOOL
        "Enables building of Python bindings.")
 set(MLIR_PYTHON_BINDINGS_VERSION_LOCKED 1 CACHE BOOL
        "Links to specific python libraries, resolving all symbols.")
 
 if(MLIR_BINDINGS_PYTHON_ENABLED)
   include(MLIRDetectPythonEnv)
+  if (LLVM_INCLUDE_TESTS)
+    # Test suite uses features of 3.6
+    find_package(Python3 3.6 COMPONENTS Interpreter Development NumPy REQUIRED)
+  else()
     find_package(Python3 COMPONENTS Interpreter Development NumPy REQUIRED)
+  endif()
   message(STATUS "Found python include dirs: ${Python3_INCLUDE_DIRS}")
   message(STATUS "Found python libraries: ${Python3_LIBRARIES}")
   message(STATUS "Found numpy v${Python3_NumPy_VERSION}: ${Python3_NumPy_INCLUDE_DIRS}")
   mlir_detect_pybind11_install()
   find_package(pybind11 2.6 CONFIG REQUIRED)
   message(STATUS "Found pybind11 v${pybind11_VERSION}: ${pybind11_INCLUDE_DIR}")
   message(STATUS "Python prefix = '${PYTHON_MODULE_PREFIX}', "
                  "suffix = '${PYTHON_MODULE_SUFFIX}', "