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
py.exe should unset the __PYVENV_LAUNCHER__ environment variable #79992
Comments
In 3.7.2 on Windows, venv now uses a redirecting launcher 'script' for python[w].exe. This replaces (on Windows only) the setup requirement to copy or symlink the interpreter binaries (i.e. python[w].exe, python37.dll, and vcruntime140.dll). Apparently this is required to be able to install Python as a Windows Store app. I haven't experimented with it yet, so I'll just accept that as a given. I'm curious to find out whether using symlinks would still work, and even if it doesn't work for the app installation, whether we could still support that option for desktop installations. I've used The new launcher reads pyvenv.cfg to locate and execute the real python.exe. Since the process image is no longer in the virtual environment's "Scripts" directory (which has various consequences), we need a way to communicate the launcher's path to make Python use the virtual environment. A -X command-line option could work, but then all packages and tools that spawn worker processes, such as multiprocessing, would need to be updated to look for this option in sys._xoptions and propagate it. Instead the launcher sets a special "__PYVENV_LAUNCHER__" environment variable. This is reasonable because processes are usually created with a copy of the parent's environment. Some environment variables may be added or modified, but it's rare for a child process to get a completely new environment. (One example of the latter would be creating a process that runs as a different user.) An oversight in the current ecosystem is that py.exe and the distlib entry-point launchers do not unset "__PYVENV_LAUNCHER__". Thus, when executing a script from a virtual environment (e.g. either directly via py.exe or via the .py file association), it will mistakenly be pinned into the virtual environment if it runs in Python 3.7. Similarly, pip.exe for an installed Python 3.7 will mistakenly install into the virtual environment. However, the latter is out of scope here since the entry-point launchers are in distlib. It's also a problem if we run the fully-qualified path for an installed Python 3.7, e.g. from shutil.which('python'). We can't automatically address this since it's exactly the reason "__PYVENV_LAUNCHER__" exists. We have to know to manually unset "__PYVENV_LAUNCHER__" in the environment that's passed to the child. This should be documented somewhere -- maybe in the venv docs. It's not a problem if a script runs unqualified "python.exe" since the final result is usually the same, but for different reasons. With the launcher, it's locked down by inheriting "__PYVENV_LAUNCHER__". With the previous design, the application directory was the virtual environment's "Scripts" directory. Unqualified "python.exe" was thus pinned for CreateProcessW, which checks the application directory first. It's also not a problem if we run sys.executable, since previously that was the virtual environment's executable. (In 3.7.2, sys.executable gets set to the virtual environment's launcher, but that breaks multiprocessing. See bpo-35797.) |
FWIW, a symlink should be able to launch the Store Python - the problem with the old approach was it never tried to launch the original Python, but only load it's DLLs which won't work (maybe there's a way to enable the app context, but that would still require a new redirector and so wasn't any simpler than a direct launch). Otherwise, I agree with this analysis and proposal. Just needs a patch! |
It's worth noting the consequences of changing the application directory (i.e. the "%APPDIR%" dynamic variable from GetEnvironmentVariableW). In practice maybe no scripts will be affected by this change. I'm just noting potential problems. Changing the application directory will break scripts that depend on the environment's "Scripts" directory being in one of the above search paths. It will also break scripts that depend on it being the first directory searched in order to shadow system files that aren't otherwise reserved. We can lessen the impact of this change by prepending the "Scripts" directory to PATH. However, some directories will unavoidably have precedence over it now. Specifically, for the CreateProcessW EXE search, it's any of the following directories:
Also, applications that call SetDefaultDllDirectories get a secure DLL search path that can only reference "%APPDIR%", "%SystemRoot%\System32", and directories added via AddDllDirectory or SetDllDirectory. They no longer use "%CD%", "%SystemRoot%\System", "%SystemRoot%", or PATH. An affected script will have to manually add the "Scripts" directory to the DLL search path via AddDllDirectory or SetDllDirectory. Scripts that wish to restrict searches to PATH should use shutil.which(). It also works more predictably for relative filenames. In particular, a relative filename that has a path separator in it (e.g. r"eggs\spam.txt") is only searched relative to the process current directory. In contrast, searches based on RtlDosSearchPath_Ustr handle all relative filenames, whether or not they contain a path separator, by trying every directory in the search path (e.g. it will look for r"%APPDIR%\eggs\spam.txt", and so on). [1] If enabled by the call flags, searching for a relative filename |
I'm not particularly happy with the statement that "However, the latter is out of scope here since the entry-point launchers are in distlib." If Python is changing the semantics of __PYVENV_LAUNCHER__ in such a way that it breaks 3rd party code, it seems to me that this is a bug, which should be addressed by reverting or fixing the change so that the 3rd party code does not need to change. Either that, or the change is publicised as a backward compatibility break, so that tools are given a chance to update. Having said all of that, I appreciate that __PYVENV_LAUNCHER__ is not exactly a public API, so there should be some leeway here. But I think that whatever solution we propose here needs to address how we manage the change required of third parties. I still need to fully read and understand this discussion - I've not really followed any of the work around publishing Python as a Store application, taking the view that I won't use it myself, so "it won't affect me". But now I'm starting to worry about whether I'll see pip bug reports being filed because pip's installing things in the wrong places ("Similarly, pip.exe for an installed Python 3.7 will mistakenly install into the virtual environment.") If that sort of thing does happen, it harms our credibility and we need to be able to clearky explain what's going on, and how to fix it. Can I clarify 2 things:
(Again, apologies if I've misunderstood something here - there's quite a lot of technical detail that I haven't had time to understand yet). |
It might be worth adding a summary of that to the porting notes, but I think the actual impact will be minimal. Launching processes by relative path has been a bad decision for long enough now that I'd expect it to be rare, and the Scripts directory layout being slightly different helps. The launcher scripts used by pip also changed somewhat recently (between 12.x and 18.x IIRC) with something along these lines without causing significant issues. Ultimately, there isn't much of an alternative. Those who have been following good practices and supported patterns in their code will be okay, and those who have not (e.g. Numpy putting DLLs on PATH) will be bitten sooner or later anyway. So let's suppress the variable in the launcher to deal with that particular issue. It's fairly minor compared to the multiprocessing one. For Paul: Yes, there's a chance of pip issues. This is why I sent you all a heads-up email before anything was released. But pip is certainly the best tested package and I've even been doing pip development using the Store package and a venv without trouble. Everything here is severe edge cases. Also, the __PYVENV_LAUNCHER__ variable is totally new - previously it only ever existed on macOS (since some of the same launch restrictions apply there). It's not the same as the VIRTUALENV variable that you added support to py.exe to detect. That one is fine. |
Steve - thanks for the clarification. If it's only affecting launching with relative paths, then agreed it's minor (although I do recall recent discussions somewhere about using launchers with relative paths - that use case may need checking, but we can deal with that when it comes up). And I hadn't appreciated that __PYVENV_LAUNCHER__ was MACOS only before now. So again, thanks forthe reassurance there. |
A bug occurs when running an entry-point script, such as pip.exe, for a system- or user-installed Python 3.7.2+ from the context of a virtual environment for 3.7.2+. The script executable redirects to run |
But if I understood what Steve said, only if path\to\python.exe is a *relative* pathname? |
I believe Steve is referring to behavior changes for applications that relied on a virtual environment's "Scripts" directory always being in the EXE and default DLL and generic search paths because it was the application directory (i.e. "%APPDIR%"), which I had discussed in a follow-up note. I suggested that this could be mitigated by having the new venv launchers also prepend their directory to PATH. It wouldn't help in all cases, but it's the best we can do. The issue with __PYVENV_LAUNCHER__ and the py.exe and entry-point launchers is unrelated to relative paths. Perhaps it will clarify the situation to show how this variable is actually used in PC/getpathp.c. It's a pretty simple change: /* The launcher may need to force the executable path to a
* different environment, so override it here. */
pyvenv_launcher = _wgetenv(L"__PYVENV_LAUNCHER__");
if (pyvenv_launcher && pyvenv_launcher[0]) {
wcscpy_s(program_full_path, MAXPATHLEN+1, pyvenv_launcher);
} else if (!GetModuleFileNameW(NULL, program_full_path, MAXPATHLEN)) {
/* GetModuleFileName should never fail when passed NULL */
return _Py_INIT_ERR("Cannot determine program path");
} https://github.com/python/cpython/blob/v3.7.2/PC/getpathp.c#L535 |
I think that snippet may have to change to accommodate the multiprocessing issue, where we need to stop replacing sys.executable with the venv launcher and have it point to the base python.exe (and then anyone launching sys.executable needs to preserve __PYVENV_LAUNCHER__, which is the default behavior). Having done a code scan, there are quite a few places where we explicitly choose __PYVENV_LAUNCHER__ on macOS over sys.executable, so I guess we need to do those on Windows now as well. But that's a discussion for bpo-35797 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: