Page MenuHomePhabricator

Make mangled_names.test and update_cc_test_checks.py work with Python 2.
ClosedPublic

Authored by thakis on Dec 16 2019, 12:48 PM.

Details

Summary

LLVM hasn't dropped Python 2 support yet, and this makes it easier to check that the test passes.

Diff Detail

Event Timeline

thakis created this revision.Dec 16 2019, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2019, 12:48 PM

I think it might be better to just always use sys.executable and skip the tests if the version is less than (3, 5).

I'm not sure it makes sense to make a developer only script compatible with a python version that will be eol very soon.

Do you have a platform where installing Python 3 instead of Python 2 will be such an insurmountable task? Python 2.7 will retire in 15 days.

Do you have a platform where installing Python 3 instead of Python 2 will be such an insurmountable task? Python 2.7 will retire in 15 days.

If llvm updates its project policy to say that only py 3.x is supported (which x?), I'm fine with that. For now, we still support 2.7, so this script too must stick to that.

The "will retire in 15 days" is fairly meaningless, it's not like it'll just disappear then.

Do you have a platform where installing Python 3 instead of Python 2 will be such an insurmountable task? Python 2.7 will retire in 15 days.

If llvm updates its project policy to say that only py 3.x is supported (which x?), I'm fine with that. For now, we still support 2.7, so this script too must stick to that.

The "will retire in 15 days" is fairly meaningless, it's not like it'll just disappear then.

My understanding is that this does not interact with the infrastructure. python3 should be pervasive enough that we probably don't have to worry that not enough build bots run REQUIRES: python3. The infrastructure itself may require Python 2.7, but that is different.

About whether we should add Python 2 compatibility to new features: http://lists.llvm.org/pipermail/llvm-dev/2019-December/137634.html

Per thread, it sounds like you're ok with merging this?

arichardson accepted this revision.Dec 18 2019, 12:38 PM

Looks fine to me if @MaskRay is happy with it.

llvm/test/tools/UpdateTestChecks/lit.local.cfg
20

Not related to this review, but while you are changing this line it might be good to replace the single-quoted %s: i.e.:

(name, '%s %s %s' % (shell_quote(config.python_executable), script_path, extra_args)))

This revision is now accepted and ready to land.Dec 18 2019, 12:38 PM
arichardson added inline comments.Dec 18 2019, 12:39 PM
llvm/test/tools/UpdateTestChecks/lit.local.cfg
13

The parameter here is unused now so can be removed.

MaskRay added inline comments.Dec 18 2019, 1:02 PM
llvm/utils/update_cc_test_checks.py
1
MaskRay added inline comments.Dec 18 2019, 1:11 PM
llvm/utils/update_cc_test_checks.py
1

Everything else looks good to me.

thakis marked an inline comment as done.Dec 21 2019, 5:18 PM
thakis added inline comments.
llvm/utils/update_cc_test_checks.py
1

This makes the script look like every other python in the script. How is that a cause for concern?

MaskRay added inline comments.Dec 21 2019, 5:27 PM
llvm/utils/update_cc_test_checks.py
1

Many Python scripts in llvm/utils use #!/usr/bin/env python3 as the shebang now.

Looks like it's exactly 10, out of 140, none of them covered by tests.

Since I debugged this failure, I'll go ahead and land this change.

If someone wants to do the work and pull the py3 detection out of this lit.local.cfg into an higher-level lit place and use that, that's fine with me. But I think per thread there's consensus that we shouldn't have this kind of logic on a per-tool level.

MaskRay added a subscriber: rnk.EditedDec 26 2019, 10:23 PM

Looks like it's exactly 10, out of 140, none of them covered by tests.

Since I debugged this failure, I'll go ahead and land this change.

If someone wants to do the work and pull the py3 detection out of this lit.local.cfg into an higher-level lit place and use that, that's fine with me. But I think per thread there's consensus that we shouldn't have this kind of logic on a per-tool level.

@rnk partially reverted a previous update_*_checks.py change in 287307a0c60b68099d5f9dd22ac1db2a42593533.

(FWIW, I still think changing shebang from python3 back to python does not worth the trouble.)

Looks like it's exactly 10, out of 140, none of them covered by tests.

I doubt there are 140-10 actively used #!.*python scripts. There may be lots of ancient scripts that can be removed.

rnk added a comment.Dec 27 2019, 11:41 AM

I kind of wish we'd go back to the plain "#!/usr/bin/env python` version of the shebang. On Windows, because symlinks are not commonplace (they can be enabled, but are usually not), Python packages do not have versioned python binary names (python2.7, python3, etc). There is and has always been just python.exe.

If people want to add a startup version check to the test updating scripts so that the exit with a clear error message ("use Python 3"), IMO that would be the best solution. I want to live in a world where there is only one Python version at a time.

thakis closed this revision.Jan 2 2020, 10:46 AM

I landed the bits of this that rnk didn't revert in 9d49e5c0876f7cf75ce0b5d8b3c8473300eb096a (including the py3->py change in the shebang line; sounds like that's even necessary to get the test landed and passing on Windows).

This should make it possible to attempt relanding the test.