-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
Fix Pex to handle venvs with multiple site-packages dirs. #2383
Conversation
return pex | ||
|
||
|
||
REQUESTS_CMD = [ |
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 REQUESTS_CMD
is the only real change here in tests/integration/test_excludes.py
. I also belatedly ran dos2unix
to fix line endings which makes the GitHub diff look worse than it is.
pex/wheel.py
Outdated
""" | ||
|
||
class LoadError(Exception): | ||
"""Indicates an error loading WHEEL metadata.""" |
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 took some focus to distinguish pex.pep_376.LoadError
, pex.pep_427.WheelLoadError
and this new WHEEL.LoadError
, especially when using two of them in the same file (as in pep_376.py, pep_427.py and installer.py).
AFAICT pex.pep_376.LoadError
relates to loading the layout file for an InstalledWheel, which admittedly is slightly different, but the other two seem to mean the same thing (error loading the WHEEL metadata)?
So WDYT about consolidating at least the last two, and possibly distinguishing the other via naming, for example:
pex.pep_376.LoadError
-> InstalledWheelLoadError
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 went with InstalledWheel.LoadError
for the one and moved pex.pep_427.Wheel
to pex.wheel.Wheel
and consolidated with pex.wheel.WheelMetadataLoadError
now that Wheel
and WHEEL
are in the same module.
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.
Thanks! That structure is much more clear.
7ec6b11
to
b4c326d
Compare
Previously the pex venv code would blow up when it encountered a venv with multiple distinct site-packages directories. Although this was safe, it was unhelpful for virtualenvs created by modern Virtualenv on RedHat-based distributions where there is a seperation between purelib (lib/pythonX.Y/site-packages) and platlib (lib64/pythonX.Y/site-packages). Interestingly this was not a problem for Python stdlib `-mvenv` virtualenvs since those create the `lib64/` platlib directory as a symlink to the `lib/` purelib directory. It's unclear who is constructing venvs correctly here, but given the Virtualenv layout, its definitely correct to honor the purelib / platlib distinction when installing wheels in venvs. Pex now does this and tests are added to confirm. Also fix up various existing tests that failed on ARM machines where this problem was discovered.
The 1.22.0 release breaks us on multiple fronts: + Console scripts are no longer generated. + readme is slurped into pyproject.toml and fouls metadata expansion. This is to be sorted out later.
b4c326d
to
f003f0e
Compare
Previously the Pex venv code would blow up when it encountered a venv
with multiple distinct site-packages directories. Although this was
safe, it was unhelpful for virtualenvs created by modern Virtualenv on
Red Hat based distributions where there is a separation between
purelib (
lib/pythonX.Y/site-packages
) andplatlib (
lib64/pythonX.Y/site-packages
). Interestingly this was not aproblem for Python stdlib
-mvenv
virtualenvs since those create thelib64/
platlib directory as a symlink to thelib/
purelibdirectory. It's unclear who is constructing venvs correctly here, but
given the Virtualenv layout, its definitely correct to honor the
purelib / platlib distinction when installing wheels in venvs. Pex now
does this and tests are added to confirm.
Also fix up various existing tests that failed on ARM machines where
this problem was discovered.