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

Use sys.executable to format upgrade message #7532

Merged
merged 2 commits into from
Jan 7, 2020

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 31, 2019

Fix #7376.

Note this still fails if sys.executable contains space. This is really difficult to fix because shlex.quote() does not work on Windows, and is not available in Python 2.7 anyway. But various parts of ecosystem also do not work well with a prefix with space, so this is probably not the most significant problem.

There are a couple of potential improvements available:

  • Maybe use sys.argv[0] on non-Windows?
  • Manually look through PATH and prettify to value? (Say if sys.executable is /usr/local/bin/python3.7 and PATH has /usr/local/bin we can kind of safely suggest python3.7).

@pfmoore
Copy link
Member

pfmoore commented Dec 31, 2019

Manually look through PATH and prettify to value? (Say if sys.executable is /usr/local/bin/python3.7 and PATH has /usr/local/bin we can kind of safely suggest python3.7).

Not sure this one is worth it. For safety, we'd need to check that no directory on PATH before /usr/local/bin contained a python3.7 executable. And shell aliases could still mess us up.

IMO, we should just print sys.executable, and accept that it's a bit ugly.

@uranusjr
Copy link
Member Author

It is not too complicated if shutil.which() is available, but yeah I completely agree neither of those are particularly worthwhile :)

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.

Can we add a test to ensure this code path is exercised?

But various parts of ecosystem also do not work well with a prefix with space, so this is probably not the most significant problem.

Are there examples showing how prevalent this is? I only know of a numpy installation issue related to having spaces in paths. Either way I don't have a problem with this being tracked as a separate enhancement if at all.

src/pip/_internal/self_outdated_check.py Outdated Show resolved Hide resolved
src/pip/_internal/self_outdated_check.py Outdated Show resolved Hide resolved
@uranusjr
Copy link
Member Author

uranusjr commented Jan 1, 2020

Comment updated

# command context, so be pragmatic here and suggest the command
# that's always available. This doea not accomodate spaces in
# `sys.executable`.
pip_cmd = "{} -m pip".format(sys.executable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using pipes/shlex.quote for commands with spaces

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of those work on Windows. In other situations the effect is extremely marginal (e.g. shebangs don’t work so almost no pip users would try that).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work if all of the following hold:

  1. generated console script
  2. wheel install
  3. on Unix

because we delegate to distlib which uses a neat trick to handle long and space-containing Python installation paths.

Can we make a separate issue to follow up on handling space-containing Python paths?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a good reason for not just surrounding with quotes is because that would be bad advice on e.g. PowerShell, which would need something like &'C:\path\to a\python.exe' -m pip.

Copy link
Member

@pfmoore pfmoore Jan 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. We're talking about advising the user to run {sys.executable} -m pip" and whether to quote sys.executable. Where do shebang lines come into the discussion?

As @chrahunt pointed out, quoting is wrong in Powershell, so blindly quoting is wrong here. Quoting on Unix may well be OK, although this will still be wrong for anyone using the cross-platform Powershell Core.

IMO, at some point we have to give up on hand-holding the user too much, and expect a certain amount of common sense. Personally, I think that suggesting python -m pip with a following note saying "use the path to your Python interpreter if you don't invoke Python using the python command" should be sufficient, But I get the impression others disagree...

Copy link
Member Author

@uranusjr uranusjr Jan 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned the shebang thing as a (misguided) example that a prefix with space is problematic and might not be worthwhile to handle. Sorry for the digression.

I also don’t think opening a new issue for the space in prefix situation is worthwhile, mostly because I don’t see there is a way to completely fix it at all, and feel it’s just as good to leave it along.

I don’t like unconditionally suggesting python -m pip because an uninformed user tends to simply copy-pasting whatever the tool suggests, and this can very easily break the user’s environment in a lot of cases. sys.executable feels like a good compromise to me; it wouldn’t always generate the right command, but at least the user will be updating the correct pip. And in the space-in-prefix situation it is almost guaranteed to not run at all (instead of running but doing the wrong thing), giving the user a chance to review the command before it takes effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, sys.executable seems like a good compromise.

Co-Authored-By: Christopher Hunt <chrahunt@gmail.com>
@uranusjr uranusjr closed this Jan 7, 2020
@uranusjr uranusjr reopened this Jan 7, 2020
@pradyunsg pradyunsg dismissed chrahunt’s stale review January 7, 2020 12:33

I think it's a good idea to have a test here, but it's not a blocker for a test here.

@pradyunsg pradyunsg added the type: enhancement Improvements to functionality label Jan 7, 2020
@pradyunsg
Copy link
Member

pradyunsg commented Jan 7, 2020

/cc @uranusjr to file an issue the requested test (it seems to be pretty tricky to test this without significant monkeypatching, so a dedicated discussion would help).

@pradyunsg pradyunsg merged commit 52309f9 into pypa:master Jan 7, 2020
@pradyunsg
Copy link
Member

Thanks @uranusjr for the PR! ^>^

@uranusjr
Copy link
Member Author

uranusjr commented Jan 7, 2020

I filed #7568

@uranusjr uranusjr deleted the pip-upgrade-prompt-message branch January 13, 2020 09:27
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 12, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 12, 2020
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.

Incorrect command is shown to upgrade pip3
5 participants