I see that a new Python launcher implementation has been created in PC/launcher2.c by @steve.dower, and looking around, I also came across bpo-46566 / gh-90724. As the implementer of the original PC/launcher.c, it’s a shame I wasn’t kept in the loop on this. Is it normal practice these days to completely reimplement a committer’s code without informing them or discussing it with them? Or have I completely missed an approach about this?
I get that the requirement is to support PEP 514. But I’m wondering - what was the need for a complete rewrite, as opposed to adapting the existing implementation? And from reading the issue comments, I seem to see that @brettcannon has also created a separate launcher - is that right?
I maintain a variant of the PC/launcher.c launcher - which supports win32, amd64 and arm64 - and which is used in distlib (and through that in pip) for implementing installed scripts in venvs as .exe files. Some issues have been reported on built executables in venvs, which have resulted in some improvements to the launcher used in distlib. Probably some or all of these improvements need to come into the CPython launcher(s).
I also note that the new code doesn’t have the old (disabled by default) logic for allowing it to be prepended to a Python file. While that code was never to my knowledge in common use, and may not even have worked any more, I always had a hope that some day Python would ship with a launcher that tools could use to (for example) make a zipapp executable just by combining the 2 files.
I mentioned this on the issue, but it doesn’t seem to have happened, and I don’t see any comments suggesting that the idea had been rejected (at the time @steve.dower sait “But yeah, it can probably go in”, but that was the last I heard of it). This wouldn’t bother me so much if the existing launcher had just been modified, leaving the existing code present but maybe stale. But by making a complete rewrite, the code will now have to be reimplemented from scratch, making it harder if I ever do get round to dusting off my C skills to try to add this.
To be honest, I don’t think it’s helpful having multiple implementations of the launcher code - the logic (especially around correctly launching processes) is tricky and subtle, and if multiple people are implementing it, we’ll see bugs being encountered and fixed multiple times, possibly in different ways, causing yet more divergence.
Could we not ship as part of core Python, a launcher that supported all of the commonly needed forms (PEP 397/486, entry point launcher, “prepend to zipapp/script” launcher) - or at the very least, a C library that implemented the important logic? Even if we don’t have the resources to implement all of that right now (getting the bugs fixed for the 3.11 release is probably the priority right now) can we at least agree what will be part of the core launcher longer term? If it helps, I’d be willing to write a PEP formalising the spec.
Which is essentially the same as the entry point launcher ↩︎
The main differences between the py / pyw and the simple_launcher / distlib / pip launchers are:
Slightly different shebang processing - the former looks in the registry for installed Pythons to determine the interpreter for certain forms of shebang, and the latter uses a slightly different algorithm to determine the same (no looking in the registry, but searching the Windows PATH is supported).
The former supports launcher-specific command line arguments, e.g. Python version/“bitness” specifiers, whereas the latter does not - all arguments are assumed to be meant for the associated script.
The latter handles the <launcher_dir> prefix in a shebang to locate the Python interpreter relative to the launched executable’s directory.
The logic surrounding the launching of the child process should be common to both.
I use the simple_launcher in projects other than distlib, and expect at some point in the future to optionally enhance the shebang processing to be more POSIX-like for the /usr/bin/env XXX case, e.g. supporting -Dvar=value in shebangs just as the POSIX env program does. (This won’t affect the distlib launchers, which will remain backwards compatible with current behaviour in their shebang processing - I just thought I’d mention it while I’m here.)
I certainly can’t see a good reason for having multiple implementations. As you say, the interactions between the launcher, the spawned process, the idle state, the Job API etc. are tricky and subtle. I would say that the pip launchers get the widest exposure to real-world situations, and this is in fact where these tricky situations have cropped up in practice.
Just another thought - I believe there’s yet another form of the launcher, which is used as the “redirector” python.exe executable in the stdlib venv module. I’ve no idea about this, as I believe it’s considered an implementation detail and as such isn’t documented anywhere (that I know of) apart from the source code. There are some additional interactions there, I think - there’s a __PYVENV_LAUNCHER__ environment variable that’s referenced in site.py, but that’s about all I know
Right, the venvlauncher. I haven’t looked at it closely, but it seems to be using launcher.c. The __PYVENV_LAUNCHER__ variable was there from the original py / pyw launcher in Python 3.3, I think because of how macOS and framework builds worked (they used a stub executable). Not sure that environment variable’s needed nowadays (for recent macOS), except perhaps for backwards compatibility for any third-party code which relied on it.
The virtual environment launcher (implemented by “PC/launcher.c”) uses the “__PYVENV_LAUNCHER__” variable to pass the path of the launcher to Python. The environment variable is removed after it’s processed by Python’s startup code. It’s also passed to multiprocessing worker processes when running under a virtual environment. multiprocessing has to execute the base “python.exe” because an intermediating launcher process would interfere with manually duplicating handles between the main process and workers.
So we now have two implementations in Python core? launcher.c for the venv launcher, and launcher2.c for the standalone launcher (which has no mention of that variable). That seems like something that should be fixed…
Having separate implementations is not technically broken. However, maintaining separate implementations is a burden, particularly when changes are made to how Python gets spawned. The script entry-point launchers in distlib have been extensively updated in that regard.
Unlike the virtual environment launcher, the py launcher has no direct use for “__PYVENV_LAUNCHER__”. However, as a precautionary step to ensure Python starts correctly, the old implementation of the py launcher does delete this variable via SetEnvironmentVariableW(L"__PYVENV_LAUNCHER__", NULL). Since Python’s startup code also deletes the variable, in theory this precaution shouldn’t be necessary. That said, other code could implement the same trick as the multiprocessing module, but mistakenly leak the environment variable to other processes. It’s best to at least prevent that where we can, in the py launcher.
Sure, but it’s not the default approach unless there’s a good reason for those separate implementations - i.e. some justification for the ongoing maintenance burden. I’ve not seen any compelling reason, but perhaps @steve.dower can enlighten us.
There are other tidy-ups that can be done. The original launcher.c had a variant for building setuptools-style launchers (which expect a foo-script.py[w] adjacent to the launcher foo.exe), but I think this style is now deprecated in setuptools (IIUC, in favour of distlib’s approach) so ISTM the whole #if defined(SCRIPT_WRAPPER) stuff could perhaps be removed.
Considering the original launcher had three different variants selected by preprocessor variables, I don’t really think there’s any major difference in having them in separate files. Though my eventual goal is to have all the functionality in the new one and remove the older one - this is a transitional step. Right now, launcher.c is only used for the venv redirector scenario (though launcher2.c ought to be able to handle it too, and without needing a separate build), and launcher2.c is used for py.exe behaviour. Neither are used by CPython for pip-style script wrappers, and it wouldn’t give the behaviour that people expect anyway.
The main reason for making a new one is that the older one was too complicated to integrate support for the PEP 514 company/tag concept. It was going to require modifying the core interpreter data structure and everything that touches it, which is everything, but in a fairly scattered approach. We also needed to add additional detection logic for Store installs, as the “Lookaside” registry key isn’t reliable enough, and there were long-standing shebang bugs that were not easy to fix under the older design.
My apologies for embarking on the rewrite without checking in with you, Vinay. I don’t know that it’s “normal practice”, but I’ve certainly added a decent amount to the launcher in the past, which gave me the impression that it wasn’t being maintained elsewhere. At the same time, there was a bug, and it sat there for 2ish months before I posted that I was going to start work on a rewrite. If you’re going to claim ownership of the code, please keep a closer eye on the issues relating to that code
Brett’s launcher is written in Rust and targets non-Windows platforms. It has virtually none of the functionality we’d require on Windows (I considered extending it, but it would have been another complete rewrite and in a new language, which seemed too risky).
Yeah, as I said, we can add this back. Or you can take the older version of the launcher and use that. Either way, it’s not a built-in feature of a normal CPython install, so it wasn’t critical to have it in immediately. Still, I think I’d rather see that be part of a packaging tool rather than the core runtime, though. (Should also note that if you append a file to py.exe, it’ll break the code signature and Windows may refuse to launch it/antivirus may complain about it. So it’s not quite as simple as concatenating the data.)
We can, but it needs a volunteer to do it. I only volunteered to add support for the platforms we already had. It probably won’t be too hard to refactor launcher2.c into a static library that can be reused, as I deliberately kept detection, selection and execution separate.
I missed the PATH search originally, but it’s been added in (and tweaked silghtly, within the bounds of the documented behaviour, to be a bit more predictable).
Yes, this is still using launcher.c, because I wanted to reuse the process creation logic. The environment variable is only used in this case, and mimics how macOS handles venvs.
We haven’t actually changed the process creation code in a while, so I’d expect that’s going to stay fairly stable. I’ve seen the full spectrum of bug reports that could possibly arise around how this code works, and I’m yet to come up with a more user-friendly approach then “leave it alone so we don’t break everyone’s workarounds”.
And apart from that one function, none of the active code is actually shared. venvlauncher doesn’t use any of the registry search or shebang handling, py.exe doesn’t look for pyvenv.cfg, and neither are used for script wrappers. So there’s definitely dead code in both, but it’s very unlikely that we’ll get a spate of issues that affect both.
Once we (probably “I”) have a chance to verify that launcher2.c’s implementation of the venv launcher is reliable, we can switch to that and drop launcher.c entirely (apart from those who want to use it for a script wrapper, but none of those are in the core repo, so the source doesn’t need to be in there either).
Though honestly I’d prefer script wrappers to “know” where their Python executable is, so they don’t have to do lookups either. So again, we’d be sharing the process creation step, but none of the rest of the code. The exception would only be for something like pip’s thoughts about being a runtime-independent app (which I fully support), but those are so rare right now that building your own copy from source or otherwise extracting and modifying py.exe seem reasonable enough.
Except that the common bits of functionality are … well … common, and so would need to be maintained in multiple places. For example, the idle state handling / Job API setup code. It seems fine for the other functionality to be in #define-controlled parts of the file - that’s pretty commonplace in C source code, isn’t it?
I don’t see how. If you’re talking about identifying which interpreter to run, wouldn’t it just be a small-to-medium change to the logic for doing that part?
This seems rather hand-wavy to me. Haven’t you ended up with just a different C file that does what you think is needed?
Not sure why you would think that - I don’t expect to be the only committer to make changes to launcher.c, and AFAIK the Python source code all has multiple people working on it. It’s not being maintained elsewhere exactly for the py.exe / pyw.exe functionality - what would be the point of that?
I don’t recall any mention of a complete rewrite - please point me to where you mentioned that. Also note that I wasn’t made nosy on the original bpo issue (46566), and it’s not reasonable on a codebase as large as Python’s to expect someone to be on top of all issues and changes to the codebase all the time. And I’m not so proprietorial (“claim ownership”) as to think any of the other committers (or even other contributors) can’t work on code I originally authored, obviously. I regularly review and merge PRs from various people on the bits that I maintain.
Some people are paid to work on Python (perhaps part of their time) and others are purely volunteers. Sometimes volunteers can’t always respond as fast as others. Which specific bug are you referring to which sat around for 2 months? Was I nosied on it? Not saying I wasn’t, but I might have had other things going on at the time … I can’t say without knowing the specific issue you mean. I don’t think it is unreasonable of me to expect the courtesy of a direct message for a complete rewrite, but maybe I’m showing my age
So you can see how the approach you took has already resulted in extra work / duplication.
As @eryksun has already said, the distlib launchers have already addressed some issues around process creation which apparently launcher2.c doesn’t address (for example, better idle state tracking). Is that more stuff that needs duplicating?
If code is in #defined blocks for different use cases, that doesn’t necessarily make it “dead code” - for example, blocks for console and GUI code aren’t dead just because they occur in only one output executable. There are some things in launcher.c that are legacy, such as the SCRIPT_WRAPPER stuff I mentioned.
This is 150 lines out of a 2000+ line file. It could be refactored into a shared component easily enough, but it’s not as big a deal as you’re making it out to be.
It splits the existing version field (capped at 8 characters in launcher.c) into two orthogonal fields, with different ordering and comparison characteristics, as well as additional metadata fields for handling architecture selection. It also adds a new command line option to allow users to continue using the older options and get compatible behaviour, or to choose the richer behaviour when that’s what they want. So it’s quite a large change, and as the logic for finding an interpreter is the main purpose of this tool, “that part” is most of the file.
Sorry for that. I started adapting it and quickly found that I was adapting virtually everything in the file. But the decision that it would be easier to rewrite from the ground up (almost - I copy pasted the 150 lines for launching the executable) was gut-feel based on my own experience.
Exactly. But some parts of the core repo are being maintained elsewhere (e.g. platform, importlib_resources, distutils (R.I.P.)), and the people who maintain those tend to be fairly vocal about it, so when changes come up for those parts it’s more widely known that those maintainers should be involved. In comparison, the py.exe launcher doesn’t necessarily need “its maintainer” to be notified so they can mirror any changes.
You linked to Support -3.11-arm64 in py.exe launcher · Issue #90724 · python/cpython · GitHub. The first message is my summary of the need and a call for someone to work on it, the third message is me saying that I’m working on it, and the 4th is me saying that it’s become a rewrite and posting a link to my changes thus far. A few weeks later I posted that I was largely done and included a test build.
Indeed, you weren’t nosied. As I said above, I didn’t feel there was a need to explicitly add anyone for work on the launcher, so I didn’t, but the issue was definitely out there. There’s no nosy list for this tool directly, but you could add yourself to the codeowners file for it so that you are notified of any PRs. (Others are discussing how to replicate the experts index - all I’ll say is that it’s a shame we lost it, and hopefully GitHub being more accessible will make up for it, but I’m still skeptical )
I mean, a rewrite is a lot of extra work But I didn’t duplicate the code for it, it’s brand new, more secure, and more reliable than what was there before. And I could easily add it into the existing flow as a single, clearly named function call.
Maybe. I’m not tracking changes that go into distlib, so if they aren’t also being reported to the core CPython repo, they aren’t going to be in the core CPython repo. I haven’t heard of anything major that needs fixing in process launch though (at least, nothing that won’t break enough other users that we just can’t do it).
Yes, this is what I meant. There’s also code in launcher2.c that won’t be used (yet), because it’s intended for when we’re able to use it to replace the venv launcher. I don’t think it’s a big deal.
If I’m correct, a substantial part of that 200+ line file is the PEP 514 registry checking code, that locates Python installations. I have, on more than one occasion, wanted to be able to use that (and its predecessor, the code in launcher.c that did the same for simpler version specs like -3.9) elsewhere. Both in C code and in Python code. But because it’s embedded non-reusably in the launcher, I’d have to rewrite it. Often, I’ve simply not bothered and ended up with a less user-friendly interface as a result.
While I guess it’s too late now, I do feel that the rewrite was a huge lost opportunity to collect use cases like these, and write something that satisfied them. Whether that was a “multi-purpose” launcher, a C library that could be used from different tools, or even just the code being structured as separate C files that could be included in other projects, I honestly don’t care that much. But a single, monolithic file 2000+ lines long is emphatically not reusable, and it’s a shame that miscommunication meant we missed that opportunity.
It’s definitely a judgement call, and I mostly agree that there’s very few people who take an interest in launcher changes (and even fewer who actually make them). But larger changes do tend to impact a lot of people, and to be honest, have a track record of coming as a surprise to people (I remember the venv launcher changes causing a lot of issues when they were first introduced). I don’t have a particularly good answer, but I’d suggest that “err on the side of more communication rather than less” would be a good rule of thumb in the light of the history here.
One specific point on communication here. I think we really should have included the rewrite in the Python 3.11 release announcements. I don’t know when the new launcher was introduced, but I think it deserved a prominent notice saying that we’d rewritten the launcher, and we’d appreciate people testing the new code to make sure it worked the same as the old one. I know I only realised the code was new when I tested the next to last 3.11 beta and found it didn’t work. I can’t promise I’d have tested the alphas or earlier betas if I’d known the launcher had been rewritten, but I’d hope someone would have (and the bug certainly wouldn’t have gone unnoticed).
Just to be clear, though, I understand that maintaining a big chunk of C code like the original launcher is a lot of work. And the fact that you’re actively working on it is much appreciated. It’s awfully easy for bystanders like myself to give unwanted advice after the fact. Please treat the above as “how can we learn for the future” rather than “this is what you did wrong” - it’s really not intended in that way.
The startup time of all processes is much slower on Windows That’s the biggest point of perf difference with other platforms right now. Unfortunately, it’s only really fixable at the OS level (and I’m pushing the right people to work on it, but I don’t have much power in that space).
It gets worse with the venvlauncher because now there are three processes being launched to get it to start. The best way to shorten that would be for the script launcher to detect the pyvenv.cfg and run the base executable directly, passing the (undocumented, unsupported, etc.) __PYVENV_LAUNCHER__ secret variable, which might require documenting and supporting the secret variable. But since we’ve changed the semantics of that variable in the past, it would be nice to keep the freedom to change it again if needed, so I’d prefer launchers not rely too much on internal details like that (or we might even be able to replace the venv’s python.exe with a more efficient link, if one of the other teams I’m leaning on does what I’d like them to do…)
Good. It’s not meant to be set in any environment, it should only exist for the brief period between the venv python.exe launching the real python.exe and calculating its initial sys.path. No Python code should ever run during that period, and python.exe will un-set the value after it’s used it.
The actual process creation and job assignment via CreateProcessW() and AssignProcessToJobObject() hasn’t changed, but there have been many changes to how the distlib launchers spawn Python:
The launcher’s standard handles are ignored if the handle values in the process startup info are flagged as hotkey data or a monitor handle.
Otherwise the launcher’s standard handles are made inheritable directly instead of duplicating them as inheritable. This supports inheriting C runtime file descriptors properly via the startup info’s lpReserved2 field.
The launcher’s stdin and stdout streams, inherited file descriptors, and standard handles are carefully closed. This primarily concerns pipes. File handles in the launcher shouldn’t keep pipes open. It also concerns the sharing mode of open files, particularly if an open doesn’t share delete access.
The launcher’s current work directory is changed to the user’s temp directory. The launcher shouldn’t prevent deleting the initial working directory.
The GUI launcher proxies the input idle state of the child process. It calls WaitForInputIdle() to wait for the child to signal its input idle state. If the wait succeeds, the launcher creates a hidden message window and peeks messages in order to signal its own input idle state. That way the launcher’s parent can rely on calling WaitForInputIdle() on the launcher process.
On a related note, the launcher’s console control handler was changed to handle CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT, and CTRL_SHUTDOWN_EVENT. The new implementation is flawed, however. It waits on the child forever in these cases. Windows calls console control handlers sequentially, not in parallel, in which case the launcher’s handler hangs for the system’s hung-process or shutdown timeout, and Python’s handler doesn’t get called. There’s a better NEW_LOGIC implementation, which for some reason is not currently used. This version removes the child from the launcher’s job object, allowing the child to outlive the launcher. Decoupling from the launcher is fine because no matter what these console events require the process to either exit on its own or get forcefully terminated by the system.
I thought I’d done this, but it turns out I only did a NEWS entry (for 3.11.0a7). As it is, I’m pretty happy with the amount of feedback I’ve gotten, but will add a more prominent note before RC.
Yeah, it would be a nice library to have for those occasions - the VS and VS Code teams would have used it, for sure.
I don’t particularly want to maintain it as a public API though, or at least no more API than py --list-paths (possibly with better formatted output). There’d be too much compatibility to have to preserve, and it’s already independent enough from the rest of the core that it could live alone.
So I’m not enthusiastic about making it a public library, but I’m not opposed to having someone refactor the monolithic launcher2.c into more easily copyable files for certain parts of the process (provided that person isn’t overly enthusiastic about turning 2000 lines into 5000 lines… I’d rather not have that much spaghetti to wade through when trying to fix bugs).