Default to --user install in certain conditions

[takluyver]

A possible solution to #1668. I'm opening this for discussion, not as something ready to merge - I don't want to spend too much time on the details if the basic idea is no good.

Background

With Python installed systemwide on Linux, pip install typically fails because it tries to install packages somewhere where the user doesn't have write access. Some distros try to make --user installs the default to ease this - e.g. Debian has done so in the past, and Manjaro is experimenting with it. This can conflict with use cases which the distro maintainers weren't aware of, leading to confusion, and to issues which upstream developers reject as caused by distros (e.g. #3826, #4222, pypa/virtualenv#1416).

This was mitigated by an improved error message in #5239, but there's still a desire to change behaviour.

The challenge with changing the default is that there are many scenarios where you don't want a user install. Installing in virtualenvs is the most obvious one people run into, but there are other environment tools such as conda and pyenv (pip's running_under_virtualenv function does not detect these). Then there are cases like installing packages as root in a docker container - you don't want to get a user install as root!

Proposal

This PR aims to enable --user by default under very specific conditions:

1. The user has not explicitly specified otherwise in the command line, environment variable, or a config file.
2. The --prefix and --target options are not in use.
3. The global site-packages dir is not writeable, so doing a non-user install would fail.
4. User site-packages are enabled - to avoid surprises in isolated environments.

Crucially, it doesn't need to decide if it's in an environment - I don't think that question makes sense.

Checking if a directory is writeable on Windows is a bit tricky, because of some Python limitations. I've copied code I use for the same purpose in Flit, but it has had orders of magnitude less use than pip, so I wouldn't be surprised if it fails in some edge cases.

Drawbacks

More complex behaviour would be more to understand.

E.g. if on a shared system I create a conda environment, and someone with read-only access tries to add to it using pip, they would get a user install - so it would appear to work, but break isolation and not modify the environment for collaborators. Whereas with the status quo they would get a PermissionError showing that something is wrong.

Arguably tools for conveniently creating Python environments should disable user site packages by default. But there are scenarios where it's very convenient (e.g. an institution provides a common conda environment, users can add a few packages without needing to learn about creating environments themselves).

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[pradyunsg]

There's an approach that we have concensus over -- #1668 (comment)

This is fairly similar AFAICT but yea, I have to take a closer look at this proposal and the one linked to above, to think through what the effects (and differences) of these changes are.

[takluyver]

Thanks, that's quite similar. It looks like the difference between this and Donald's 2015 proposal are:

1. My approach doesn't try to detect virtualenvs - I want user created virtualenvs, conda envs or pyenvs to get similar behaviour, and I don't know a good way to detect them all.
2. Donald wanted to eventually make --user the default even if the global site-packages is writeable. As a consequence of 1, that's not part of my plan.

[takluyver]

As there's some interest in this, I've rebased it and fixed some issues.

There are two remaining test failures on Windows, both checking that a no-op install command doesn't modify files. This code checks if a directory is writeable on Windows by trying to create a file in it, and immediately removing that file if it succeeds. That updates the directory mtime, which is enough to cause those failures.

Possible solutions:

1. Relax the tests.
2. Find some way to check if a directory is writable on Windows without creating a file there. This discussion on a Python tempfile issue suggests that it's not feasible.
3. Only check the permissions when pip is definitely going to try to install something. This sounds good, but in practice I suspect that keeping it after the last possible point where another check can bail out, while still having all the necessary effects to make user-install work, would get messy.

[pfmoore]

+1 on just relaxing the test (if possible, just allow and ignore the mtime change, but I'd be OK with a broader relaxation if that's too fiddly).

[takluyver]

I don't think such a fine-grained relaxation is possible without getting deep into the test fixtures - they're both just asserting that no files have been modified during a run of pip.

I've attempted a somewhat coarser relaxation that I think still leaves the essential purpose of those two tests intact.

[pfmoore]

Yes, that LGTM

[pradyunsg]

