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

Fix pip install --target #4103

Closed
wants to merge 7 commits into from
Closed

Fix pip install --target #4103

wants to merge 7 commits into from

Conversation

halms
Copy link

@halms halms commented Nov 9, 2016

Currently pip install --target does fail when a prefix is set (e.g. in a .pydistutils.cfg).
This is especially the case on macOS when Python is installed via Homebrew.
It used to fail in the same way when using the --user flag. This was initially fixed in b227c45

Now I just replicated this fix for the usage of --target.

Interestingly, --target used to work when supplying --install-option="--prefix=" at the command line as advised in Homebrew-and-Python.md, but now doesn't anymore.
The error message changes though whether --install-option="--prefix=" is set or not. - When the option is set, pip fails when get_lib_location_guesses calls distutils_scheme, so it seems the --install-prefix switch is ignored there.

Anyway, this pull request should fix it all by enforcing prefix to be empty when --target is used.
Although this works fine for me, I am not yet sure if these changes would break anything else,

@RonnyPfannschmidt
Copy link
Contributor

do you think you can derive an acceptance test for this?

@techtonik
Copy link
Contributor

Do --prefix and --target mutually exclusive or should work together?

@halms
Copy link
Author

halms commented Nov 10, 2016

Regarding exclusivity:
I have no proof that they should be mutually exclusive, but I strongly think so. From what I've seen, regardless if a wheel or a non-wheel package should be installed, distutils is somehow used.
For wheels it is used to directly install them. For non-wheels it is used to determine the installation path (which could be set in a pydistutils.cfg). [Please correct me if I'm wrong here].

Setting --target actually sets home for the underlying distutils and there it is definitely not possible to set both home and prefix.
That leads to a DistutilsOptionError: must supply either home or prefix/exec-prefix -- not both
Therefore I can only conclude that the cannot be used simultaneously.

Regarding tests:
I do not really have much experience with testing, but from what I've seen in the test scripts I might come up with something.
Probably just copying and adapting the tests for --user and --prefix should do fine.

@techtonik
Copy link
Contributor

techtonik commented Nov 10, 2016

So, if --user, --target and --prefix are mutually exclusive, what is the priority if they are coming from different sources?

@RonnyPfannschmidt
Copy link
Contributor

my opinion is that the priority in order should be --target --prefix --user

@halms
Copy link
Author

halms commented Nov 10, 2016

My guess would be the one given most explicitly should have the highest priority, i.e. anything directly set on the command line overrides defaults set somewhere else. (There we can easily check if more than one is given and throw an error).

The other question is what happens when multiple defaults are set somewhere. (Ubuntu has --user as a default now, Homebrew has a --prefix, I don't know yet of any combination).
I'd say though if defaults are set that are contradicting each other, failing loudly instead of doing something quietly is a valid option.

@techtonik
Copy link
Contributor

techtonik commented Nov 10, 2016

I suppose that priorities are (low)-->(high) hardcoded_into_pip --> specified_in_config (which?) --> specified_on_command_line

Is there a mechanism to know that some --target option is default, is set in config and not set from command line? So that --target specified on command line will override --user in config (with a message). And clash with error if both are specified on the same level. (the problem sounds generic enough for something like http://exercism.io/)

halmi and others added 4 commits November 10, 2016 17:39
Also override defaults set somewhere (e.g. .pydistutils.cfg) when command line switches are given.
Also check if prefix from .pydistutils.cfg is overriden.
@halms halms closed this Nov 13, 2016
@halms halms deleted the fix-install-target branch November 13, 2016 16:54
@halms
Copy link
Author

halms commented Nov 13, 2016

Sorry, I messed up my git and ended up deleting and recreating the branch. Did not know that this will force-close the pull-request.
I will open a new one.

@Rustinante
Copy link

What happened to this pull request in the end?

@pradyunsg
Copy link
Member

#4557 handles all of this.

@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants