-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix isolated builds when enum34 is installed #8217
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
Conversation
With the prior code, a python library installed into the same directory as pip will be promoted to be found before other libraries of the same name when an isolated build is performed. This is because pip/__main__.py adds the directory holding pip to the front of sys.path where both the pip library and the other libraries will be found before the other libraries. This is especially problematic for libraries which shadow the names of stdlib libraries. The enum34 library, for instance, will shadow the stdlib enum library. When pip runs an isolated build and the enum34 library is installed, enum.py from the enum34 library will be found and imported instead of enum.py from the stdlib. That, in turn, breaks the stdlib re module which then breaks pip. This commit fixes that by changing build_env to invoke pip a different way (by calling python on pip/__main__.py instead of calling python on pip/). Using __main__.py changes what python sets the value of __package__ to which, in turn, makes it so the code in __main__.py does not add the directory holding pip to sys.path. Fixes pypa#8214
cc18fc2
to
cf08eb3
Compare
I've been thinking about this all week and I'm pretty sure that there is a drawback to this one. I think it break the guarantee that the pip library that is imported when pip is re-invoked is the same as the pip library which was originally run. The reason is that python is probably going to treat |
Given that the issue is with the internal code that invokes pip to build the isolated environment, why can't we use pip internals here, and do something like
There's no need that I can see to mess with pip's command line startup code at all if we do that. Please do not just put together yet another PR implementing this idea. Instead, can you do as I suggested and start with a test that demonstrates the issue, which can be added to one of the existing PRs, which we can then iterate on to decide on the best implementation. |
Given that the issue is with the internal code that invokes pip to build
the isolated environment, why can't we use pip internals here, and do
something like
<sys.executable> -c "sys.path.insert(0, <what we want>); from pip import main; main(...)"
This might be a good idea as it makes the two cases of sys.path munging
separate. `__main__.py` will still handle path munging for running
directly from a wheel and this code can change the path so build isolation
uses the same library as the parent pip process.
However, whether it addresses the bug or not depends on what the values
passed to sys.path.insert are. If we pass `(0, pip_location)` then we
haven't solved the problem; only shifted it around into a different
location.
You still need something like either uranusjr or felixfontein's suggestion
of detecting which paths are safe to insert our path in front of and which
you must put the path after or you'd have to construct an artifact (real
directory or zip/wheel) which only contains pip so that when you add it to
the front of sys.path you know the only libraries it overrides are the ones
you intend it to.
|
Closing due to lack of movement here. Please feel free to file a new PR if you want to get the ball rolling again! :) |
No problem. I open prs because it's fine easier for me to see pros and cons of different ways to implement something with actually working code rather than abstract discussion. I can open another pr if the score of what pip needs to actually care about is determined in #8214 |
With the prior code, a python library installed into the same directory
as pip will be promoted to be found before other libraries of the same
name when an isolated build is performed. This is because
pip/__main__.py
adds the directory holding pip to the front of sys.pathwhere both the pip library and the other libraries will be found before
the other libraries.
This is especially problematic for libraries which shadow the names of
stdlib libraries. The enum34 library, for instance, will shadow the
stdlib enum library. When pip runs an isolated build and the enum34
library is installed, enum.py from the enum34 library will be found and
imported instead of enum.py from the stdlib. That, in turn, breaks the
stdlib re module which then breaks pip.
This commit fixes that by changing build_env to invoke pip a different
way (by calling python on
pip/__main__.py
instead of calling python onpip/). Using
__main__.py
changes what python sets the value of__package__
to which, in turn, makes it so the code in__main__.py
does not add the directory holding pip to sys.path.
Fixes #8214