Mutually exclusive --target, --user and --prefix

[techtonik]

Rebased #4111.

(edit by @pradyunsg: fixes #4390)

[pradyunsg]

Thanks for this! ^.^

@techtonik You have a failing test, could you look into it?

[techtonik]

I can't read the test right away. Not clear for me what is it doing. Some methods were deprecated and I don't really have the time to study them.

[dpkp]

fix for test failure:

diff --git a/tests/functional/test_install_reqs.py b/tests/functional/test_install_reqs.py
index 93592b2..1fedb20 100644
--- a/tests/functional/test_install_reqs.py
+++ b/tests/functional/test_install_reqs.py
@@ -262,6 +262,7 @@ def test_wheel_target_with_prefix_in_pydistutils_cfg(script, data,
                                                      virtualenv):
     # Make sure wheel is available in the virtualenv
     script.pip('install', 'wheel')
+    script.pip('download', 'setuptools', 'wheel', '-d', data.packages)
     virtualenv.system_site_packages = True
     homedir = script.environ["HOME"]
     script.scratch_path.join("bin").mkdir()

[pradyunsg]

fix for test failure:

@techtonik This indeed does fix the test. Could you add this to your PR?

In fact, you don't need to install wheel for this test.

[Bo98]

The tests have since changed, so common_wheels will be the way to go here, which will resolve the failing test.

See b709b1e#diff-0ef5cb8465369ba3cca1484fcfb75e7fL226

Could you update the PR @techtonik?

[pradyunsg]

Just to build it.

[techtonik]

Thanks. Fixed the comment.

[pypa-bot]

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 eligible for code review and hopefully merging!

[pradyunsg]

I would phrase this as:

pip install `--target` is now mutually exclusive with `--user` and `--prefix`.

[techtonik]

Done.

[techtonik]

Travis outage?

[pradyunsg]

The pypy job is flaky because of an upstream pytest bug. #4687 should help with that. No idea about the py34 job though.

I've restarted both of them.

[techtonik]

Looks like it doesn't help. Still some problems on Travis with websockets, probably unrelated but still.. https://www.traviscistatus.com/

[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 eligible for code review and hopefully merging!

[techtonik]

I am out of mana. (

[dpkp]

I think this is an important fix and would love to help push across the finish line. I believe this fixes the merge conflict:

diff --cc src/pip/_internal/locations.py
index 5a20c92a,1043e57a..00000000
--- a/src/pip/_internal/locations.py
+++ b/src/pip/_internal/locations.py
@@@ -155,10 -155,12 +155,12 @@@ def distutils_scheme(dist_name, user=Fa
      # NOTE: setting user or home has the side-effect of creating the home dir
      # or user base for installations during finalize_options()
      # ideally, we'd prefer a scheme class that has no side-effects.
 -    assert not (user and prefix), "user={0} prefix={1}".format(user, prefix)
 -    assert not (home and prefix), "home={0} prefix={1}".format(home, prefix)
 -    assert not (home and user), "home={0} user={1}".format(home, user)
 +    assert not (user and prefix), "user={} prefix={}".format(user, prefix)
-     i.user = user or i.user
-     if user:
++    assert not (home and prefix), "home={} prefix={}".format(home, prefix)
++    assert not (home and user), "home={} user={}".format(home, user)
+     if user or home:
          i.prefix = ""
+     i.user = user or i.user
      i.prefix = prefix or i.prefix
      i.home = home or i.home
      i.root = root or i.root

[pradyunsg]

I think I did that right.

@techtonik Could you take a look to see if the merge is indeed correct?

[techtonik]

I will try to rebase, but if anybody can explain the role ~/.pydistutils.cfg and how it is used for tests - that will help greatly. I don't want to leave "I don't know what I am doing" for commit message. :D

[RonnyPfannschmidt]

this is a toxic destructive test - if ever any user with a preconfigured configfile runs it, they will face a lost configuration, and potentially hours or days of figuring why something they knew worked is suddenly broken

[techtonik]

I just copied that from the test below to avoid code duplication in many places. I have no idea how this config file is supposed to be used.

[techtonik]

The code in current master does the same.

pip/tests/functional/test_install_reqs.py

Lines 232 to 236 in 404838a

	if os.name == 'posix':
	user_filename = ".pydistutils.cfg"
	else:
	user_filename = "pydistutils.cfg"
	user_cfg = os.path.join(os.path.expanduser('~'), user_filename)

So yes, it points to user HOME, but I thought that test framework should mock that, no?

[techtonik]

@RonnyPfannschmidt so any ideas how to change that?

[pradyunsg]

This is fine since we have an isolate fixture to isolate the tests from the actual system, which is used when virtualenv fixture is used.

[benoit-pierre]

AFAIK, by the time we isolate, it's too late, since this is done during collection.

[pradyunsg]

Ah right. The execution during collection can be deferred using a function then, with the appropriate comment on why it's a function.

[pradyunsg]

anybody can explain the role ~/.pydistutils.cfg

It's basically a configuration file to pass options to distutils (i.e. setup.py considers those values, which can be overidden by CLI options to setup.py that pip does not pass) -- https://docs.python.org/3/install/index.html#distutils-configuration-files

[lorencarvalho]

In case anyone is curious how pydistutils.cfg (and it's various permutations) gets discovered by distutils, the pertinent bits are here:
https://github.com/python/cpython/blob/8cf4b34b3665b8bb39ea7111e6b5c3410899d3e4/Lib/distutils/dist.py#L333-L363

of note is the line:

    The file in the user's home directory can be disabled with the
   --no-user-cfg option.

which actually only applies to source distributions, doesn't apply to wheels (since this code was written before wheels existed)

[pradyunsg]

Sorry for the lack of a heads-up.

I'm dropping this PR from the 18.1 milestone since there seem to be changes still needed here; albeit minor ones.

[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!

[jaraco]

This PR is a really important one. Pip is currently broken for a very basic use-case (#4106), among others. I should think this would be a blocker, but instead, now two contributors have been burned out trying to fix it. Perhaps one of the project's maintainers would consider adopting this PR and push it over the line, enacting whatever changes are necessary to make it stable and satisfactory?

[pfmoore]

@jaraco CI needs fixing - it appears to simply be a lint fix, so that shouldn't be hard. After that, it's more a matter of fixing review comments and getting someone to care enough.

There seems to be some debate over the tests, which I don't really understand but I think needs addressing before this can be merged (or at least @pradyunsg and @RonnyPfannschmidt need to confirm that they are now happy with the test.

As to moving this forward, I stated my own position above - I have no issues with this, but I'm not sufficiently convinced that this change is important to be willing to merge it without being sure that the various concerns others have raised have been addressed. @techtonik hasn't fixed the issue that was pointed out, he's asked for someone else to suggest a fix, but that hasn't happened. IMO, unless someone cares enough about this PR to (a) explain its importance better, and (b) follow up on review comments. then it'll probably remain unmerged.

#4390 (which AIUI was the trigger for this) is simply saying we should fail more gracefully if --target is specified with other options that don't make sense in conjunction with it. So I'm not clear how pip can be "broken" because of this, it's not like there's any behaviour that should work which doesn't, simply our failure mode for some unsupported cases is suboptimal.

#4106, to which you refer, seems to be about not being able to use --target if you have an incompatible option (--user, presumably, although it's not confirmed in the issue discussion) set in global config. This doesn't fix that, nor is there any agreed way forward which would fix that (other than "don't set things in your global config that are incompatible with the options you want to use"). A resolution would be nice, for the people affected by it, but it's likely to need a pretty comprehensive overhaul of the option system, which wasn't really designed with people setting fundamental behaviour-altering flags (like --user) in global config, in mind.

Note - I'm entirely happy if @techtonik (or anyone else) says they haven't got the interest to push this forward. I'm not demanding anything here, quite the opposite, I'm saying that no-one feels the problem is important enough to be worth getting stressed about. (Or more accurately, no-one feels this solution is that important, and no-one has yet stepped up to contribute on the more fundamental issue).

[jaraco]

This [pr] doesn't fix #4106.

In that case, since #4106 is the main issue that I'm concerned about, perhaps that issue should be re-opened and unbundled from #4390.

I'll provide more detail in #4106 to describe how this isn't about --user and it's implicated in a very vanilla install.

[Bo98]

One thing this looks like it might fix is:
http://stackoverflow.com/questions/24257803/distutilsoptionerror-must-supply-either-home-or-prefix-exec-prefix-not-both for Homebrew Python.

[jaraco]

@Bo98: There are several workarounds in that SO post, none of which are really robust as each requires mutating the user's file system in ways that aren't reliably available. For instance, writing to ./setup.cfg or ~/.distutils.cfg (aka ~/distutils.cfg on Windows) could overwrite existing values in those files. Plus, these workarounds are not properly coupled with the issue (they're user-global or project-local, whereas the cause is proximate to the Python environment). None of those is a proper fix.

[lorencarvalho]

The rudimentary workaround mentioned above (and formerly suggested by the Homebrew maintainers) works but is not a good long term solution due to the fact that modifying a global configuration file is fraught with issues. For example, if pip were to modify that file (even temporarily) other concurrently running processes could have their configuration unexpectedly changed out from underneath them. A good example of this can be found in linkedin/shiv#49

[Bo98]

@jaraco Exactly - I meant that this PR has the proper fix. @pfmoore wanted an example of where this solution is needed.

I was more referring to the problem rather than the workarounds.

[pradyunsg]

Thanks for this PR and the discussion around it!

I'm closing this PR since it's bitrotten. Please file a new PR if there's still interest in this.