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

Do not allow --target, --user, --root and --prefix together #7829

Open
McSinyx opened this issue Mar 8, 2020 · 9 comments
Open

Do not allow --target, --user, --root and --prefix together #7829

McSinyx opened this issue Mar 8, 2020 · 9 comments
Labels
state: needs discussion This needs some more discussion type: enhancement Improvements to functionality

Comments

@McSinyx
Copy link
Contributor

McSinyx commented Mar 8, 2020

--target, --user, --root and --prefix are mutually exclusive and simply don't make sense when used together. Instead of failing for irrelevant errors, the error should explicitly describe why it is not excepted. This is currently implemented in GH-7828. I move the discussion here since the PR to avoid distracting the code review process.

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Mar 8, 2020
@uranusjr
Copy link
Member

uranusjr commented Mar 8, 2020

Also see #7777, which implemented the --user and --target mutually exclusive check.

@McSinyx
Copy link
Contributor Author

McSinyx commented Mar 8, 2020

Current master also have --user and --prefix check. My proposed implementation is done here; I'm trying to ask if such behavior is welcomed and if is there any potential complication I may miss.

@pradyunsg pradyunsg added state: needs discussion This needs some more discussion type: enhancement Improvements to functionality labels Mar 8, 2020
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Mar 8, 2020
@pradyunsg
Copy link
Member

pradyunsg commented Mar 8, 2020

Flagging that there is a lot of nuance around the use / default values of --user and --target; so we should be very careful while changing behavior for those options.

I likely won't have the time or mental space to review this PR or participate in this discussion for a few weeks.

@McSinyx
Copy link
Contributor Author

McSinyx commented Mar 9, 2020

@pradyunsg: Since it seems that you're in middle of something else at the moment, may I remind you that you already approved raising error on --user and --target used together (GH-7777). Until this is revisited, I think it's best to save everybody's time by posting the current (master) behavior:

target and user

$ pip install --target /tmp --user appdirs
ERROR: Can not combine '--user' and '--target'

target and root

Something is seriously wrong here since (not /tmp/foo, /tmp/bar or /tmp/bar/tmp/foo but /tmp/bar/tmp):

$ pip install --target /tmp/foo --root /tmp/bar appdirs
Collecting appdirs
  Using cached appdirs-1.4.3-py2.py3-none-any.whl (12 kB)
Installing collected packages: appdirs
Successfully installed appdirs-1.4.3
$ ls /tmp/foo
$ tree /tmp/bar
/tmp/bar
└── tmp
    └── pip-target-_apxmir1
        └── lib
            └── python
                ├── appdirs-1.4.3.dist-info
                │   ├── DESCRIPTION.rst
                │   ├── INSTALLER
                │   ├── METADATA
                │   ├── metadata.json
                │   ├── RECORD
                │   ├── top_level.txt
                │   └── WHEEL
                ├── appdirs.py
                └── __pycache__
                    └── appdirs.cpython-37.pyc

target and prefix

Not really helpful error:

$ pip install --target /tmp --prefix /tmp appdirs
Collecting appdirs
  Using cached appdirs-1.4.3-py2.py3-none-any.whl (12 kB)
Installing collected packages: appdirs
ERROR: Exception:
Traceback (most recent call last):
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/cli/base_command.py", line 199, in _main
    status = self.run(options, args)
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/cli/req_command.py", line 185, in wrapper
    return func(self, options, args)
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/commands/install.py", line 407, in run
    use_user_site=options.use_user_site,
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/req/__init__.py", line 71, in install_given_reqs
    **kwargs
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/req/req_install.py", line 795, in install
    prefix=prefix,
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/locations.py", line 186, in get_scheme
    dist_name, user, home, root, isolated, prefix
  File "/home/cnx/.local/lib/python3.7/site-packages/pip/_internal/locations.py", line 118, in distutils_scheme
    assert not (home and prefix), "home={} prefix={}".format(home, prefix)
AssertionError: home=/tmp/pip-target-wie7l92e prefix=/tmp

user and root

Install to {root}/{user-site}: is this expected behavior?

$ pip install --user --root /tmp appdirs
Collecting appdirs
  Using cached appdirs-1.4.3-py2.py3-none-any.whl (12 kB)
Installing collected packages: appdirs
Successfully installed appdirs-1.4.3
$ tree -a /tmp/home
/tmp/home
└── cnx
    └── .local
        └── lib
            └── python3.7
                └── site-packages
                    ├── appdirs-1.4.3.dist-info
                    │   ├── DESCRIPTION.rst
                    │   ├── INSTALLER
                    │   ├── METADATA
                    │   ├── metadata.json
                    │   ├── RECORD
                    │   ├── top_level.txt
                    │   └── WHEEL
                    ├── appdirs.py
                    └── __pycache__
                        └── appdirs.cpython-37.pyc

user and prefix

$ pip install --user --prefix /tmp appdirs
ERROR: Can not combine '--user' and '--prefix' as they imply different installation locations

root and prefix

Install to {root}/{prefix}: is this expected behavior?

