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

Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning #11057

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/11057.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning
before falling back to sys.executable
110 changes: 101 additions & 9 deletions src/pip/_internal/self_outdated_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
import logging
import optparse
import os.path
import platform
import shutil
import subprocess
import sys
from pathlib import Path
from typing import Any, Dict

from pip._vendor.packaging.version import parse as parse_version
Expand Down Expand Up @@ -96,6 +100,102 @@ def was_installed_by_pip(pkg: str) -> bool:
return dist is not None and "pip" == dist.installer


def get_py_executable() -> str:
"""Get path to launch a Python executable.

First test if python/python3/pythonX.Y on PATH matches the current
interpreter, and use that if possible. Then try to get the correct
pylauncher command to launch a process of the current python
version, fallback to sys.executable
"""

if not sys.executable:
# docs (python 3.10) says that sys.executable can be can be None or an
# empty string if this value cannot be determined, although this is
# very rare. In this case, there is nothing much we can do
return "python3"
Copy link
Member

Choose a reason for hiding this comment

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

This would be wrong on Windows, where there's never a python3 executable. I'm not sure giving the wrong answer is better than explicitly failing to give any answer.

Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to fail in this case.

Copy link
Author

Choose a reason for hiding this comment

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

I was inclined to spit an error too, but I did not want the error to be too disruptive. But I see, giving a wrong suggestion does not help much either, this can be changed to put another warning instead (in place of the upgrade suggestion warning)


# windows paths are case-insensitive, pathlib takes that into account
sys_executable_path = Path(sys.executable)

major, minor, *_ = sys.version_info

# first handle common case: test if path to python/python3/pythonX.Y
# matches sys.executable
for py in ("python", "python3", f"python{major}.{minor}"):
which = shutil.which(py)
if which is None:
continue

try:
# resolve() removes symlinks, normalises paths and makes them
# absolute
if Path(which).resolve() == sys_executable_path.resolve():
return py
Copy link
Member

Choose a reason for hiding this comment

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

What if the user has a shell alias that overrides what shutil.which returns? Then what we suggest won't work as we expect it to.

Also on a case-insensitive filesystem, this == needs to be a case insensitive comparison (although this will simply miss some possible abbreviations, not return something wrong).

Copy link
Member

Choose a reason for hiding this comment

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

In both these cases won't this check return False then and correctly use the sys.executable fallback?

Copy link
Author

Choose a reason for hiding this comment

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

What if the user has a shell alias that overrides what shutil.which returns? Then what we suggest won't work as we expect it to.

Hmmm that's a good point... I can think one workaround, looking into os.environ for aliases, but this would only work on Windows and cmd shell (yeah, would not even work with something like powershell for instance). Trying to workaround this for all other platforms/configurations seems impractical and error-prone, definitely high-maintenance for a relatively minor thing.

I believe #10959 too would also suffer from the same issue

Also on a case-insensitive filesystem, this == needs to be a case insensitive comparison (although this will simply miss some possible abbreviations, not return something wrong).

Well... We are comparing Path objects here so this is already handled internally by pathlib. Under windows it uses WindowsPath which does case insensitive comparison.


except RuntimeError:
# happens when resolve() encounters an infinite loop
pass

# version in the format used by pylauncher
pylauncher_version = f"-{major}.{minor}-{64 if sys.maxsize > 2**32 else 32}"

# checks that pylauncher is usable, also makes sure pylauncher recognises
# the current python version and has the correct path of the current
# executable.
try:
proc = subprocess.run(
["py", "--list-paths"],
Copy link
Member

Choose a reason for hiding this comment

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

Not all versions of the launcher support --list-paths - how can we be sure the user doesn't have an old version? I guess it just fails to find anything, so that's sort of fine.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that why it's an a try/except block and if that's the case correctly use the sys.executable fallback?

Copy link
Member

Choose a reason for hiding this comment

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

FYI it appears to be part of the Windows CPython 3.7 release onward and the documented flag to use is -0p not --list-paths: https://github.com/python/cpython/blob/3.7/Doc/whatsnew/3.7.rst#windows-only-changes

Copy link
Member

@pfmoore pfmoore Apr 22, 2022

Choose a reason for hiding this comment

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

Also, upgrading the launcher is optional, so people could end up using a later Python but an old launcher (one reason being if the launcher is installed globally, needing admin rights).

Isn't that why it's an a try/except block and if that's the case correctly use the sys.executable fallback?

Yeah, that's why I said "I guess it just fails to find anything, so that's sort of fine"...

Copy link
Author

@ankith26 ankith26 Apr 22, 2022

Choose a reason for hiding this comment

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

Yeah, it would fallback in this case (EDIT: github was being weird, when I sent this message it appeared that it was the first reply on this review chain, after a browser reload this comment seems to have come a few comments below)

capture_output=True,
timeout=1,
text=True,
check=True,
)

except (
subprocess.CalledProcessError,
subprocess.TimeoutExpired,
FileNotFoundError,
):
pass

else:
for line in proc.stdout.splitlines():
# this is not failsafe, in the future pylauncher might change
# the format of the output. In that case, this implementation
# would start falling back to sys.executable which is better than
# throwing unhandled exceptions to users
try:
line_ver, line_path = line.strip().split(maxsplit=1)
except ValueError:
# got less values to unpack
continue

# strip invalid characters in line_path
invalid_chars = "/\0" # \0 is NUL
if platform.system() == "Windows":
invalid_chars += '<>:"\\|?*'

line_path = line_path.strip(invalid_chars)
Copy link
Member

Choose a reason for hiding this comment

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

strip() only removes invalid characters from the start and end. Why would you want to do that? Also, why are you doing this anyway? If the returned value is invalid, what are you trying to achieve by making it valid?

try:
if (
line_ver == pylauncher_version
and Path(line_path).resolve() == sys_executable_path.resolve()
):
return f"py {line_ver}"
except RuntimeError:
# happens when resolve() encounters an infinite loop
pass

# Returning sys.executable is reliable, but this does not accommodate for
# spaces in the path string. Currently it is not possible to workaround
# without knowing the user's shell.
# Thus, it won't be done until possible through the standard library.
# Do not be tempted to use the undocumented subprocess.list2cmdline, it is
# considered an internal implementation detail for a reason.
return sys.executable


def pip_self_version_check(session: PipSession, options: optparse.Values) -> None:
"""Check for an update for pip.

Expand Down Expand Up @@ -165,15 +265,7 @@ def pip_self_version_check(session: PipSession, options: optparse.Values) -> Non
if not local_version_is_older:
return

# We cannot tell how the current pip is available in the current
# command context, so be pragmatic here and suggest the command
# that's always available. This does not accommodate spaces in
# `sys.executable` on purpose as it is not possible to do it
# correctly without knowing the user's shell. Thus,
# it won't be done until possible through the standard library.
# Do not be tempted to use the undocumented subprocess.list2cmdline.
# It is considered an internal implementation detail for a reason.
pip_cmd = f"{sys.executable} -m pip"
pip_cmd = f"{get_py_executable()} -m pip"
logger.warning(
"You are using pip version %s; however, version %s is "
"available.\nYou should consider upgrading via the "
Expand Down