Stop explicitly depending on python 2 #66605
Conversation
GuillaumeGomez
force-pushed
the
drop-python2
branch
from
83fe57f to
49c58f2
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
High-level comment after skimming parts of the diff: if possible we should be more specific than "Python 3", since 3.0 and other very old 3.x releases will likely not work (for long). These scripts will only be tested with the versions that |
|
python3 should be fine in most (if not all) cases no? By installing python3, we'll generally get the 3.5 version. But even if we get a lower version, I don't know a case where it wouldn't work. Do you have any? (very interested in such cases!) |
|
One example that comes to mind is that And of course there's also a large number of new features that were added over the years since 3.0 which only work after whatever version they were introduced, so that's a risk for code written in the future. As concrete examples, 3.6 added f-strings and underscores in numeric literals, both of which are really convenient. When I write Python code (which admittedly isn't that often these days) I use them rather often, and I can't imagine I'm the only one. |
|
Fwiw the oldest 3.x Python version officially supported is 3.5, which will reach end-of-life on 2020-09-13. However there are still distros out there that use Python 3.4 (Debian Jessie for instance, which will reach end of life on 2020-06-30). Anyway the scripts are currently written to be compatible with python 2.7 so they are most likely compatible with any 3.x version. |
|
Seems like the tests passed. Should we give it a try? |
|
I find this PR somewhat unexpected. I would not expect us to drop compatibility with Python 2 for some time still, regardless of whether it is officially supported or not. For one thing, it's still supported until January 1st (I believe), so this PR is at least premature from that perspective. I also don't personally expect that we should be so eager to drop support just because it's not officially maintained -- if we can be compatible, why not be? To my knowledge, it's not historically been a burden on us to maintain that compatibility, so it's not clear why we should suddenly stop doing so. I am on board with us trying to change the default to python 3, though, and making sure that works on CI and such. I am not sure if LLVM's use of python has sufficiently migrated for that to be possible, but if yes, then we can switch our defaults to I think. |
|
What about symlinking |
|
@Mark-Simulacrum just one piece of anecdotal evidence, but without this PR I was still able to successfully complete |
|
IMO, we do not need to take any action with regards to changing scripts to remove python 2 compatibility. We should likely switch the default to python 3 as soon as is feasible (@smmalis37's results sound like we can do it now), but we should not remove compatibility with python 2 intentionally. I don't think we need to test that compatibility -- but for at least until all known distro LTS releases, etc. have moved off python 2 by default we shouldn't just drop support for it entirely. |
|
I'd debate over the fact that relying on an unmaintained/unsupported technology is never a good idea, but I can rollback the few python2 support removals. |
|
It won't be supported by python.org, but in those LTS distros it will still be maintained and supported by their vendor. |
|
Added back the python2 compatibility code. |
There was a problem hiding this comment.
I'd like to hear @alexcrichton's thoughts here as well, and will try to bring this up at infra meeting tomorrow (nominating).
src/etc/htmldocck.py
Outdated
|
|
||
| def handle_charref(self, name): | ||
| code = int(name[1:], 16) if name.startswith(('x', 'X')) else int(name, 10) | ||
| self.__builder.data(unichr(code)) | ||
| self.__builder.data(chr(code)) |
There was a problem hiding this comment.
AFAICT, this drops compatibility with python 2 here -- is there a way to avoid that?
configure
Outdated
| try python2 "$@" | ||
| exec python $script "$@" | ||
| try python3 "$@" | ||
| exec python3 $script "$@" |
There was a problem hiding this comment.
It might make sense to fallback on python 2 here to avoid breaking compatibility, i.e., we just try python 3 first.
src/bootstrap/sanity.rs
Outdated
| @@ -120,10 +120,9 @@ pub fn check(build: &mut Build) { | |||
| } | |||
|
|
|||
| build.config.python = build.config.python.take().map(|p| cmd_finder.must_have(p)) | |||
| .or_else(|| cmd_finder.maybe_have("python2.7")) | |||
| .or_else(|| cmd_finder.maybe_have("python2")) | |||
| .or_else(|| cmd_finder.maybe_have("python3")) | |||
There was a problem hiding this comment.
Same here, I'd prefer to avoid breaking compat with python 2, we should keep it as a fallback everywhere (in bootstrap/configure/etc, our CI scripts can assume it's installed).
Mark-Simulacrum
added
I-nominated
T-infra
GuillaumeGomez
force-pushed
the
drop-python2
branch
from
2a4b551 to
a9f3f54
Compare
|
Updated! |
|
My personal opinion about python versions and such basically boils down to: (ranked in order of importance):
Other than that I do not have a preference as to what is supported nor what all the nitty-gritty is in the various locations. I do not also understand all the intricacies of why we might not support python 3 today. |
|
Unrelated to python... shouldn't exec python $script "$@"to: exec python "$script" "$@"to work in paths that contain spaces? Also, wouldn't the remaining An alternative idea: since it looks like the Python interpreter path is determined in several places, maybe this should be refactored into an environment variable? Users can set the variable, but |
|
☔ The latest upstream changes (presumably #66739) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
src/etc/htmldocck.py
Outdated
| def py2_chr(x): | ||
| if sys.version_info[0] < 3: | ||
| return unichr(x) | ||
| return chr(x) |
There was a problem hiding this comment.
There is already compatibility code for this below. There is no need to modify this file at all.
| @@ -1,2 +1,2 @@ | |||
| all: | |||
| python2.7 test.py | |||
| python3 test.py | |||
There was a problem hiding this comment.
| python3 test.py | |
| '$(PYTHON)' test.py |
src/etc/test-float-parse/runtests.py
Outdated
| @@ -90,6 +90,7 @@ | |||
| import threading | |||
| import ctypes | |||
| import binascii | |||
| import queue as Queue | |||
There was a problem hiding this comment.
| import queue as Queue |
This is handled below.
x.py
Outdated
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env python | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
These shebangs should all remain #!/usr/bin/env python because the scripts support both Python 2 and 3.
There was a problem hiding this comment.
For this file I agree, for the others not really. The default should be python3 if started as an executable. None of the other scripts are starting like this.
|
Discussed this at the infra meeting, we're fine with switching everything to Python 3 as long as we don't break Python 2, which seems the direction of this PR. |
GuillaumeGomez
force-pushed
the
drop-python2
branch
from
a9f3f54 to
91742fa
Compare
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
Mark-Simulacrum
force-pushed
the
drop-python2
branch
3 times, most recently
from
685fc79 to
bf35d8f
Compare
|
@bors r+ Fixed the conversion to a str, hopefully (it works locally). Also added some debug logging just in case. |
|
📌 Commit bf35d8f3559c6376caf4542236dddfb2388bd650 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit bf35d8f3559c6376caf4542236dddfb2388bd650 with merge 264d449596c4f21e2f9c55301bf64ba712420d3c... |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
Mark-Simulacrum
force-pushed
the
drop-python2
branch
from
bf35d8f to
38eb369
Compare
|
@bors r+ Hopefully fixed that failure... |
|
📌 Commit 38eb369 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
|
👍 |
This PR revises our previous policy of officially only supporting and testing with python 2 in the CI environment to instead test with python 3. It also changes the defaults to python 3 in our various scripts (usually, by way of
pythonrather thanpython3to preserve compatibility with systems that do not have a python 3 available).The effect of this is that we expect all new patches to support python 3 (and will test as such). We explicitly also expect that patches support python 2.7 as well -- and test as such, though only on one builder. This is intended as a temporary, though likely long-lived, measure to preserve compatibility while looking towards the future which is likely to be a python 3 only world. We do not at this point set a timeline for when we'll drop support for python 2.7; it's plausible that this is months or years into the future, depending on how quickly the ecosystem drops support and how painful it is for us to maintain that support over time.
Closes #65063 (as far as I can tell; please file explicit and separate issues or PRs if not).