$ pip install --root /tmp/foo --prefix /tmp/baz appdirs
Collecting appdirs
  Using cached appdirs-1.4.3-py2.py3-none-any.whl (12 kB)
Installing collected packages: appdirs
Successfully installed appdirs-1.4.3
$ tree /tmp/foo/tmp/baz
/tmp/foo/tmp/baz
└── lib
    └── python3.7
        └── site-packages
            ├── appdirs-1.4.3.dist-info
            │   ├── DESCRIPTION.rst
            │   ├── INSTALLER
            │   ├── METADATA
            │   ├── metadata.json
            │   ├── RECORD
            │   ├── top_level.txt
            │   └── WHEEL
            ├── appdirs.py
            └── __pycache__
                └── appdirs.cpython-37.pyc

McSinyx added a commit to McSinyx/pip that referenced this issue Mar 12, 2020
@McSinyx
Copy link
Contributor Author

McSinyx commented Mar 31, 2020

I'm not sure if it's time but this basically propose a dirty patch for the same issue as discussed in GH-4575.

@McSinyx
Copy link
Contributor Author

McSinyx commented Apr 4, 2020

@pradyunsg and other @pypa/pip-committers, may we talk about this now?

@pfmoore
Copy link
Member

pfmoore commented Apr 4, 2020

There are subtleties here, as @pradyunsg said. I appreciate that current behaviour is weird and likely broken in many cases, but see this xkcd comic 🙂

Specific things that should be considered:

  • Users can set --user in their global config, and then they don't have a way of switching it off. Having it ignored rather than erroring out when combined with other flags may be important to such users. This may be sufficient to want to behave differently depending on whether the flag came from config or command line, Which isn't something we've ever done before, I believe.
  • Ubuntu/Debian patch pip to make --user the default. This is an even more extreme case of the previous point.
  • Some tools I believe force --root or --prefix - they are likely the main users of those flags (I don't think end users specify them manually very often) so we'd need to understand how they expect things to work, or be prepared to react quickly if we break something for them.

That's quite a lot of research to do.

With the new resolver work in full swing, and a release coming soon, I doubt many of the @pypa/pip-committers will have a lot of time to look at this issue in the near future. But if you were able to investigate some of the points above, and summarise your findings as you did with the current behaviour, that would help a lot when the discussion does get restarted.

Sorry if this sounds like stalling on the issue - the work you've put in here is genuinely appreciated, and I'd love to get this tidied up. Just not right now 😉

@McSinyx
Copy link
Contributor Author

McSinyx commented Apr 9, 2020

That's quite a lot of research to do.

I've been procrastinating on it for quite a while, and figured that incremental addition might be better since I don't have to rely on memory for the findings (I don't have the habit of noting things down, should start doing it some day 😉).

From the log above, there are only three cases where is not already an error, all of which involving --root. For the moment, I think we can move it out of the scope of this issue, since this is mainly to make the decision making logic easier to understand so I can come up with a correct conditional flow for GH-6762.

Anyway, to update the situation relating to this:

Users can set --user in their global config, and then they don't have a way of switching it off.

--target and --prefix are already marked as error in some recent commits.

Ubuntu/Debian patch pip to make --user the default.

I'm on Debian testing and it seems that downstream 20.0.2 is using the falling back to user-site logic instead of hardcoding like before.

Some tools I believe force --root or --prefix

I can confirm that after a quick GitHub source search. I don't understand why would any sane human ever mix --root with any of other location directive, but again, XKCD 1172 😞

So I believe, the discussion here is whether to support --root with other directives or not. If it's agreed that we should opt that out, I'd be happy to revise my PR.

McSinyx added a commit to McSinyx/pip that referenced this issue Jun 6, 2020
McSinyx added a commit to McSinyx/pip that referenced this issue Jul 8, 2020
@hexagonrecursion
Copy link
Contributor

target and root

Something is seriously wrong here since (not /tmp/foo, /tmp/bar or /tmp/bar/tmp/foo but /tmp/bar/tmp):

$ pip install --target /tmp/foo --root /tmp/bar appdirs
Collecting appdirs
  Using cached appdirs-1.4.3-py2.py3-none-any.whl (12 kB)
Installing collected packages: appdirs
Successfully installed appdirs-1.4.3
$ ls /tmp/foo
$ tree /tmp/bar
/tmp/bar
└── tmp
    └── pip-target-_apxmir1
        └── lib
            └── python
                ├── appdirs-1.4.3.dist-info
                │   ├── DESCRIPTION.rst
                │   ├── INSTALLER
                │   ├── METADATA
                │   ├── metadata.json
                │   ├── RECORD
                │   ├── top_level.txt
                │   └── WHEEL
                ├── appdirs.py
                └── __pycache__
                    └── appdirs.cpython-37.pyc

It smells like tempfile.mkdtemp(dir=root/'tmp', prefix='pip-target-') is used as a temporary installation location and the files are never moved into target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: needs discussion This needs some more discussion type: enhancement Improvements to functionality
Projects
None yet
Development

No branches or pull requests

5 participants