bpo-34977: Use venv redirector instead of original python.exe on Windows #11029
Conversation
|
@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 :) ) |
There was a problem hiding this comment.
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) | |||
There was a problem hiding this comment.
What is the purpose of this change?
There was a problem hiding this comment.
"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. |
| @@ -0,0 +1,2 @@ | |||
| venv on Windows will now use a python.exe redirector rather than copying the | |||
| actual binaries from the base environment. | |||
There was a problem hiding this comment.
You may add "As a result, the base Python install can be safely upgraded without breaking all venvs, and venv creation is significantly faster." (or explain it differently)
Moreover, this change might have side effect, so I suggest to document it in https://docs.python.org/dev/whatsnew/3.7.html#notable-changes-in-python-3-7-1 (add a new Python 3.7.2 section).
I'm not sure if it's worth it to add a ".. versionchanged:: 3.7.2" to document the change in https://docs.python.org/dev/library/venv.html as well.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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).
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 |
|
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 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 Any further comments/concerns? |
|
@vstinner We're holding up 3.7.2rc1 on your review - any further comments/concerns? |
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