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

Add a --python option #11320

Merged
merged 19 commits into from
Aug 6, 2022
Merged

Add a --python option #11320

merged 19 commits into from
Aug 6, 2022

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Jul 28, 2022

This is an initial draft for a --python option for pip, that runs pip with a specified Python interpreter.

To do:

  • Tests
  • Documentation
  • Refactoring (_get_runnable_pip should probably be moved somewhere more general)
  • Support friendlier interpreter names, not just the full path (check how other apps do this - is there a common library?)

The use of an environment variable to stop infinitely creating new subprocesses feels OK to me, but is there a better way? Stripping the --python option out of the command line is probably impractical (i.e., even more risk of missing something and fork-bombing the user's machine 🙁).

@pfmoore
Copy link
Member Author

pfmoore commented Jul 28, 2022

Support friendlier interpreter names

Virtualenv and tox have a pretty sophisticated mechanism for supporting names like py310. It's nice, but probably not appropriate for our needs. I believe it focuses more on locating "system" Python interpreters, whereas we'll be more likely to want to focus on finding virtual environments.

In reality, I would expect the --python option to be mostly useful in the following use cases:

  1. To manage a virtual environment that does not itself have pip installed.
  2. To run a standalone copy of pip (for example the zipapp, or installed via pipx).

For case (1), we need to point at the environment's interpreter. A path (absolute or relative) to that interpreter is probably perfectly sufficient. We might want to allow a shortcut of pointing to the virtualenv (a directory containing either bin/python or Scripts/python.exe). I'd personally use such a shortcut quite frequently (pip --python .venv install ...).

For case (2), we might also want to point at the currently active interpreter. Allowing an alias of "py" or "python", would cover this. We'd need to smooth over platform differences here, but that's a minor point. Note that I don't think we should try to detect when pip is run via pipx, and automatically supply --python python - not doing so means that for a pipx-installed python, pip install foo will quietly do the wrong thing, but I'm fine with that (it's not a core installation method - yet 😉).

Basically, let's keep it simple until we get some actual use cases.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Nice! A couple of total nit-picks.

src/pip/_internal/cli/main_parser.py Outdated Show resolved Hide resolved
src/pip/_internal/cli/main_parser.py Show resolved Hide resolved
Comment on lines 74 to 76
# TODO: On Windows, `--python .venv/Scripts/python` won't pass the
# exists() check (no .exe extension supplied). But it's pretty
# obvious what the user intends. Should we allow this?
Copy link
Member

Choose a reason for hiding this comment

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

We could do this without too much fuss with

directory, filename = os.path.split(python)
if shutil.which(filename, path=directory) is not None:
    ...

Copy link
Member Author

Choose a reason for hiding this comment

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

From the shutil.which docs,

On Windows, the current directory is always prepended to the path whether or not you use the default or provide your own

That could give weird false positives (for example, we have a python.exe in the CWD, and we do --python=.venv/python.exe).

On reflection, I think this is too much effort, and we should just expect the user to supply the .exe (i.e., provide the name of a file that exists). I suspect most people will use autocomplete anyway, if they are entering the path by hand.

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

I'm on board with this. Thanks for this work!

src/pip/_internal/cli/main_parser.py Show resolved Hide resolved
@sbidoul
Copy link
Member

sbidoul commented Jul 29, 2022

Ah, another thing: I tried pip --python /usr and it happens /usr/bin/python is Python 2, so it aborts with an ugly stack trace. So we may want __pip-runner__.py to be syntax compatible with python 2 + 3 and start with a version compatibility check ?

@pfmoore
Copy link
Member Author

pfmoore commented Jul 29, 2022

I tried pip --python /usr

Ouch, that's not a usage I'd expected (/usr isn't a virtual environment!) While I sort of agree that we should fail more gracefully if someone points at a Python interpreter that the invoked pip doesn't support, I also think that passing a directory should be restricted to virtual environments. I'm inclined to check for pyvenv.cfg, and we can then get the Python interpreter version from there (annoyingly, while both venv and virtualenv supply this information, they record it under different names, but I'm OK with dealing with that).

So how about the following:

  1. We restrict the --python directory_name syntax to directories containing pyvenv.cfg.
  2. We read the python version from pyvenv.cfg and check that it's a version that pip supports.
  3. If the user supplies an executable, and there's a pyvenv.cfg in the directory above the executable, proceed as above. If there's no pyvenv.cfg, skip the version check and assume the user knows what they are doing.
  4. If the user specifies py or python, do the pyvenv.cfg check on the executable we get from shutil.which.

I don't mind making __pip-runner__.py do a version check and use Python 2 compatible syntax, but it feels like an annoying restriction that we'll be bound to forget over time, so I'd rather not bother (given the above checks).

Also, do we have a machine readable definition of what Python versions pip supports? I guess I could get it via importlib.metadata.metadata("pip")["requires-python"], but we may not always have metadata (e.g., the zipapp, which deliberately excludes it so that pip doesn't show up in the user's environment). I could just hard-code it, but there's a risk of people not remembering to update it. I'm happy not single-sourcing it (IMO that gets too complicated) but maybe we could add a __supports_python__ attribute in pip/__init__.py?

@sbidoul
Copy link
Member

sbidoul commented Jul 29, 2022

Ouch, that's not a usage I'd expected (/usr isn't a virtual environment!)

Eh eh, just me trying to do end-user-ish things :)

But what's wrong with not having a virtual environment?

@pfmoore
Copy link
Member Author

pfmoore commented Jul 29, 2022

But what's wrong with not having a virtual environment?

Nothing at all, but I view "provide a directory name" as a shortcut for "provide a virtual environment". If you don't have a virtual environment, just supply the executable name1. In the docs, I've explicitly said that what you can pass is "the path to a virtual environment", not a general directory.

Footnotes

  1. On Windows, your trick wouldn't work, as the python executable isn't in a Scripts subdirectory in non-virtual environments.

docs/html/user_guide.rst Outdated Show resolved Hide resolved
@uranusjr
Copy link
Member

Maybe we should check for python3 on non-Windows instead? PEP 394 recommends scripts to use python3, so this command is likely to be present on all reasonably recent operating systems.

@pfmoore
Copy link
Member Author

pfmoore commented Jul 29, 2022

Can anyone shed any light on why the github actions Ubuntu runner is showing a bunch of packages present in a newly-created --without-pip venv? It doesn't happen on my local PC, either on Windows or on Ubuntu (under WSL). I'm at the point of wanting to blame some weird Ubuntu/Debian patching or hack, but I'll be honest, that's mostly just because I can't think what else it could be 🙁

Hmm. Why the heck does the venv sys.path contain the nox site-packages???

['',
 '/home/runner/work/pip/pip/.nox/test-3-7/lib/python37.zip',
 '/home/runner/work/pip/pip/.nox/test-3-7/lib/python3.7',
 '/home/runner/work/pip/pip/.nox/test-3-7/lib/python3.7/lib-dynload',
 '/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7',
 '/home/runner/work/pip/pip/.nox/test-3-7/lib/python3.7/site-packages']

If anyone has any suggestions, either on what might be happening or how to debug this further, I'd be really grateful.

@pfmoore
Copy link
Member Author

pfmoore commented Jul 29, 2022

I could of course weaken the test, to not assert that the environment is empty before and after, but that seems bad - a major point here is that the environment doesn't need anything in it 🙁

@pfmoore pfmoore force-pushed the python_option branch 4 times, most recently from db42458 to dde2b17 Compare July 29, 2022 16:14
@pfmoore
Copy link
Member Author

pfmoore commented Jul 29, 2022

# /home/runner/work/pip/pip/.nox/test-3-10/lib/python3.10/site-packages/_distutils_hack/__pycache__/__init__.cpython-310.pyc matches /home/runner/work/pip/pip/.nox/test-3-10/lib/python3.10/site-packages/_distutils_hack/__init__.py

Hmm, is this somehow related to the (accurately named, IMO 🙂) setuptools _distutils_hack? @pradyunsg I think you were involved in the discussions about how pip-runner interacted with that. Any ideas?

@sbidoul
Copy link
Member

sbidoul commented Jul 31, 2022

@pfmoore I sent pfmoore#4 your way to let the pip runner fail gracefully on Python 2 too.

@pfmoore
Copy link
Member Author

pfmoore commented Jul 31, 2022

@pfmoore I sent pfmoore#4 your way to let the pip runner fail gracefully on Python 2 too.

That's quite a substantial change, and not just to the version check code. I don't want to simply accept that without consensus from @pypa/pip-committers that we're OK requiring the limitation to Python 2 compatible syntax in __pip-runner__.py.

Personally, I'm rather aggressively against catering for Python 2, so my instinct is to reject it. But I know that's an extreme view, so I'd like to mitigate it by getting other people to tell me I'm wrong 🙂

On the other hand, I do like the simplification you made to the version check - I was being far too clever for my own good there. I'll happily accept that regardless of the question over Python 2. (Don't worry about changing your PR, I'll cherry-pick the bits we agree on by hand if needed).

@sbidoul
Copy link
Member

sbidoul commented Jul 31, 2022

It's the solution I found to avoid an ugly syntax error stack trace when running pip --python /some/python2 (which is what happens now when you do pip --python python). There might be something simpler that I did not think of.

@pfmoore
Copy link
Member Author

pfmoore commented Jul 31, 2022

There might be something simpler that I did not think of.

If we restrict the option to only supporting virtual environments, we can introspect pyvenv.cfg and do the check before running the subprocess. But I guess pip --python /usr/bin/python3 list --format=json is a reasonable use case that we'd like to support, so that's not sufficient.

However, pip --python /usr/bin/cat list will dump out __pip-runner__.py, and pip --python /usr/bin/vi list will open an editor on it. At some point, we probably have to accept that the only answer is "don't do that". I accept that putting "point at Python 2" on the "don't do that" side of the line might be wrong, but we can't protect the user from everything.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Jul 31, 2022
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 1, 2022
@pfmoore
Copy link
Member Author

pfmoore commented Aug 1, 2022

I don't want to simply accept that without consensus from https:/orgs/pypa/teams/pip-committers that we're OK requiring the limitation to Python 2 compatible syntax in pip-runner.py.

One specific issue I have is that I don't see any way to easily test that we keep to Python 2 compatible syntax. I guess we could add a Python 2 CI test, but I'd prefer a lint check that we can run locally (i.e. without needing Python 2 installed...) Personally, I don't even remember what counts as Python 2 compatible these days...

@pfmoore
Copy link
Member Author

pfmoore commented Aug 1, 2022

Thinking some more about this, though, the main changes for Python 2 compatibility are removing type annotations in favour of # type: ignore, which doesn't bother me too much. So if we're OK with the risk that someone inadvertantly breaks Python 2 compatibility, I withdraw my objections.

It makes me somewhat sad that I find the version without type annotations a lot more readable than the annotated version...

Comment on lines 35 to 45
def check_python_version() -> None:
# Import here to ensure the imports happen after the sys.meta_path change.
from pip._vendor.packaging.specifiers import SpecifierSet
from pip._vendor.packaging.version import Version

py_ver = Version("{0.major}.{0.minor}.{0.micro}".format(sys.version_info))
if py_ver not in SpecifierSet(PYTHON_REQUIRES):
raise SystemExit(
f"This version of pip does not support python {py_ver} "
f"(requires {PYTHON_REQUIRES})"
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def check_python_version() -> None:
# Import here to ensure the imports happen after the sys.meta_path change.
from pip._vendor.packaging.specifiers import SpecifierSet
from pip._vendor.packaging.version import Version
py_ver = Version("{0.major}.{0.minor}.{0.micro}".format(sys.version_info))
if py_ver not in SpecifierSet(PYTHON_REQUIRES):
raise SystemExit(
f"This version of pip does not support python {py_ver} "
f"(requires {PYTHON_REQUIRES})"
)
def check_python_version() -> None:
if sys.version_info < (3, 7):
raise SystemExit(
f"This version of pip requires Python 3.7+. You have:\n"
f"{sys.version}"
)

Copy link
Member

Choose a reason for hiding this comment

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

We aren't going to get more complicated anytime in the future, so I wouldn't bother with trying to be fully flexible here TBH.

@pradyunsg
Copy link
Member

FWIW, I think we should omit all the changes to the pip-runner from this PR, and make them in a separate follow up. It's a valid point that we should present a better error message -- but that's auxilary to the main functionality change in this PR and there's no reason to block the rest of the discussion on handling the edge case of unsupported Python versions with the runner (even though, yes, I admit we'll hit that and should improve that). :)

@pfmoore
Copy link
Member Author

pfmoore commented Aug 3, 2022

FWIW, I think we should omit all the changes to the pip-runner from this PR, and make them in a separate follow up.

That's an excellent idea. I've done that, and added #11342 for the runner change.

@sbidoul sbidoul added this to the 22.3 milestone Aug 3, 2022
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

A small update to the doc, otherwise looks good to me.

@pfmoore
Copy link
Member Author

pfmoore commented Aug 3, 2022

Fixed, thanks.

@pfmoore
Copy link
Member Author

pfmoore commented Aug 4, 2022

OK, I believe that's all the review comments addressed. Unless anyone flags any last minute objections, I'll merge this at the weekend.

@pfmoore pfmoore merged commit 9473e83 into pypa:main Aug 6, 2022
@pfmoore pfmoore deleted the python_option branch August 6, 2022 14:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants