-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Support PEP 405 include-system-site-packages configuration #7155
Support PEP 405 include-system-site-packages configuration #7155
Conversation
d831bb5
to
cb3bb1d
Compare
Ahahahah. This breaks our tests, since they were installing in |
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.
To be clear, this is specifically to fix a bug where the expected pip behavior of "aborting if --user
is provided while using a virtual environment" doesn't work under venv (only virtualenv).
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.
For addressing review comments from the GitHub UI.
You're right, this probably doesn't do that. Amended. FWIW, IIUC, that PR is suggesting a behavior change from "aborting if |
e582d85
to
47593c4
Compare
Why: This would allow for use in an updated `virtualenv_no_global` that supports PEP 405 virtual environments.
Why: This change makes it easier to introduce handling for PEP 405 based virtual environment's global site-package exclusion logic
Why: PEP 405 virtual environments have a different mechanism for ignoring system site-packages in virtual environments.
sigh Our tests setup for venv is a lot more broken than I'd like. The following test fails with you run def test_ensure_script_isolates_environment(script):
"""
Test that sys.path when using `script`, does not reach into project directory.
This catches issues with nox and tox environments being used to run tests
and to ensure that they aren't being made available directly by `script`.
"""
result = script.run("python", "-c", "import sys\nprint(sys.path)")
got = ast.literal_eval(result.stdout)
problematic_locations = [
location
for location in got
if location and os.path.realpath(location).startswith(SRC_DIR)
]
assert not problematic_locations |
This change already missed one release window because our test suite was sub-par for dealing with venv. I'm inclined to merge this PR in, after adding skip markers to the failing tests (skip when using |
47593c4
to
a1eafde
Compare
Oh, I missed a test. |
a1eafde
to
89309ab
Compare
Our isolation logic for venv isn't correct and that is causing these tests to fail. The culprits for this are: tests/lib/venv.py::VirtualEnvironment.user_site_packages tests/lib/venv.py::VirtualEnvironment.sitecustomize Both these together are supposed to create an environment to isolate the tests. However, they were written for virtualenv and make assumptions that are not true for environments created with venv. Until we can fix VirtualEnvironment to properly isolate the test from the underlying test environment when using venv, these tests will continue to fail. This is blocking an important bugfix for users facing issues with since pip is installing packages into `--user` when run in a venv, even when `--user` isn't visible from that environment. As a temporary band-aid for this problem, I'm skipping these tests to unblock us from shipping the bugfix for the aforementioned issue. The test isolation logic should be fixed to work for venv. Once such a fix is made, this commit should be reverted.
89309ab
to
8981895
Compare
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.
As mentionned in #7270 we'd likely want to add some test running with --use-venv
in our CI.
We do -- Travis CI and Appveyor run with --use-venv. |
Merging since there's an approval and the only build failing is RTD's new preview hooks that we've opted-in for testing. |
Closes #5702
Supercedes and closes #7149
Basically, pip was never updated to also look at PEP 405's system-site-packages mechanism.
That's what caused a fairly widespread "bad" bug. :(