Skip to content
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

Merged
merged 8 commits into from
Nov 6, 2019

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Oct 7, 2019

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. :(

@pradyunsg pradyunsg added type: enhancement Improvements to functionality type: bugfix skip news Does not need a NEWS file entry (eg: trivial changes) and removed skip news Does not need a NEWS file entry (eg: trivial changes) labels Oct 7, 2019
@pradyunsg pradyunsg force-pushed the support-pep-405-site-packages branch from d831bb5 to cb3bb1d Compare October 7, 2019 10:44
@pradyunsg
Copy link
Member Author

Ahahahah.

This breaks our tests, since they were installing in --user because of this bug. :)

Copy link
Member

@chrahunt chrahunt left a 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).

src/pip/_internal/utils/virtualenv.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/virtualenv.py Show resolved Hide resolved
src/pip/_internal/utils/virtualenv.py Outdated Show resolved Hide resolved
@chrahunt
Copy link
Member

chrahunt commented Oct 8, 2019

I don't think this closes #6984. For that I think we just need to provide clear indication of where to take that discussion - I think #4575?

Copy link
Member Author

@pradyunsg pradyunsg left a 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.

src/pip/_internal/utils/virtualenv.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/virtualenv.py Outdated Show resolved Hide resolved
@pradyunsg
Copy link
Member Author

I don't think this closes #6984

You're right, this probably doesn't do that. Amended.

FWIW, IIUC, that PR is suggesting a behavior change from "aborting if --user is provided while using a virtual environment" to "ignore --user while using a virtual environment".

@pradyunsg pradyunsg force-pushed the support-pep-405-site-packages branch from e582d85 to 47593c4 Compare October 8, 2019 08:48
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.
@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 5, 2019

sigh

Our tests setup for venv is a lot more broken than I'd like. The following test fails with you run tox -e py37 -- -k test_ensure_script_isolates_environment --use-venv. Dropping the --use-venv test, makes the tests pass, implying that --use-venv does not isolate tests properly. :(

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

@pradyunsg
Copy link
Member Author

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 --use-venv), since the underlying problem is with our test suite setup and not the code change.

@pradyunsg pradyunsg force-pushed the support-pep-405-site-packages branch from 47593c4 to a1eafde Compare November 5, 2019 15:38
@pradyunsg
Copy link
Member Author

Oh, I missed a test.

@pradyunsg pradyunsg force-pushed the support-pep-405-site-packages branch from a1eafde to 89309ab Compare November 5, 2019 17:26
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.
@pradyunsg pradyunsg force-pushed the support-pep-405-site-packages branch from 89309ab to 8981895 Compare November 5, 2019 17:36
Copy link
Member

@xavfernandez xavfernandez left a 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.

@pradyunsg
Copy link
Member Author

We do -- Travis CI and Appveyor run with --use-venv.

@pradyunsg pradyunsg closed this Nov 6, 2019
@pradyunsg pradyunsg reopened this Nov 6, 2019
@pradyunsg
Copy link
Member Author

Merging since there's an approval and the only build failing is RTD's new preview hooks that we've opted-in for testing.

@pradyunsg pradyunsg merged commit 7b3d3a3 into pypa:master Nov 6, 2019
@pradyunsg pradyunsg deleted the support-pep-405-site-packages branch November 6, 2019 06:17
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip install --user misbehaves inside venv
4 participants