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

bpo-42041: Clarify how Windows path search affects virtual environments #22715

Merged
merged 4 commits into from
Oct 20, 2020

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Oct 15, 2020

Copy link

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@pfmoore
Copy link
Member Author

pfmoore commented Oct 15, 2020

I've added the "skip news" label as this is simply a docs clarification.

@pfmoore
Copy link
Member Author

pfmoore commented Oct 18, 2020

Updated based on discussions on the tracker.

@pfmoore
Copy link
Member Author

pfmoore commented Oct 18, 2020

@vsajip Are you OK with the revised version?

Copy link
Member

@vsajip vsajip left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -356,6 +356,15 @@ functions.
arguments for additional differences from the default behavior. Unless
otherwise stated, it is recommended to pass *args* as a sequence.

.. warning::

Using the string ``"python"`` as the program to execute is not a good
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally feel like we should not single out python executable because wouldn't the same apply with any other program? IMHO we should point out that on Windows non-absolute path executables don't resolve by using the PATH environment variable and as such we encourage users to pass executables as absolute paths. And then we can mention to use shutil.which if they want a discover executable using the PATH environment variable, and in particular if they want to execute a script with the running python sys.executable. Wouldn't that be more accurate and general?

Copy link
Member

Choose a reason for hiding this comment

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

It will only apply to anything inside the same directory as python.exe, which is basically just python. In a venv, it's also anything that installed a script wrapper, so pip or virtualenv, among others.

That said, it's been "broken" for 2-3 full releases now (depending on how you count), and the only issues I've heard of were people who thought "python" would be found on PATH. So I don't think switching it back is super important. Arguably, we made it closer to "search PATH".

@@ -356,6 +356,25 @@ functions.
arguments for additional differences from the default behavior. Unless
otherwise stated, it is recommended to pass *args* as a sequence.

.. warning::
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also change os.execvp to os.execvpe at the beginning of the Popen docs?

Also, further down in the cwd section it claims "In particular, the function looks for executable (or for the first item in args) relative to cwd if the executable path is a relative path". That should be changed to restrict it to just POSIX. You could change "In particular" to "In POSIX".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@pfmoore pfmoore merged commit 5ab27cc into python:master Oct 20, 2020
@pfmoore pfmoore deleted the venv_exe_search branch October 20, 2020 20:02
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…GH-22715)

Clarify in the subprocess documentation how searching for the executable to run works, noting that ``sys.executable`` is the recommended way to find the current interpreter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants