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

Please test/review Python for issue 22490 #52046

Closed
jaraco opened this issue Mar 22, 2020 · 6 comments
Closed

Please test/review Python for issue 22490 #52046

jaraco opened this issue Mar 22, 2020 · 6 comments
Labels
outdated PR was locked due to age

Comments

@jaraco
Copy link

jaraco commented Mar 22, 2020

In bpo-22490, a bugfix is now in place for an issue where PYVENV_LAUNCHER would leak to the interpreter and child processes. The fix was applied today to the master, 3.8, and 3.7 branches.

One action item that remained was to test the change against homebrew. Please consider testing one of these branches against homebrew as time permits. Thanks.

@iMichka
Copy link
Member

iMichka commented Mar 22, 2020

Hi. We may need a little bit of time to test this. The initial issue was reported in 2014 and there is a lot of discussion in that ticket. @tdsmith may give us some help if he is still around :)

@MikeMcQuaid MikeMcQuaid transferred this issue from Homebrew/brew Mar 23, 2020
@SMillerDev
Copy link
Member

Can you make a test script that would prove this is resolved?

@jonchang
Copy link
Contributor

I suspect that this is the reproduction as originally stated in pypa/pip#2031

# incorrect
% /usr/local/opt/python/bin/python3 -c "import os; print(os.environ['__PYVENV_LAUNCHER__'])"
/usr/local/Cellar/python/3.7.7/bin/python3

# correct
% /usr/bin/python -c "import os; print(os.environ['__PYVENV_LAUNCHER__'])"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/UserDict.py", line 40, in __getitem__
    raise KeyError(key)
KeyError: '__PYVENV_LAUNCHER__'

And it seems the relevant patches are here:

master: python/cpython#9516
3.8: python/cpython#19110
3.7: python/cpython#19111

@tdsmith
Copy link
Contributor

tdsmith commented Mar 26, 2020

The lingering manifestation of this in Homebrew was the case here: https://bugs.python.org/issue22490#msg283859

The persistence of __PYVENV_LAUNCHER__ poisons the sys.executable of virtualenv interpreters launched as a subprocess of another Python interpreter:

$ virtualenv -p python3 test

# Expected
$ test/bin/python3 -c 'import sys; print(sys.executable)'
/Users/tim/test/bin/python3

# Not expected
$ /usr/local/bin/python3 -c 'import subprocess; subprocess.call(["/Users/tim/test/bin/python3", "-c", "import sys; print(sys.executable)"])'
/usr/local/bin/python3

# This is the right outcome, but we had to delete __PYVENV_LAUNCHER__ manually
$ /usr/local/bin/python3 -c 'import subprocess, os; del os.environ["__PYVENV_LAUNCHER__"]; subprocess.call(["/Users/tim/test/bin/python3", "-c", "import sys; print(sys.executable)"])'
/Users/tim/test/bin/python3

We could identify that this is fixed if running the second case gave e.g. /Users/tim/test/bin/python3.

I don't think there's anything Homebrew-specific about this anymore; there was an issue with script shebangs but pip 6 (which is quite old now) fixed that for us.

@stale
Copy link

stale bot commented Apr 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Apr 17, 2020
@Bo98 Bo98 added the in progress Stale bot should stay away label Apr 17, 2020
@stale stale bot removed the stale No recent activity label Apr 17, 2020
@iMichka
Copy link
Member

iMichka commented Apr 17, 2020

@tdsmith brought some insights and I understand, this looks good now, or at least pip 6 already fixed this for us back then. So the fix does not change anything for us I guess.

Thanks for merging the patch though!

@iMichka iMichka closed this as completed Apr 17, 2020
@Bo98 Bo98 removed the in progress Stale bot should stay away label Apr 17, 2020
@lock lock bot added the outdated PR was locked due to age label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

No branches or pull requests

6 participants