Page MenuHomePhabricator

[CMake] Require python 3.6 if enabling LLVM test targets
ClosedPublic

Authored by ctetreau on Jan 28 2021, 11:02 AM.

Details

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.

Diff Detail

Event Timeline

ctetreau created this revision.Jan 28 2021, 11:02 AM
ctetreau requested review of this revision.Jan 28 2021, 11:02 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJan 28 2021, 11:02 AM
yln added a comment.Jan 28 2021, 11:09 AM

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

yln accepted this revision.Jan 28 2021, 11:09 AM
This revision is now accepted and ready to land.Jan 28 2021, 11:09 AM
JDevlieghere added a comment.EditedJan 28 2021, 11:19 AM

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 added a comment.Jan 28 2021, 11:41 AM

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

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:

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

This revision was landed with ongoing or failed builds.Jan 29 2021, 11:47 AM
This revision was automatically updated to reflect the committed changes.
JDevlieghere reopened this revision.Jan 29 2021, 12:10 PM

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:

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.

This revision is now accepted and ready to land.Jan 29 2021, 12:10 PM

@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.

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

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

yln added a comment.Jan 29 2021, 4:18 PM

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 planned changes to this revision.Mar 11 2021, 6:55 AM

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 updated this revision to Diff 329965.Mar 11 2021, 7:44 AM

Only require if adding test suite targets

This revision is now accepted and ready to land.Mar 11 2021, 7:44 AM
ctetreau requested review of this revision.Mar 11 2021, 7:45 AM
ctetreau retitled this revision from [CMake] Actually require python 3.6 or greater to [CMake] Require python 3.6 if enabling LLVM test targets.
ctetreau edited the summary of this revision. (Show Details)

Ping

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 updated this revision to Diff 330078.Mar 11 2021, 3:03 PM

set a var for the minimum bound

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.

LGTM, thanks for doing this!

This revision is now accepted and ready to land.Mar 11 2021, 10:17 PM
This revision was landed with ongoing or failed builds.Mar 15 2021, 9:51 AM
This revision was automatically updated to reflect the committed changes.

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.