Skip to content

bpo-35872 and bpo-35873: Clears __PYVENV_LAUNCHER__ variable #11745

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

Merged
merged 3 commits into from
Feb 4, 2019

Conversation

zooba
Copy link
Member

@zooba zooba commented Feb 3, 2019

Clears __PYVENV_LAUNCHER__ variable after reading it and sets sys._base_executable value for later use.
This allows simple use of sys.executable to launch in the current venv (either during execution or later), and internal use of sys._base_executable to reference the original Python when necessary (for example, when using multiprocessing or creating a new venv).
Since the variable is cleared, using the full path to a non-venv Python will no longer launch in the venv.

https://bugs.python.org/issue35872

…ading it and sets sys._base_executable value for later use.
@zooba
Copy link
Member Author

zooba commented Feb 3, 2019

@ned-deily I made a few changes that affect macOS here, but they should be all neutral. Despite the differences in calculating sys._base_executable, I don't think there are any difference in the places where we want to use it, so this avoids a few special cases.

@zooba
Copy link
Member Author

zooba commented Feb 3, 2019

@vstinner This is an important bugfix for 3.7, but doesn't have to end up in 3.8 in this form - presumably it's best to go into the config structures like the other values.

However, since we're so close to 3.8a1 I'd rather get the fix in with a private attribute and we can add the full configurability later. Does that sound okay?

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

LGTM. I did a non-exhaustive test of some macOS configurations with no obvious failures.

@vstinner
Copy link
Member

vstinner commented Feb 3, 2019

I am in PTO for one week, I will not be able to review properly this change.

A better design would be to expose the internal "core config" and "main config", but we didn't agree with Nick Coghlan and Eric Snow how to expose them. Anyway, even if we expose it in 3.8, it wouldn't help for 3.7.

I guess that a private sys attribute is fine as soon as we can replace it with a better solution whenever.

IMHO a better solution would include to rewrite site in C to call it earlier in the initialisation of the config. But that's a very large and intrusive project...

@zooba
Copy link
Member Author

zooba commented Feb 4, 2019

@vstinner Thanks, I won't hold this up for a full review, just wanted to check the idea and make sure you weren't close to any major work here.

IMHO a better solution would include to rewrite site in C to call it earlier in the initialisation of the config. But that's a very large and intrusive project...

Personally I'd like to rewrite initialisation into Python and figure out how to be able to run it at startup, rather than putting all this in C. This is probably a bigger and more intrusive project, but I think the long-term value is worth it :) But right now, I just want to fix this bug.

@zooba zooba merged commit a8474d0 into python:master Feb 4, 2019
@miss-islington
Copy link
Contributor

Thanks @zooba for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @zooba, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker a8474d025cab794257d2fd0bea67840779b9351f 3.7

@zooba zooba deleted the bpo35873 branch February 4, 2019 07:20
zooba added a commit to zooba/cpython that referenced this pull request Feb 4, 2019
…H-11745)

After reading __PYVENV_LAUNCHER__ we now set sys._base_executable value for later use.
Make the same changes for macOS to avoid extra platform checks.
@bedevere-bot
Copy link

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

zooba added a commit that referenced this pull request Feb 4, 2019
After reading __PYVENV_LAUNCHER__ we now set sys._base_executable value for later use.
Make the same changes for macOS to avoid extra platform checks.
@vstinner
Copy link
Member

Personally I'd like to rewrite initialisation into Python and figure out how to be able to run it at startup, rather than putting all this in C.

That's someone compatible with PEP 432 design which redesign Python initialization into multiple steps and you are free to stop at any stage. For example, Nick would like to be able to get a Python initialized without access to the filesystem.

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

Successfully merging this pull request may close these issues.

6 participants