bpo-34977: Use venv redirector instead of original python.exe on Windows by zooba · Pull Request #11029 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Previously on Windows we would copy enough of Python's binaries into a virtual environment that it could run independently from the original install. This has two problems: the first is that updating your main Python install would give you incompatible binaries for the standard library now in use, and the second is that when the main Python install needs to be loaded in a special context, the copied binary would not be capable of configuring this (for example, when the main install has come from an app package).
This change creates a slightly modified version of py.exe that redirects based on a pyvenv.cfg file and copies only that python.exe instead of the real one. As a result, the base Python install can be safely upgraded without breaking all venvs, and venv creation is significantly faster.
https://bugs.python.org/issue34977
@vstinner Just waiting on clean CI and your approval, so feel free to merge when ready. (The venv experts have had over a month to review the change already when I pinged them on the other issue, so I don't see a good reason to wait longer when there's been no code change.)
GH-11030 is a backport of this pull request to the 3.7 branch.
Note: Rather than backporting manually, I prefer to add "needs backport 3.7" label to automatically generate the backport. So I don't have to update my PR twice if I have to change my PR. (For your larger App Store change, you wrote that 3.7 and master changes are different, I don't know if it's the case here.)
The backport will conflict, so it has to be done manually.
(I do spend a decent amount of time merging PRs and am familiar with how it works :) )
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate the change in the commit message?
Why do you need a new venvwlauncher program? I guess that you can copy something from https://bugs.python.org/issue34977#msg330166 for example?
I understood that you made the following changes:
- Add a new venvlauncher.exe and venvwlauncherw.exe programs
- venv: Fix support for .pdb (debug symbols) files
| @@ -64,10 +64,11 @@ def create(self, env_dir): |
|---|
| self.system_site_packages = False |
| self.create_configuration(context) |
| self.setup_python(context) |
| if not self.upgrade: |
| self.setup_scripts(context) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"setup_python" on Windows no longer creates a "python.exe" file - those come from the scripts directory. So we need to set up scripts before installing pip, or else we can't install pip.
These were previously totally orthogonal steps on all platforms, and now there is a dependency that requires a certain ordering.
@vstinner I updated the PR description, as that is what will become the commit message when squashed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change backward compatible? For example, if you create a venv with Python 3.7.1 and upgrade Python to 3.7.2, what happens?
| @@ -0,0 +1,2 @@ |
|---|
| venv on Windows will now use a python.exe redirector rather than copying the |
| actual binaries from the base environment. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's not backwards compatible, but upgrading Python without recreating all your venvs wasn't backwards-compatible either. So it remains broken for that scenario, which would be broken without this change. But creating a venv with 3.7.2 and upgrading to 3.7.3 will be fine :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding the docs - I found a spot in the venv docs where it's worth noting the change (specifically that "setup_python" doesn't copy the binaries/DLLs anymore).
Technically it's not backwards compatible, but upgrading Python without recreating all your venvs wasn't backwards-compatible either. So it remains broken for that scenario, which would be broken without this change. But creating a venv with 3.7.2 and upgrading to 3.7.3 will be fine :)
Nice :-) It sounds like a bugfix. Should we backport this change up to Python 3.6?
We can backport it, but I don't need to. I think the 3.7 version should backport automatically though, so once we've got the two tricky versions merged we can see how easy it is.
A reminder: this weekend's 3.6.8rc1 is the release candidate for the final bugfix release for 3.6. So any changes like this that aren't in 3.6.8 are not going into 3.6 period. I'm not advocating a backport to 3.6, just pointing out that we need to realize that 3.6 is moving to security-fix-only mode shortly.
Considering we don't know the full extent to which people are doing unsupported things with venvs (we've seen some evidence that it's going on in the issue tracker from time to time), I'd rather not drop in the change to the last release of 3.6.
In 3.7 we can more easily justify telling people to change (if they were doing things like directly loading the DLLs) or determine whether it's a genuinely supported case (IMO, only python.exe and generated script launchers are "supported" here) and have time to fix it.
Can I clarify a point here (sorry, this is a drive-by comment, I've not read the PR in any detail). Does this mean that if I run the py.exe launcher with a venv active, I'll end up with three processes - py.exe which launches the new venv redirector, which then launches python.exe?
While I appreciate that dormant processes don't have a huge cost, I'm still bothered by the additional complexity, and the potential for confusion (if, for example, a user trying to kill a rogue process goes into Task Manager and is trying to make sense of the list of processes). I'm not saying that I'd block this PR because of an extra process, but I would like to be sure that there's no easier way of achieving the same result.
Yes, you'll end up with all three processes, though since they're all using job objects termination will Just Work. Unfortunately, there isn't really an easier way to enable this - launching an executable from within an app container is the only way to activate the container (and this is why the PR makes less sense when split out from the original one, which starts putting Python in an app container).
We could potentially make the py launcher start looking for the pyvenv.cfg itself, so it can bypass the venv redirector, but now the code complexity is getting high enough that I wouldn't want to block the rest of the improvements on that one case. Plus we know that the launcher already connects between subprocesses correctly, so the current change is safe while that one introduces greater risk overall.
Specifically, here's how it looks in Process Explorer. And I confirmed that killing any of them will close all of them correctly.
@vstinner We're holding up 3.7.2rc1 on your review - any further comments/concerns?
zooba added a commit that referenced this pull request