Make Python version setting actually effective

[cbiesinger]

Summary

This needs to be outside the if to actually work.

Also, this adjusts the list of versions to match LLVM.

[clayborg]

Probably best to have someone else review cmake changes, as I am not very familiar with all the intricacies of all the settings. I will add some people with more cmake experience to the reviewers. Jonas and Stefan, feel free to add more cmake experts here.

[sgraenitz]

I think it's more a question of our Python support and its tolerance. Are you sure about 2.6 and 2.5? LLVM's default sounds more comfortable:

set(Python_ADDITIONAL_VERSIONS 3.7 3.6 3.5 2.7)

Do we need any more then that?

[JDevlieghere]

Stefan is right, no point in supporting anything less than LLVM.

Can you motivate why we need this in the first place? Don't we inherit the values form LLVM in the non-standalone scenario? Can we remove the same line in LLDBStandalone.cmake if we do it here?

[cbiesinger]

Match LLVM and remove the line from LLDBStandalone

[cbiesinger]

Yes, this does not seem to be used when building as part of LLVM, only standalone.

The motivation for this change is that without it, the versions line seems to be ignored and so CMake will pick Python 2.

I followed your suggestion to match the versions in LLVM and also removed the line from LLDBStandalone.cmake, which seems to work for still making it find Python 3. But I don't know why this code is duplicated between the two files.

[sgraenitz]

Hi Christian, thanks for following up on this.

I think it should be the other way around. We don't need to set Python_ADDITIONAL_VERSIONS in in-tree builds, because then LLVM defines it already. We do need it in standalone builds, because then LLVM is not part of our configuration (its build-tree lives in a different location), so we must define it ourselves. The latter is exactly the purpose of LLDBStandalone.cmake. It is included in the very beginning to find LLVM&co and fill in all the definitions that an in-tree build would usually have at this point (some are imported via LLVMConfig.cmake). Ideally all subsequent code, like LLDBConfig.cmake, doesn't need to care about it anymore. In some cases, however, we still want to check later on. That's what LLDB_BUILT_STANDALONE is used for.

I guess the reason why it's not defined in your case is that it's behind the if (PYTHON_EXECUTABLE STREQUAL "") condition.

[cbiesinger]

Move the version list outside of the strequal "" check

[cbiesinger]

How about this, then? It does seem to work.

[JDevlieghere]

This looks reasonable to me.

[cbiesinger]

Thanks! Could you/someone land this? I'm not a committer.

cmake/modules/LLDBStandalone.cmake

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   # CMake modules to be in that directory as well.
   list(APPEND CMAKE_MODULE_PATH "${LLVM_DIR}")
   include(AddLLVM)
   include(TableGen)
   include(HandleLLVMOptions)
   include(CheckAtomic)
   include(LLVMDistributionSupport)
 
+  set(Python_ADDITIONAL_VERSIONS 3.7 3.6 3.5 2.7)
   if (PYTHON_EXECUTABLE STREQUAL "")
-    set(Python_ADDITIONAL_VERSIONS 3.7 3.6 3.5 3.4 3.3 3.2 3.1 3.0 2.7 2.6 2.5)
     include(FindPythonInterp)
     if( NOT PYTHONINTERP_FOUND )
       message(FATAL_ERROR
               "Unable to find Python interpreter, required for builds and testing.
                Please install Python or specify the PYTHON_EXECUTABLE CMake variable.")
     endif()
   else()
     message(STATUS "Found PythonInterp: ${PYTHON_EXECUTABLE}")