I have a request here (not a demand, please feel free to say no if you'd prefer to not do it).

Since we're adding some complexity to how we're setting the value use_user_site, I'd like to factor out all the handling into a top-level function in the module (like the ones we already have at the end).

That should make it much easier to test this logic and IMO makes it easier to reason about this bit of code.

(I'd rephrase this to better words, but little time and this is decent enough; sorry!)

[takluyver]

I've had a go at that in the latest commit, but I'm not convinced it's clearer - the function needs quite a few options passed in, so it's quite tightly coupled to the code it's factored out of. Maybe there's a better way to do this that I haven't thought of?

[pradyunsg]

IMO it's definitely better than inline, since the logic is encapsulated in a single location and we can utilize early returns + type annotations closer to the logic.

Maybe there's a better way to do this that I haven't thought of?

I'd prefer we add early returns more aggresively, and add an assert use_user_site is None, all with a few more comments to explain what's happening.

def decide_user_install(
    use_user_site,  # type: Optional[bool]
    prefix_path,  # type: Optional[str]
    target_dir,  # type: Optional[str]
    root_path,  # type: Optional[str]
    isolated_mode,  # type: bool
):
    # type: (...) -> bool
    """Determine whether to do a user install based on the input options.

    If use_user_site is False, no additional checks are done.
    If use_user_site is True, it is checked for compatibility with other
    options.
    If use_user_site is None, the default behaviour depends on the environment,
    which is provided by the other arguments.
    """
    if use_user_site is False:
        return False

    if use_user_site is True:
        if prefix_path:
            raise CommandError(
                "Can not combine '--user' and '--prefix' as they imply "
                "different installation locations"
            )
        if virtualenv_no_global():
            raise InstallationError(
                "Can not perform a '--user' install. User site-packages "
                "are not visible in this virtualenv."
            )
        return True

    # If we are here, user installs have not been explicitly requested/avoided
    assert use_user_site is None

    # user install incompatible with --prefix/--target
    if prefix_path or target_dir:
        return False

    # If user installs are not enabled, choose a non-user install
    if not site.ENABLE_USER_SITE:
        return False

    # If we don't have permissions for a non-user install, choose a user install
    return not site_packages_writable(
        root=root_path, isolated=isolated_mode,
    )

[pradyunsg]

To be abundantly clear, if you don't think this is an improvement and reckon it's better to not do it this way, please do feel free to revert the change adding the dedicated function.

I'd prefer that we spend energy discussing the fixes being proposed here, rather than a code-style-like detail. :)

[pradyunsg]

LGTM. Once we make the choices w.r.t. implementation/code style, I'm good to go with this PR.

[takluyver]

I've added logging - only going up to info at the moment, because warning kind of implies that there's something wrong.

And I've added unit tests covering all the things decide_user_site does to determine the default. I started writing tests for the cases where user install is explicitly requested or prohibited, but it quickly felt like I was just asserting that the code is exactly what it is, which feels pointless, so I dropped those. I'll add them back if people really want them, but for projects I maintain I prefer not to have tests like that.

[chrahunt]

The current test coverage is fine IMO. Given that it's existing behavior I assume it's covered by existing functional tests (but it would be nice to know for sure).

[takluyver]

I've rewritten most of the tests with pytest's parametrize decorator, as @chrahunt suggested. Personally, I find this kind of test harder to read than the more verbose 'set x, assert y' style, but if other people prefer it, that's fine.

[chrahunt]

Thank you! I can sympathize with that. For me it makes it easier to see that all scenarios are covered. For example, originally the first and last assertions were actually checking the same thing, but it took a few seconds to verify; now it's obvious that there's no duplicate or missing test IMO.

[takluyver]

That makes sense. :-)

[pradyunsg]

I'll be merging this in tomorrow (~12 hours after). If anyone has blocking concerns w.r.t this PR, please holler. :)

[atugushev]

FYI, this PR might break tox (tested with tox==3.14.0). Simple reproducer:

$ cd $(mktemp -d)

$ cat setup.py
from setuptools import setup
setup()

$ cat tox.ini
[tox]
envlist = py37
[testenv]
deps = -e git+https://github.com/pypa/pip.git@master#egg=pip
commands = pip --version

$ tox -v
using tox.ini: /Users/albert/Projects/test-package/tox.ini (pid 16383)
using tox-3.14.0 from /usr/local/lib/python2.7/site-packages/tox/__init__.pyc (pid 16383)
GLOB sdist-make: /Users/albert/Projects/test-package/setup.py
[16388] /Users/albert/Projects/test-package$ /usr/local/opt/python@2/bin/python2.7 setup.py sdist --formats=zip --dist-dir /Users/albert/Projects/test-package/.tox/dist >.tox/log/GLOB-0.log
package .tmp/package/1/atugushev-testpackage-0.1.12.zip links to dist/atugushev-testpackage-0.1.12.zip (/Users/albert/Projects/test-package/.tox)
py37 cannot reuse: no previous config /Users/albert/Projects/test-package/.tox/py37/.tox-config1
py37 create: /Users/albert/Projects/test-package/.tox/py37
[16393] /Users/albert/Projects/test-package/.tox$ /usr/local/opt/python@2/bin/python2.7 -m virtualenv --no-download --python /Users/albert/Projects/test-package/.venv/bin/python3.7 py37 >py37/log/py37-0.log
py37 installdeps: -egit+https://github.com/pypa/pip.git@master#egg=pip
[16408] /Users/albert/Projects/test-package$ /Users/albert/Projects/test-package/.tox/py37/bin/python -m pip install '-egit+https://github.com/pypa/pip.git@master#egg=pip' >.tox/py37/log/py37-1.log
py37 inst: /Users/albert/Projects/test-package/.tox/.tmp/package/1/atugushev-testpackage-0.1.12.zip
write config to /Users/albert/Projects/test-package/.tox/py37/.tox-config1 as '6ca0676fb0398a654a2483eb31e082f6cfba52beb10b4af1387e8e3741382f9a /Users/albert/Projects/test-package/.venv/bin/python3.7\n3.14.0 0 0 0\n00000000000000000000000000000000 -egit+https://github.com/pypa/pip.git@master#egg=pip'
[16478] /Users/albert/Projects/test-package$ /Users/albert/Projects/test-package/.tox/py37/bin/python -m pip install --exists-action w .tox/.tmp/package/1/atugushev-testpackage-0.1.12.zip >.tox/py37/log/py37-2.log
ERROR: invocation failed (exit code 2), logfile: /Users/albert/Projects/test-package/.tox/py37/log/py37-2.log
================================================== log start ==================================================
ERROR: Exception:
Traceback (most recent call last):
  File "/Users/albert/Projects/test-package/.tox/py37/src/pip/src/pip/_internal/cli/base_command.py", line 153, in _main
    status = self.run(options, args)
  File "/Users/albert/Projects/test-package/.tox/py37/src/pip/src/pip/_internal/commands/install.py", line 302, in run
    isolated_mode=options.isolated_mode,
  File "/Users/albert/Projects/test-package/.tox/py37/src/pip/src/pip/_internal/commands/install.py", line 638, in decide_user_install
    assert use_user_site is None
AssertionError

=================================================== log end ===================================================
___________________________________________________ summary ___________________________________________________
ERROR:   py37: InvocationError for command /Users/albert/Projects/test-package/.tox/py37/bin/python -m pip install --exists-action w .tox/.tmp/package/1/atugushev-testpackage-0.1.12.zip (exited with code 2)

Caught in pip-tools, haven't investigated yet deeply, just let you know.

[pradyunsg]

sigh Why is our option handling so sad?

With:

diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py
index e50155c2..5aca32f2 100644
--- a/src/pip/_internal/commands/install.py
+++ b/src/pip/_internal/commands/install.py
@@ -635,7 +635,7 @@ def decide_user_install(
         return True
 
     # If we are here, user installs have not been explicitly requested/avoided
-    assert use_user_site is None
+    assert use_user_site is None, use_user_site
 
     # user install incompatible with --prefix/--target
     if prefix_path or target_dir:

I see that the issue is that:

Traceback (most recent call last):
  File "/Users/pradyunsg/Projects/pip/src/pip/_internal/cli/base_command.py", line 153, in _main
    status = self.run(options, args)
  File "/Users/pradyunsg/Projects/pip/src/pip/_internal/commands/install.py", line 302, in run
    isolated_mode=options.isolated_mode,
  File "/Users/pradyunsg/Projects/pip/src/pip/_internal/commands/install.py", line 638, in decide_user_install
    assert use_user_site is None, use_user_site
AssertionError: 0

[pradyunsg]

BTW, thanks for testing this out @atugushev! ^>^

[takluyver]

Should we relax it to allow 0/1? Or any int? Or convert anything besides None to a boolean, for strictest compatibility with whatever it might have received before? Or should this be fixed in the option parsing so that it can only get True/False/None in this code?

[pradyunsg]

I think we should note why we're dealing with this issue, and change the comparisons from is to ==:

>>> True == 0
False
>>> True == 1
True
>>> False == 0
True
>>> False == 1
False

[FichteFoll]

Or should this be fixed in the option parsing so that it can only get True/False/None in this code?

This would be my preferred solution. The setting to "use user site" is a tri-state boolean type and all its value states should honor that. Does it get set from other places than option parsing?

[takluyver]

That's conceptually the right solution, but I guess it might not be easy to change the combined command-line, environment variable and config file handling like that.

[pradyunsg]

If someone's interested in a deeper fix than what I've suggested above, I more than welcome investigation into that -- a PR would be great too. :)