With Enum34 installed on python3.6+, pip picks up enum34 instead of the stdlib enum package #8214
Comments
triage-new-issues
bot
added
the
S: needs triage
Because pip ships with an __main__.py file, one valid way to invoke it is: python /usr/lib/python3.6/site-packages/pip/. This results in __package__ being the empty string This, in turn, triggers the code to add the pip wheel to sys.path. Unfortunately, while this is fine to do when it's restricted to a wheel just containing pip, it isn't okay to do when it is site-packages as that will mean that other things in site-packages could end up overriding things in the stdlib (*cough*enum34*cough*). Check file extension to limit when we end up adding the extra path to sys.path. Fixes pypa#8214
Can you provide a link to the relevant line of code? I can't find what you're referring to here 🙁 Also, I just tried this and it worked fine for me: |
|
@pfmoore It's this line here: https://github.com/abadger/pip/blob/master/src/pip/_internal/build_env.py#L171 /me looks through your output to see if he can spot what is different between our two environments. |
|
|
|
Oh! I think I might know why you couldn't reproduce. Where is your initial pip located? My initial pip is also in ~/.local. That would come into play because the line in build_env.py is going to /me goes forth to verify this is the difference now that he's posted the theory here. EDITED: My language was unclear iniitally. The code in build_env re-invokes pip as |
|
Yep, that's what it was. So if you want to reproduce from where you said it worked for you, you can simply run the last pip command again. Since you just installed pip from git into the user home dir, the second time the command runs, it will use that pip and then fail. If you want to start over, here are all the steps I took with a container to reproduce (Leaving off output but adding comments) |
|
Oops, and I see why you couldn't find the line I was talking about earlier too... I posted the wrong file at the top of the issue. Sorry. I was greping the code for the command line strings to see if I could find any other occurrences that would have the same problem and I cut and pasted the wrong filename into the issue report. |
|
In python-poetry/poetry#1122 this happened with Python 3.7.3. |
|
So this is related to running pip for the isolated build environment. That makes sense, and was what I initially assumed you might be thinking about, but your reference to the wrong file put me off track. Thanks for clarifying. So I think there probably is something to address here, but I'm not sure your proposed fix is the right approach. I still think that running pip as |
|
. Yeah, that's fair. So there's several things I am thinking....
|
|
I've figured out another way to change build_env.py that looks like it works.... but I'm afraid it might be an undocumented implementation detail of the way python works so I'm not sure if it's safe to rely on. Instead of executing the directory where pip lives, execute pip's If you want to go that route, it would look something like this: diff --git a/src/pip/__main__.py b/src/pip/__main__.py
index a529758b..2e9ff7e7 100644
--- a/src/pip/__main__.py
+++ b/src/pip/__main__.py
@@ -12,16 +12,16 @@ if sys.path[0] in ('', os.getcwd()):
# If we are running from a wheel, add the wheel to sys.path
# This allows the usage python pip-*.whl/pip install pip-*.whl
+# Do not enter this codepath if __package__ is None as that
+# means that pip/__main__.py was invoked directly, which is what
+# build_env.py does (issue #8214)
if __package__ == '':
# __file__ is pip-*.whl/pip/__main__.py
# first dirname call strips of '/__main__.py', second strips off '/pip'
diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py
index b8f005f5..77e192f2 100644
--- a/src/pip/_internal/build_env.py
+++ b/src/pip/_internal/build_env.py
@@ -167,8 +167,9 @@ class BuildEnvironment(object):
prefix.setup = True
if not requirements:
return
+ command = os.path.join(os.path.dirname(pip_location), '__main__.py')
args = [
- sys.executable, os.path.dirname(pip_location), 'install',
+ sys.executable, command, 'install',
'--ignore-installed', '--no-user', '--prefix', prefix.path,
'--no-warn-script-location',
] # type: List[str] |
|
Documentation on
If you want to go this route, perhaps we should ping ncoghlan at that time to see if he considers the None/empty string split to be something that we can depend upon here. |
pradyunsg
added
state: needs discussion
triage-new-issues
bot
removed
the
S: needs triage
|
I'd prefer that we switch our BuildEnvironment logic to be not dependent on the |
|
Directly invoking |
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
|
Okay, two of the three ways I could see this working are easy to implement so I implemented them in two separate PRs. I'm not going to implement turning the pip directory into a wheel and then running that in build_env because there's probably going to be design decisions that would require some back and forth with committers in order to come up with the right way to do it and I don't have the time to commit to that. Here's my summary of the three methods:
Note that even though any one of these will fix this issue, none of them are mutually exclusive. ie: you could implement all of them if you felt that together, they made the code make more sense as a whole. |
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
|
Catching up on the comments, but I wanted to respond to this (from the other thread):
No, you misunderstood me badly. I do not think we should support Maybe you misunderstood because I noted that |
|
Thanks for responding. Unless I'm misunderstanding you now, I think I understood you just fine. Here's what I said in the portion you just quoted with some emphasis added:
which seems to match what you just said, yes?
I was asking for confirmation of that because @uranusjr was using a user invoking |
Yes it does. Sorry, this thread got massively confusing and I misread 🙁
The various misinterpretations, incorrect statements, and general confusion has rendered this discussion pretty near to impossible to follow, TBH. Some of that is my fault, for which I apologise. #8123 is entitled "fix when pip is invoked as In actual fact, the issue now appears to be something more like "Pip's machinery for creating an isolated build environment doesn't correctly ignore packages installed in the user's environment". And we seem to have jumped straight from there into debating solutions, rather than understanding what's actually going on here. For example, why is I'm going to ignore this thread for a few days, to give things a chance to settle down, and a clearer picture to emerge (in my own head, if nowhere else!) I also intend to ignore all of the proposed fixes, and focus solely on understanding what's going on with the issue when I come back to it - and frankly, I'd suggest others do the same. We can evaluate possible fixes once we better understand what we're fixing... |
|
Cool, I'm available on IRC when you are ready to talk about it from a deeper level. I can talk there and post summaries here or someone can ping me to take a look at updates here. (If I'm busy, once something goes out of my mental buffer, it can be hard to get it back in without pinging me there to get me to look at something ;-) When you get back to this, regarding |
|
I'm going to post this here, too because I can see a misunderstanding but I have no expectation that you'll read it and get back to me for a while: I changed the title on #8213. Your suggested title was slightly different than what the problem actually is. It's not that pip should ignore the user's environment when it reinvokes itself for an isolated build; it's that pip should not place any directories with libraries other than pip before the stdlib in sys.path. Here's a step through of how sys.path is being modified:
So the breakage is independent of the usage of the user's site-packages directory. The breakage can occur when pip puts any directory which contains more modules than just pip into sys.path before the stdlib entries there. The changes in any of the proposed fixes are aimed at limiting the scope of the modification to sys.path. #8213 and #8217 work by modifying sys.path in fewer cases. The fix I did not implement works by creating a different sys.path entry which would only contain pip. I'm sorry for any confusion; I think I tend to think things through in a different way than most people. I read books backwards and get the most out of movies when I've already read the plot :-) So when I lay out how something works, perhaps I lay out my thoughts and analysis in a way that is backwards for other people. |
|
(This popped up in my email, so I ended up reading it - thanks for the detailed post, but please be aware that I haven't thought it through yet. This note is mostly just something that came to mind now, and I want to record it so I don't forget. I'll follow up with a better analysis later). So this is basically related to the ages-old debate as to whether it's acceptable to "shadow" stdlib modules. It seems to me there are two main points here:
IMO, So we're left with working around the issue in pip. Putting the new entry at the end of As I say, I'm not going to think through this now, though. |
But surely (?) both of those are needed even in a general library? Protecting against fork-bombs is a general need, and a library needs to allow the user how to invoke pip.
+1 on this |
Yea, but we'd need to disentangle that logic which is what I was trying to flag. :) Also, it's only needed somewhere in the recursive chain, and pep517 doesn't come in the recursive loop. IOW, pip has these protections and since pep517 is currently just straight up using pip, pep517 doesn't need to have any protections and can "reuse" that pip does it. Keeping the expectations the same or making pep517 keep track of the recursion might be a design decision to make during this refactor. :) |
This is a holdover from long since passed Python 2.7 days. It probably should have been removed long ago. `enum34` is a backport of the stdlib enum module for Python versions < 3.4, so we no longer need it since we require 3.6. Error seen attempting to `pip install psycogreen==1.0.2` when enum34 is installed: AttributeError: module 'enum' has no attribute 'IntFlag' Relevant links: https://stackoverflow.com/q/43124775/10840 pypa/pip#8214
|
I’ve been thinking about this recently. Maybe we shouldn’t really run the pip installation to populate the isolated environment in the first place? pip can (for example) build itself into a zip somewhere, and run that instead of the installed source. This zip-building process can be done on-demand when an isolated environment is needed, and cached for the entire session to minimise performance impact (although honestly the delay is probably minor compared to the wheel-building process itself). |
The enum34 package is included in the standard library since Python 3.4, so there's no need to add it as a requirement when installing PyRDF in a Python 3.4+ environment. In fact, this requirement is messing with pip (e.g. pypa/pip#8214)
The enum34 package is included in the standard library since Python 3.4, so there's no need to add it as a requirement when installing PyRDF in a Python 3.4+ environment. In fact, this requirement is messing with pip (e.g. pypa/pip#8214)
The enum34 package is included in the standard library since Python 3.4, so there's no need to add it as a requirement when installing PyRDF in a Python 3.4+ environment. In fact, this requirement is messing with pip (e.g. pypa/pip#8214)
The enum34 package is included in the standard library since Python 3.4, so there's no need to add it as a requirement when installing PyRDF in a Python 3.4+ environment. In fact, this requirement is messing with pip (e.g. pypa/pip#8214)
|
@abadger since you’ve done the most work, and likely thought about this a lot more than anyone else, I wonder what do you feel are the caveats around this issue. There were problems raised against the two previous solutions, and maybe we’d need to rethink what we need from the logic, in order to come up with a solution to the right problem. |
|
(I can probably provide time to figure out the technical details about how we can achive what we want, but first we need to understand exactly what we want.) |
|
@uranusjr Reading your proposed fix from October, #8214 (comment) I think that would work. If I remember the problem right and understand your solution:
Your solution should fix this by isolating just the pip code which is needed into a wheel. That code will then be used in preference to any other installation of pip in You probably don't need to build pip into a wheel if it's extra steps. If you can isolate the pip code into its own directory and add that directory to |
|
I did a quick check, creating a zipapp for the pip code base takes <1s on my various machines. There is an overhead running This might be the way to go. I’ll try to find some time working on this. |
|
Thanks @uranusjr!
|
Bumps [pip](https://github.com/pypa/pip) from 21.0.1 to 21.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's changelog</a>.</em></p> <blockquote> <h1>21.1 (2021-04-24)</h1> <h2>Process</h2> <ul> <li>Start installation scheme migration from <code>distutils</code> to <code>sysconfig</code>. A warning is implemented to detect differences between the two implementations to encourage user reports, so we can avoid breakages before they happen.</li> </ul> <h2>Features</h2> <ul> <li>Add the ability for the new resolver to process URL constraints. (<code>[#8253](pypa/pip#8253) <https://github.com/pypa/pip/issues/8253></code>_)</li> <li>Add a feature <code>--use-feature=in-tree-build</code> to build local projects in-place when installing. This is expected to become the default behavior in pip 21.3; see <code>Installing from local packages <https://pip.pypa.io/en/stable/user_guide/#installing-from-local-packages></code>_ for more information. (<code>[#9091](pypa/pip#9091) <https://github.com/pypa/pip/issues/9091></code>_)</li> <li>Bring back the "(from versions: ...)" message, that was shown on resolution failures. (<code>[#9139](pypa/pip#9139) <https://github.com/pypa/pip/issues/9139></code>_)</li> <li>Add support for editable installs for project with only setup.cfg files. (<code>[#9547](pypa/pip#9547) <https://github.com/pypa/pip/issues/9547></code>_)</li> <li>Improve performance when picking the best file from indexes during <code>pip install</code>. (<code>[#9748](pypa/pip#9748) <https://github.com/pypa/pip/issues/9748></code>_)</li> <li>Warn instead of erroring out when doing a PEP 517 build in presence of <code>--build-option</code>. Warn when doing a PEP 517 build in presence of <code>--global-option</code>. (<code>[#9774](pypa/pip#9774) <https://github.com/pypa/pip/issues/9774></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fixed <code>--target</code> to work with <code>--editable</code> installs. (<code>[#4390](pypa/pip#4390) <https://github.com/pypa/pip/issues/4390></code>_)</li> <li>Add a warning, discouraging the usage of pip as root, outside a virtual environment. (<code>[#6409](pypa/pip#6409) <https://github.com/pypa/pip/issues/6409></code>_)</li> <li>Ignore <code>.dist-info</code> directories if the stem is not a valid Python distribution name, so they don't show up in e.g. <code>pip freeze</code>. (<code>[#7269](pypa/pip#7269) <https://github.com/pypa/pip/issues/7269></code>_)</li> <li>Only query the keyring for URLs that actually trigger error 401. This prevents an unnecessary keyring unlock prompt on every pip install invocation (even with default index URL which is not password protected). (<code>[#8090](pypa/pip#8090) <https://github.com/pypa/pip/issues/8090></code>_)</li> <li>Prevent packages already-installed alongside with pip to be injected into an isolated build environment during build-time dependency population. (<code>[#8214](pypa/pip#8214) <https://github.com/pypa/pip/issues/8214></code>_)</li> <li>Fix <code>pip freeze</code> permission denied error in order to display an understandable error message and offer solutions. (<code>[#8418](pypa/pip#8418) <https://github.com/pypa/pip/issues/8418></code>_)</li> <li>Correctly uninstall script files (from setuptools' <code>scripts</code> argument), when installed with <code>--user</code>. (<code>[#8733](pypa/pip#8733) <https://github.com/pypa/pip/issues/8733></code>_)</li> <li>New resolver: When a requirement is requested both via a direct URL (<code>req @ URL</code>) and via version specifier with extras (<code>req[extra]</code>), the resolver will now be able to use the URL to correctly resolve the requirement with extras. (<code>[#8785](pypa/pip#8785) <https://github.com/pypa/pip/issues/8785></code>_)</li> <li>New resolver: Show relevant entries from user-supplied constraint files in the error message to improve debuggability. (<code>[#9300](pypa/pip#9300) <https://github.com/pypa/pip/issues/9300></code>_)</li> <li>Avoid parsing version to make the version check more robust against lousily debundled downstream distributions. (<code>[#9348](pypa/pip#9348) <https://github.com/pypa/pip/issues/9348></code>_)</li> <li><code>--user</code> is no longer suggested incorrectly when pip fails with a permission error in a virtual environment. (<code>[#9409](pypa/pip#9409) <https://github.com/pypa/pip/issues/9409></code>_)</li> <li>Fix incorrect reporting on <code>Requires-Python</code> conflicts. (<code>[#9541](pypa/pip#9541) <https://github.com/pypa/pip/issues/9541></code>_)</li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/pip/commit/2b2a268d25963727c2a1c805de8f0246b9cd63f6"><code>2b2a268</code></a> Bump for release</li> <li><a href="https://github.com/pypa/pip/commit/ea761a6575f37b90cf89035ee8be3808cf872184"><code>ea761a6</code></a> Update AUTHORS.txt</li> <li><a href="https://github.com/pypa/pip/commit/2edd3fdf2af2f09dce5085ef0eb54684b4f9bc04"><code>2edd3fd</code></a> Postpone a deprecation to 21.2</li> <li><a href="https://github.com/pypa/pip/commit/3cccfbf169bd35133ee25d2543659b9c1e262f8c"><code>3cccfbf</code></a> Rename mislabeled news fragment</li> <li><a href="https://github.com/pypa/pip/commit/21cd124b5d40b510295c201b9152a65ac3337a37"><code>21cd124</code></a> Fix NEWS.rst placeholder position</li> <li><a href="https://github.com/pypa/pip/commit/e46bdda9711392fec0c45c1175bae6db847cb30b"><code>e46bdda</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9827">#9827</a> from pradyunsg/fix-git-improper-tag-handling</li> <li><a href="https://github.com/pypa/pip/commit/0e4938d269815a5bf1dd8c16e851cb1199fc5317"><code>0e4938d</code></a> 📰</li> <li><a href="https://github.com/pypa/pip/commit/ca832b2836e0bffa7cf95589acdcd71230f5834e"><code>ca832b2</code></a> Don't split git references on unicode separators</li> <li><a href="https://github.com/pypa/pip/commit/1320bac4ff80d76b8fba2c8b4b4614a40fb9c6c3"><code>1320bac</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9814">#9814</a> from pradyunsg/revamp-ci-apr-2021-v2</li> <li><a href="https://github.com/pypa/pip/commit/e9cc23ffd97cb6d66d32dc3ec27cf832524bb33d"><code>e9cc23f</code></a> Skip checks on PRs only</li> <li>Additional commits viewable in <a href="https://github.com/pypa/pip/compare/21.0.1...21.1">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details>
Environment
Description
When the enum34 package is installed on a system, pip will no longer install a project that uses pyproject.toml instead of setup.py. Instead, it tracebacks inside of the stdlib because pip is finding the enum34 package's version of enum.py instead of the stdlib version. This happens in the re-invocation of pip from
pip/_internal/commands/install.pyhttps://github.com/abadger/pip/blob/master/src/pip/_internal/build_env.py#L171 although I think the bug should be fixed in__main__.pyinstead.(edited: Put the correct file into the description)
The reason is that
__main__.pyhas code which changes sys.path in case we are running pip from a wheel. That code appears to be changing sys.path in certain circumstances when we aren't running from a wheel by mistake. So that code is where the bug lies, not in install. It may be correct to change install.py as well but I am leaning towards not doing that... It looks like the code in install is making sure that it reinvokes itself by specifying the actual path to pip which it was invoked with. If we tried to use something different likepython -mthere, then we wouldn't be assured of executing the code from the same install of pip.I'll add an easy reproducer using pip's repository in the reproducer section.
Fixed by #8213
Expected behavior
Even if enum34 is installed, pip should still be able to install other packages with a pyproject.toml file.
How to Reproduce
Output
The text was updated successfully, but these errors were encountered: