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-22490: Remove __PYVENV_LAUNCHER__ from environment during launch #9516

Merged
merged 5 commits into from
Mar 22, 2020

Conversation

ronaldoussoren
Copy link
Contributor

@ronaldoussoren ronaldoussoren commented Sep 23, 2018

Alternative patch for bpo-22490 that removes the shell environment variable PYVENV_LAUNCHER during interpreter launch because it is only used to communicate between the stub executable in Mac/Tools/pythonw.c and the real interpreter.

This avoids problems when launching the "real" interpreter without going through the stub executable.

https://bugs.python.org/issue22490

… macOS

This changeset removes the environment varialbe "__PYVENV_LAUNCHER__"
during interpreter launch as it is only needed to communicate between
the stub executable in framework installs and the actual interpreter.

Leaving the environment variable present may lead to misbehaviour when
launching other scripts.
Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think it would be nice if, since #9498 didn't fail any tests, there were a documented way (even if just in the ticket), one could verify that the behavior is working as intended (a test that would fail with #9498).

@gvanrossum
Copy link
Member

I would love to know how to repro the original bug so I can demonstrate this actually fixes it.

Here's what I think should be the repro:

/Library/Frameworks/Python.framework/Versions/3.8/bin/python3  -c 'import subprocess; p = subprocess.Popen([".mypy/venv/bin/python3","-m","mypy.dmypy","-h"]).communicate()'

The setup is that .mypy/venv is a virtualenv that has mypy.dmypy installed. This is what failed for the user who reported this.

Unfortunately this does not fail for me (with a framework build from master installed). Would I have to do the install via brew? (I would have to learn about locally testing modified brew recipes, I suppose.)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

The patch LG from a code review POV.

@ned-deily
Copy link
Member

@ronaldoussoren ping?

@gpshead gpshead added the needs backport to 3.8 only security fixes label Sep 28, 2019
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jan 15, 2020
…ld-system-reviewers,rstewart

The __PYVENV_LAUNCHER__ is set on macos to indicate which
python to use. Sadly, keeping this set confuses pip in
virtualenvs.
This corresponds to python/cpython#9516.

Differential Revision: https://phabricator.services.mozilla.com/D59853
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 15, 2020
…ld-system-reviewers,rstewart

The __PYVENV_LAUNCHER__ is set on macos to indicate which
python to use. Sadly, keeping this set confuses pip in
virtualenvs.
This corresponds to python/cpython#9516.

Differential Revision: https://phabricator.services.mozilla.com/D59853

--HG--
extra : moz-landing-system : lando
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jan 16, 2020
…ld-system-reviewers,rstewart

The __PYVENV_LAUNCHER__ is set on macos to indicate which
python to use. Sadly, keeping this set confuses pip in
virtualenvs.
This corresponds to python/cpython#9516.

Differential Revision: https://phabricator.services.mozilla.com/D59853

UltraBlame original commit: ed91bca1f4a669454801390a1e5432fa2cd9561e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jan 16, 2020
…ld-system-reviewers,rstewart

The __PYVENV_LAUNCHER__ is set on macos to indicate which
python to use. Sadly, keeping this set confuses pip in
virtualenvs.
This corresponds to python/cpython#9516.

Differential Revision: https://phabricator.services.mozilla.com/D59853

UltraBlame original commit: ed91bca1f4a669454801390a1e5432fa2cd9561e
@mattip
Copy link
Contributor

mattip commented Feb 27, 2020

We think this is borking creation of a PyPy virtualenv using cpython host on macOS and is was very difficult to detect, so it would be nice to clean this up. For a test, you could try to install pypy 7.3.0 using cpython, see https://foss.heptapod.net/pypy/pypy/issues/3175. Although now that we know we will probably fix it for PyPy's next release.

Mac/Tools/pythonw.c Outdated Show resolved Hide resolved
Co-Authored-By: Nicola Soranzo <nicola.soranzo@gmail.com>
@jaraco jaraco requested review from vsajip and a team as code owners March 22, 2020 14:55
@jaraco
Copy link
Member

jaraco commented Mar 22, 2020

The test failure is also happening in master, so unrelated to this change.

@jaraco jaraco merged commit 044cf94 into python:master Mar 22, 2020
@miss-islington
Copy link
Contributor

Thanks @ronaldoussoren for the PR, and @jaraco for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-19110 is a backport of this pull request to the 3.8 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Mar 22, 2020
@miss-islington
Copy link
Contributor

Sorry, @ronaldoussoren and @jaraco, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 044cf94f610e831464a69a8e713dad89878824ce 3.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 22, 2020
…ythonGH-9516)

* bpo-22490: Remove "__PYVENV_LAUNCHER__" from the shell environment on macOS

This changeset removes the environment varialbe "__PYVENV_LAUNCHER__"
during interpreter launch as it is only needed to communicate between
the stub executable in framework installs and the actual interpreter.

Leaving the environment variable present may lead to misbehaviour when
launching other scripts.

* Actually commit the changes for issue 22490...

* Correct typo

Co-Authored-By: Nicola Soranzo <nicola.soranzo@gmail.com>

* Run make patchcheck

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>
(cherry picked from commit 044cf94)

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>
@bedevere-bot
Copy link

GH-19111 is a backport of this pull request to the 3.7 branch.

jaraco pushed a commit that referenced this pull request Mar 22, 2020
…H-9516) (GH-19110)

* bpo-22490: Remove "__PYVENV_LAUNCHER__" from the shell environment on macOS

This changeset removes the environment varialbe "__PYVENV_LAUNCHER__"
during interpreter launch as it is only needed to communicate between
the stub executable in framework installs and the actual interpreter.

Leaving the environment variable present may lead to misbehaviour when
launching other scripts.

* Actually commit the changes for issue 22490...

* Correct typo

Co-Authored-By: Nicola Soranzo <nicola.soranzo@gmail.com>

* Run make patchcheck

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>
(cherry picked from commit 044cf94)

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>
jaraco added a commit that referenced this pull request Mar 22, 2020
…aunch (GH-9516) (GH-19111)

* bpo-22490: Remove "__PYVENV_LAUNCHER__" from the shell environment on macOS

This changeset removes the environment varialbe "__PYVENV_LAUNCHER__"
during interpreter launch as it is only needed to communicate between
the stub executable in framework installs and the actual interpreter.

Leaving the environment variable present may lead to misbehaviour when
launching other scripts.

* Actually commit the changes for issue 22490...

* Correct typo

Co-Authored-By: Nicola Soranzo <nicola.soranzo@gmail.com>

* Run make patchcheck

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>.
(cherry picked from commit 044cf94)

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>
foolip added a commit to web-platform-tests/wpt that referenced this pull request Apr 23, 2021
Exactly how this all works isn't clear, but since the fix in
python/cpython#9516 was to simply hide the
environment variable from the interpreter it seems reasonably safe.

Fixes #27377.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants