gh-148690: Build abi3t-compat\python3.dll on Windows#148912
gh-148690: Build abi3t-compat\python3.dll on Windows#148912encukou wants to merge 14 commits intopython:mainfrom
Conversation
|
Looks like a solid change, but probably simpler to just create a It won't work so nicely for dev/in-tree builds, but I'm not so concerned about that. We probably need to choose an entirely new output directory for free-threaded builds, but that's going to break all sorts of stuff that assumes directory names in a build, so probably best to just say "ABI3 will get weird in dev builds if you don't clean before switching kind of build". I expect anyone trying to work on both simultaneously has separate clones anyway. |
|
I've tried to build the installer using your branch and get the same error as CI: Changing to and adding the directory to Note As Steve mentions
then the above did work for me (when manually copying the dll just to try it out). Otherwise, you'd have to add a But this is all new to me as well ... |
|
Adding to did work for me using your current layout. |
|
And IMHO we should bundle |
This is the way.
In two more weeks we get to never touch this MSI again :) |
|
Thanks for the pointers! This is how far I got in another day: I'll continue tomorrow. |
|
I think you need to build docs manually for the installer? I remember cutting a corner in the Couple of other comments/concerns coming on the changes. |
|
I built an installer based on this branch, but am seeing issues caused by the In this output we should be seeing e.g. |
Yeah, all the CI jobs pass I have a (claude-suggested) fix for the issue I described in my last comment that I'm testing out. Sadly each build takes about a half hour end-to-end, including the Maturin and PyO3 builds needed in the stable-abi-testing repo to run end-to-end tests. |
zooba
left a comment
There was a problem hiding this comment.
The other place that will need changing is the script under PC/layout, which is the "new" install creator (essentially it's the make install command). That just has to copy in the right file if it's been built, without the subdirectory.
| } | ||
| #endif | ||
|
|
||
| wcscpy(p + 1, PY3_DLLNAME); |
There was a problem hiding this comment.
We also want to search for the compatibility DLL name without the extra directory, but only if the one with the directory doesn't exist. That will be the long-term (post 3.15) path, and also used by the PyManager package straight away (which doesn't do overlapping installs).
This will be a typo in the script that generates the |
ngoldbaum
left a comment
There was a problem hiding this comment.
With the suggested changes I'm able to run the tests in the stable-abi-testing repo on the GIL-enabled and free-threaded builds and don't see any issues. There are two failures on the GIL-enabled build for the meson-python tests, but fixing those will require updating @mgorny's meson-python branch we use in the stable-abi-testing repo or adjusting the tests in the stable-abi-testing repo.
I also manually verified using my maturin and PyO3 branches for abi3t support that a wheel built with either build is installable and importable by both builds.
Not sure how well these suggestions play with @zooba's suggestions to leave things unconditional. Hopefully there's a straightforward way to integrate my suggestions with his.
| <Fragment> | ||
| <ComponentGroup Id="core_dll"> | ||
| <Component Id="python_abi3tcompat.dll" Directory="abi3t_compat" Guid="*"> | ||
| <File Id="python_abi3tcompat.dll" Name="python$(var.MajorVersionNumber)t.dll" Source="python$(var.MajorVersionNumber)t.dll" KeyPath="yes" /> |
There was a problem hiding this comment.
| <File Id="python_abi3tcompat.dll" Name="python$(var.MajorVersionNumber)t.dll" Source="python$(var.MajorVersionNumber)t.dll" KeyPath="yes" /> | |
| <File Id="python_abi3tcompat.dll" Name="python$(var.MajorVersionNumber)t.dll" Source="abi3t-compat\python$(var.MajorVersionNumber)t.dll" KeyPath="yes" /> |
encukou
left a comment
There was a problem hiding this comment.
Thanks! I'll start today with these suggestions.
Co-authored-by: Steve Dower <steve.dower@microsoft.com> Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Documentation build overview
|
|
I looked at the error message in the installer build and immediately thought "that'll be @hugovk's fault" and one Maybe we shouldn't be assuming that every dependency (pip, in this case) is forcibly updated on every build? That's actually bad policy (you should use a lockfile to pin to a known good version, which might be a little older 😉 ) |
|
It looks like currently we're using whatever pip is in the CI runner? Short term, we can either require newer pip or revert the And/or we can start pinning pip to a certain version. We should then decide when/how to upgrade that. Easiest would be via Dependabot. Which do you prefer? |
|
I tried re-doing my testing from yesterday but hit a segfault. See this crash report from WinDbg using the stable-abi-testing maturin/rust example: https://gist.github.com/ngoldbaum/7f277b39f8c054959c47a96e4ec5c228 Or this crash using the setuptools/c example: https://gist.github.com/ngoldbaum/fa31157e5839c8425bdd6626d0d8ef3d It's possible that this is caused by other changes that were recently merged into CPython |
|
I think I see the issue - the free-threaded All the forwards above should be from |
Probably we need to audit all our uses of pip (including in Personally, I'd rather just revert the lockfile change for now, since I prefer not to mandate our users/contributors update their tools too often (see also, not mandating the latest Visual Studio or compilers). Once it's likely that they're already on a pip with support, then we can start using it. |
Yes, the builds really need to go into separate directories (or use separate checkouts). That hasn't been done yet, and I don't want to start it here. Can you update that test to clone twice and build them separately? |
Sorry, I'm not sure what this means. Do you mean build CPython twice with two clones of the repository? For some extra context, I'm generating my installer by running |
Yes. One time with GIL, one time without. Combining them into a single installer is only a temporary thing - those installer sources get deleted when we branch for 3.16. But now I also see that we're going to have the same issue with the release despite us already building them in separate directories, which means the MSI build does require additional changes to handle it. I think using a different filename and renaming during installation (bring back the name attribute I just suggested removing) is the best overall way (least likely to break code signing, etc.). I need to give more thought to which file is built to a different name, but one key aspect is that the original name will be embedded in the file as version metadata (with no runtime impact), so it will be discoverable after install - this is very useful for debugging, so ideally we use a filename like Footnotes
|
|
It's also possible to do a rename in the |
Alas you're the only one who can do that, and deadline (for the implementation) is Tuesday :( I'll keep trying, but with 30-minute builds it's hard to poke around. |
|
I need to leave my Windows box for today; this is where I am now: building the "foreign" DLL in the |
|
Everything is working in my local build of the installer using this PR with my last comment applied. Just in case there's further work that needs to happen, I think it'd be helpful to clarify exactly what needs to be done by the time the beta1 freeze starts. Is all we need a finalized file layout? If so and if the current state of things isn't acceptable, is it OK to merge a PR that implements just the final layout, but doesn't necessarily get all the build details correct? That would allow us to fix this in beta2 and make finalizing everything here less of a fire drill. Given that this is an entirely new dll in a new path, I doubt it matters that we'd be changing it after beta1? But I'm also definitely not a Windows build expert. I will also be traveling this weekend and will not have easy access to my amd64 Windows development machine. I get back Monday evening US time. |
Or we get RM permission to change the layout after feature freeze, but as you say, a net-new DLL isn't going to be too controversial. Provided the tools that are going to design build systems around using I should have some time tonight to poke at this a bit, so I'll see if I can wrap it up. Thanks for getting things this far @encukou! |
|
Not finished, but I'm now inclined to say we should just stop trying to build both FT/non-FT into the same output directory. Keeping things separate the whole way through avoids so much mess trying to deal with MSBuild and WiX, and there are some really messy behaviours around import libraries when you try to do any of the renaming tricks I'd hoped would make it simpler. Updating the default output directories may be more disruptive, but it's only going to be for the non-default FT build, so hopefully it's not too surprising. The generated |
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Per discussion in #146636 (comment), this adds
abi3t-compat\python3.dllto Windows builds.And to the WiX installer, though I haven't managed to test this.