-
-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Python 3.12. #2172
Conversation
The crux here is supporting a version of Pip that works in 3.12. There is no such released version yet; so this change adds an unreleased Pip version but goes to some length to hide this version from users and make it only activatable by those in the know / CI. What follows is fixing or adjusting many tests. The result is Pex known to work with Python 3.12 ahead of its release by several months and the spectre of Pex 3 / a Pex branch split, etc., dispelled. It turns out Pex can still ship supporting Python 2.7, 3.5, etc. along side supprting 3.12. The main trick here is to use `python3.12 -mvenv` to spirit up a bootstrap Pip that works at least enough to install the unreleased Pip that truly works with Python 3.12. Previously all Pip version bootstrapping was handled exclusively by the vendored Pip.
@@ -82,19 +88,61 @@ def _vendored_installation(interpreter=None): | |||
) | |||
|
|||
|
|||
def _bootstrap_pip( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new trick that allows a Pip that supports Python 3.12 to be bootstrapped without resorting to a resolve using the vendored Pip (the technique on line 142 below). Not much of a trick, pretty obvious in hindsight.
return self._latest | ||
|
||
|
||
class DefaultPipVersion(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously VENDORED was the global default but that no longer works. This allows dynamically choosing the correct default given the current ambient interpreter: VENDORED for everything <3.12, unreleased Pip (for now) for everything newer.
This is large, but the bulk (~400 of the 700 delta lines) is in tests that had to change to accommodate the new Python 3.12 test shards. The production changes are concentrated in PipVersion and installation with small fallout in many other files surrounding the default pip version passing. Thanks in advance for your time looking at this. |
- os: macos-12 | ||
python-version: [ 3, 12, "0-beta.3" ] | ||
pip-version: 23_2 | ||
tox-env-python: python3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out tox doesn't work (or I couldn't get it to work) with 3.12; thus this complication. More generally, trying to support a new Python version, especially when it yanks core stdlib like distutils, is hard if you're early. Eventually tox will support 3.12 and this complication will be removed.
requires_python=">=3.7", | ||
hidden=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some complication was added here to be able to use a Pip version I want to yank, but still uphold the guaranty of not breaking users. This flag and the attendant complication above all work to make it so a release of Pex between now and the moment it can be upgraded to a Pip that supports Python 3.12 won't expose a version of Pip on the CLI that will later get yanked.
@@ -651,6 +655,20 @@ def _declare_namespace_packages(cls, resolved_dists): | |||
) | |||
for index, (dist, ns_packages) in enumerate(namespace_packages_by_dist.items()) | |||
) | |||
if not pkg_resources: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change and many test fixes revolve around the full switch to importlib.resources
in Python 3.12. The setuptools maintainers are killing pkg_resources
as of Python 3.12 and pkg_resources
, like Pex, relies on import hooks to enable vendored code it uses. The setuptools team did not update those hooks to work in a Python 3.12 world (Pex had to in #2170); so even when pkg_resources
is present, it fails to import under 3.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Thank you, John.
I'm comfortable with the PR size. But fwiw, if you want to make it smaller so we can give stronger reviews, it could be worth breaking out the PipVersion.DEFAULT
change in a precursor PR to remove a lot of the noise here.
if path == strip_prefix: | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the context for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch actually. I missed an item on my cleanup checklist. This gets rid of a ..
entry in the --layout packed
zips currently created by Pex. In Python 3.12 zipfile, these are now illegal. I need to change the cache format so that new Pex under 3.12 does not see the old zips since it will blow up if it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I needed to refresh myself. Without this (the old behavior) you get packed wheel zips like:
Archive: /home/jsirois/.pex/installed_wheel_zips/858d7ad606d58d8b13b4ac842dd378ff37e94387abc4b79e281576af39c64a49/py_spy-0.3.8-py2.py3-none-manylinux_2_5_x86_64.manylinux1_x86_64.whl
Zip file size: 3126344 bytes, number of entries: 10
drwxr-xr-x 2.0 unx 0 b- defN 80-Jan-01 00:00 ../
-rw-r--r-- 2.0 unx 75 b- defN 80-Jan-01 00:00 .layout.json
drwxr-xr-x 2.0 unx 0 b- defN 80-Jan-01 00:00 .prefix/
drwxr-xr-x 2.0 unx 0 b- defN 80-Jan-01 00:00 .prefix/bin/
-rwxr-xr-x 2.0 unx 9838248 b- defN 80-Jan-01 00:00 .prefix/bin/py-spy
drwxr-xr-x 2.0 unx 0 b- defN 80-Jan-01 00:00 py_spy-0.3.8.dist-info/
-rw-r--r-- 2.0 unx 4 b- defN 80-Jan-01 00:00 py_spy-0.3.8.dist-info/INSTALLER
-rw-r--r-- 2.0 unx 16821 b- defN 80-Jan-01 00:00 py_spy-0.3.8.dist-info/METADATA
-rw-r--r-- 2.0 unx 0 b- defN 80-Jan-01 00:00 py_spy-0.3.8.dist-info/REQUESTED
-rw-r--r-- 2.0 unx 123 b- defN 80-Jan-01 00:00 py_spy-0.3.8.dist-info/WHEEL
10 files, 9855271 bytes uncompressed, 3125164 bytes compressed: 68.3%
The issue there is the 1st ../
entry. When Python 3.12 zipfile
sees that, it says:
if not arcname:
> raise ValueError("Empty filename.")
E ValueError: Empty filename.
../../../.pyenv/versions/3.12.0b3/lib/python3.12/zipfile/__init__.py:1761: ValueError
More directly:
- 3.11
$ mkdir /tmp/py311-unzip && python3.11 -mzipfile -e /home/jsirois/.pex/installed_wheel_zips/858d7ad606d58d8b13b4ac842dd378ff37e94387abc4b79e281576af39c64a49/py_spy-0.3.8-py2.py3-none-manylinux_2_5_x86_64.manylinux1_x86_64.whl /tmp/py311-unzip
- 3.12
$ mkdir /tmp/py312-unzip && python3.12 -mzipfile -e /home/jsirois/.pex/installed_wheel_zips/858d7ad606d58d8b13b4ac842dd378ff37e94387abc4b79e281576af39c64a49/py_spy-0.3.8-py2.py3-none-manylinux_2_5_x86_64.manylinux1_x86_64.whl /tmp/py312-unzip <frozen runpy>:128: RuntimeWarning: 'zipfile.__main__' found in sys.modules after import of package 'zipfile', but prior to execution of 'zipfile.__main__'; this may result in unpredictable behaviour Traceback (most recent call last): File "<frozen runpy>", line 198, in _run_module_as_main File "<frozen runpy>", line 88, in _run_code File "/home/jsirois/.pyenv/versions/3.12.0b3/lib/python3.12/zipfile/__main__.py", line 77, in <module> main() File "/home/jsirois/.pyenv/versions/3.12.0b3/lib/python3.12/zipfile/__main__.py", line 44, in main zf.extractall(curdir) File "/home/jsirois/.pyenv/versions/3.12.0b3/lib/python3.12/zipfile/__init__.py", line 1720, in extractall self._extract_member(zipinfo, path, pwd) File "/home/jsirois/.pyenv/versions/3.12.0b3/lib/python3.12/zipfile/__init__.py", line 1761, in _extract_member raise ValueError("Empty filename.") ValueError: Empty filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to whip up a unit test for the new pex/common.py guard / behavior on top of the cache re-structure avoidance fix.
pex/environment.py
Outdated
pex_warnings.warn( | ||
"The legacy `pkg_resources` package cannot be imported by the " | ||
"{interpreter} {version} interpreter at {path}.\n" | ||
"These distributions will fail to work properly:\n{dists}".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an eager error instead? The warning below makes sense—we can salvage the situation but it's not kosher. Here, this will eventually result in an error for the user, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe worth mentioning this line:
These distributions should fix their
install_requires
to include `setuptools
Otherwise the user doesn't know a possible path forward for this issue, unless they already are familiar with this niche problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An eager error would be bad: we tank the ship for an import that may never happen. When a tool tries to do too much, there is a very maddening side to it and its this.
Your second suggestion is reasonable though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually its not really reasonable, since that is not a path forward. The distributions simply won't work at all under 3.12 (unless non-ns-package parts are imported per the point above, in which case moot).
That does not cleanly break out, it needs a new PipVersion to make any sense in isolation and bringing that in brings in the rest. |
Move from `installed_wheel_zips` to `packed_wheels` for the packed layout wheel zip cache. This avoids reading old bad `../` zip entries under Python 3.12+ while allowing co-existence.
I experimented with the latest Pip main (a few new commits on top of the version used here for Python 3.12 support / testing) and there will need to be more changes to support Pip 23.2 - not Python 3.12 related though. I'll proceed to land this on green as a result since more work is ahead. |
The crux here is supporting a version of Pip that works in 3.12. There
is no such released version yet; so this change adds an unreleased
Pip version but goes to some length to hide this version from users
and make it only activatable by those in the know / CI. What follows
is fixing or adjusting many tests. The result is Pex known to work with
Python 3.12 ahead of its release by several months and the spectre of
Pex 3 / a Pex branch split, etc., being forced by Python 3.12 support
dispelled.
It turns out Pex can still ship supporting Python 2.7, 3.5, etc.
along side supporting 3.12. The main trick here is to use
python3.12 -mvenv
to spirit up a bootstrap Pip that works at leastenough to install the unreleased Pip that truly works with Python 3.12.
Previously all Pip version bootstrapping was handled exclusively by the
vendored Pip.