Page MenuHomePhabricator

[CMake] Don't set Python_ADDITIONAL_VERSIONS
ClosedPublic

Authored by JDevlieghere on Jul 17 2019, 3:40 PM.

Details

Summary

Until recently, Python_ADDITIONAL_VERSIONS was used to limit LLVM's Python support to 2.7. Now that both LLVM and LLDB both support Python 3, there's no longer a need to put an arbitrary limit on this. However, instead of removing the variable, D64443 expanded the list , which has the (presumably unintentional) side-effect of expression preference for Python 3. Instead, as Michal proposed in the original differential, we should just not set the list at all, and let CMake pick whatever Python interpreter you have in your path.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jul 17 2019, 3:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 3:40 PM
Herald added a subscriber: mgorny. · View Herald Transcript

Without this change, it is impossible to configure CMake with Python 2.7 on macOS, without either setting the paths manually, or completely removing any trace of Python 3 on the machine.

JDevlieghere edited the summary of this revision. (Show Details)Jul 17 2019, 3:51 PM
JDevlieghere added a reviewer: mgorny.
JDevlieghere edited the summary of this revision. (Show Details)

I'm in favor of this, but we should make sure there is documentation telling users how to explicitly choose the python version they want. I'm also curious, how does cmake determine which version to use if more than on interpreter is found?

I'm in favor of this, but we should make sure there is documentation telling users how to explicitly choose the python version they want.

Agreed, I'll update the docs.

I'm also curious, how does cmake determine which version to use if more than on interpreter is found?

If you don't specify a version (which we don't) it just picks the first one it finds (in your PATH).

Update the docs with information on the default behavior with regards to Python and how to overwrite it.

JDevlieghere set the repository for this revision to rL LLVM.Jul 17 2019, 7:36 PM
JDevlieghere added a project: Restricted Project.
mgorny accepted this revision.Jul 17 2019, 11:01 PM

Makes sense. I suppose you want to update LLDBStandalone as well. And probably Python_ADDITIONAL_VERSIONS in clang & co are outdated.

This revision is now accepted and ready to land.Jul 17 2019, 11:01 PM
JosephTremoulet added inline comments.
llvm/docs/GettingStarted.rst
601–604 ↗(On Diff #210451)

FWIW, my experience yesterday was that I needed to also set PYTHON_LIBRARY and PYTHON_INCLUDE_DIR to avoid just erroring out with mismatches (but setting all three did work, once I figured out that PYTHON_LIBRARY needs to point to a file while the other two need to point to directories). It might be worth mentioning something along the lines of "You may also need to set PYTHON_LIBRARY and PYTHON_INCLUDE_DIR"? I phrased that as a question b/c I don't know if it's something that others will run into, or if I just have something funny in my environment somehow.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked 2 inline comments as done.Jul 18 2019, 8:20 AM
JDevlieghere added inline comments.
llvm/docs/GettingStarted.rst
601–604 ↗(On Diff #210451)

This shouldn't be necessary anymore after this change!

mgorny added inline comments.Jul 18 2019, 8:43 AM
llvm/docs/GettingStarted.rst
601–604 ↗(On Diff #210451)

Actually, I think it will be necessary if you change PYTHON_EXECUTABLE while having the other two cached. However, I'd rather fix the code to do a second lookup than expect users to override everything. However, I still need to think how we could achieve that.

Hmm, maybe instead of erroring out immediately we could clean cache vars and retry find_package()?

Preferring Python 3, if present, was actually intentional on my behalf on the basis that we should use the newer Python if we can. But I did not realize this would break MacOS :( (and, it sounds like, any system where you have a python 3 binary but not devel package?)

I can confirm that -DPYTHON_EXECUTABLE=/usr/bin/python3 does work (I only tried with a deleted CMakeCache.txt). It is unfortunate that on most Linux systems we'd still default to Python 2

Should this be merged to LLVM 9?

somehow when doing stage2 build, it is stubborn and does not respect any of PYTHON_EXECUTABLE. PYTHON_LIBRARY or PYTHON_INCLUDE_DIR that were passed with -D on cmake invocation even after they are added to -DDCLANG_BOOTSTRAP_PASSTHROUGH list. llvm finds its own python during stage2. in stage1 it respects the above settings.