| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks congruent to me
Sorry, something went wrong.
|
Looks like a solid change, but probably simpler to just create a python3t.vcxproj and put the python3t.dll in the default location, which will remove a lot of the modifications (and can simplify things you didn't even add, since we'll just build it twice and python3.dll and python3t.dll are now both constants). We rearrange output files as part of building the installer, so that's where the directory should come into it. 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. |
Sorry, something went wrong.
|
I've tried to build the installer using your branch and get the same error as CI: error CNDL0027: The File/@Name attribute's value, 'abi3t-compat\python3t.dll', is not a valid long name because it contains illegal characters. Changing to <Component Id="python_abi3tcompat.dll" Directory="abi3t_compat" Guid="*">
<File Id="python_abi3tcompat.dll" Name="python$(var.MajorVersionNumber)t.dll" KeyPath="yes" />
</Component>
and adding the directory to Tools/msi/common.wxs: <Fragment>
<DirectoryRef Id="InstallDirectory">
<Directory Id="abi3t_compat" Name="abi3t-compat" />
</DirectoryRef>
</Fragment>
Note Id="abi3t_compat" is chosen, because - is not allowed in an identifier. 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 Source in <File Id="python_abi3tcompat.dll" Name="python$(var.MajorVersionNumber)t.dll" KeyPath="yes" /> But this is all new to me as well ... |
Sorry, something went wrong.
|
Adding <LinkerBindInputPaths Include="$(BuildPath)">
<BindName>build</BindName>
</LinkerBindInputPaths>
to Tools\msi\msi.props and using <File Id="python_abi3tcompat.dll" Name="python$(var.MajorVersionNumber)t.dll" Source="!(bindpath.build)\abi3t-compat\python$(var.MajorVersionNumber)t.dll" KeyPath="yes" /> did work for me using your current layout. |
Sorry, something went wrong.
|
And IMHO we should bundle python3t_d.dll in the installer as well. |
Sorry, something went wrong.
This is the way.
In two more weeks we get to never touch this MSI again :) |
Sorry, something went wrong.
|
Thanks for the pointers! This is how far I got in another day: python3dll and python3tdll with fixed names. Build FAILED. "C:\Users\encukou\dev\cpython\Tools\msi\bundle\snapshot.wixproj" (default target) (1) -> "C:\Users\encukou\dev\cpython\Tools\msi\doc\doc.wixproj" (Build target) (7) -> (Link target) -> C:\Users\encukou\dev\cpython\Tools\msi\doc\doc.wxs(12): error LGHT0094: Unresolved reference to symbol 'WixComponen tGroup:doc_html' in section 'Product:*'. [C:\Users\encukou\dev\cpython\Tools\msi\doc\doc.wixproj] C:\Users\encukou\dev\cpython\Tools\msi\doc\doc.wxs(27): error LGHT0094: Unresolved reference to symbol 'WixComponen tGroup:doc_html' in section 'Product:*'. [C:\Users\encukou\dev\cpython\Tools\msi\doc\doc.wixproj] I'll continue tomorrow. |
Sorry, something went wrong.
|
I think you need to build docs manually for the installer? I remember cutting a corner in the msi/build.bat that I regretted, but solved it by basically never using that script again... buildrelease.bat should do it all. Couple of other comments/concerns coming on the changes. |
Sorry, something went wrong.
|
I built an installer based on this branch, but am seeing issues caused by the python3t.dll produced being self-referential: C:\Users\goldbaum\AppData\Local\Programs\Python\Python315>dumpbin /exports abi3t-compat\python3t.dll
Microsoft (R) COFF/PE Dumper Version 14.44.35226.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file abi3t-compat\python3t.dll
File Type: DLL
Section contains the following exports for python3t.dll
00000000 characteristics
FFFFFFFF time date stamp
0.00 version
1 ordinal base
948 number of functions
948 number of names
ordinal hint RVA name
1 0 PyABIInfo_Check (forwarded to python3t.PyABIInfo_Check)
2 1 PyAIter_Check (forwarded to python3t.PyAIter_Check)
3 2 PyArg_Parse (forwarded to python3t.PyArg_Parse)
4 3 PyArg_ParseTuple (forwarded to python3t.PyArg_ParseTuple)
5 4 PyArg_ParseTupleAndKeywords (forwarded to python3t.PyArg_ParseTupleAndKeywords)
In this output we should be seeing e.g. PyABIInfo_Check re-exporting from python315.dll, but it's re-exporting from itself. |
Sorry, something went wrong.
Yeah, all the CI jobs pass --doc, so building without it seems to have broken at some point. 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. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
| } | ||
| #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).
Sorry, something went wrong.
This will be a typo in the script that generates the .def file. I missed it when reviewing, but I assume it's just a wrong variable that'll be easy to spot (for Petr, who has worked on this generator a lot). |
Sorry, something went wrong.
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.
Sorry, something went wrong.
| <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" /> |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks! I'll start today with these suggestions.
Sorry, something went wrong.
Co-authored-by: Steve Dower <steve.dower@microsoft.com> Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Documentation build overview1 file changed ± download.html |
Sorry, something went wrong.
|
I looked at the error message in the installer build and immediately thought "that'll be @hugovk's fault" and one git log Doc\make.bat later I see I was right 😆 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 😉 ) |
Sorry, something went wrong.
|
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 make.bat change, so docs' deps are installed from requirements.txt instead of pylock.toml. 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? |
Sorry, something went wrong.
|
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 main that somehow broke my WIP branch for PyO3 PEP 803 support. Let me try again with the branch I got to successfully work yesterday and see what's different in that version of things. |
Sorry, something went wrong.
|
I think I see the issue - the free-threaded python3t.dll is clobbering the python3t.dll that gets produced in the first build pass for the GIL-enabled build, so the abi3t_compat\python3t.dll that ends up installed re-exports from the wrong dll, and we get crashes because sizeof(PyObject) is wrong: C:\Users\goldbaum\AppData\Local\Programs\Python\Python315>dumpbin /exports abi3t-compat\python3t.dll
Microsoft (R) COFF/PE Dumper Version 14.44.35226.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file abi3t-compat\python3t.dll
File Type: DLL
Section contains the following exports for python3t.dll
00000000 characteristics
FFFFFFFF time date stamp
0.00 version
1 ordinal base
948 number of functions
948 number of names
ordinal hint RVA name
1 0 PyABIInfo_Check (forwarded to python315t.PyABIInfo_Check)
2 1 PyAIter_Check (forwarded to python315t.PyAIter_Check)
3 2 PyArg_Parse (forwarded to python315t.PyArg_Parse)
4 3 PyArg_ParseTuple (forwarded to python315t.PyArg_ParseTuple)
5 4 PyArg_ParseTupleAndKeywords (forwarded to python315t.PyArg_ParseTupleAndKeywords)
All the forwards above should be from python315, not python315t. |
Sorry, something went wrong.
Probably we need to audit all our uses of pip (including in release-tools) and get them all pinned to the same version that Dependabot can find and update. I don't really want pip install --update pip everywhere, but equally, I don't want it to be pinned in multiple different places. 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. |
Sorry, something went wrong.
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, something went wrong.
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 .\Tools\msi\build.bat -x64 --doc in the root of the CPython repository with IncludeFreethreaded=true set in the shell environment. Should I be doing something else instead? |
Sorry, something went wrong.
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 python3t-for-abi3.dll or python3-for-abi3t.dll1 that is useful even after the rename. Footnotes
|
Sorry, something went wrong.
|
It's also possible to do a rename in the PC/layout script, so we don't need twisted build logic. The only thing that would be missing is linking to python3[t]_for_abi3[t].dll as python3[t].dll straight out of the build directory, which impacts cross-build testing (mostly just you two, I expect). |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I need to leave my Windows box for today; this is where I am now: building the "foreign" DLL in the abi3t-compat directory. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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 abi3t on all runtimes are aware that it's coming (or we get it there before they start work), nobody else should care. 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! |
Sorry, something went wrong.
|
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 python.bat will still reflect the most recent build, and there should only be a couple of tests that explicitly need the directory. Our real builds already build separately, so there shouldn't be too much to update there, and the MSI build probably only needs an extra bind path for it to work fine, which I'll try tomorrow and see if it's less trouble than what I was fighting with today. |
Sorry, something went wrong.
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
| Back | FazBrowse Home | New Git URL |
Per discussion in #146636 (comment), this adds abi3t-compat\python3.dll to Windows builds.
And to the WiX installer, though I haven't managed to test this.