Skip to content
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

Enable delvewheel in CIBW on Windows #179

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

ksunden
Copy link
Contributor

@ksunden ksunden commented Jul 3, 2024

This will package the MSVC runtime DLL in a way that works reliably and does not require external C dependencies to be installed when installing the wheel.

This is what we do in Matplotlib, and we have had a handful of reports of failure to import caused by _cext within kiwisolver, this should resolve those.

Closes #178

I haven't tried with PyInstaller, but possibly related to #168 as well.

See also #173

@MatthieuDartiailh
Copy link
Member

Thanks for taking the time to do this. I will merge and plan to make a new release ASAP.

@MatthieuDartiailh MatthieuDartiailh merged commit 394e5d0 into nucleic:main Jul 4, 2024
@MatthieuDartiailh
Copy link
Member

I did a test run https://github.com/nucleic/kiwi/actions/runs/9789785072/job/27030113241 which failed to find the right dll. I assume I could remove the option older redistributable version but I am curious if you have seen such issue for matplotlib.

@ksunden
Copy link
Contributor Author

ksunden commented Jul 4, 2024

Hmmm... most likely related to Windows ARM builds specifically, which we don't do in Matplotlib, so have not had any issues. Nothing obvious on initial look, but can look into it more later.

If that is the case, worst case is to take it out of the ARM build (which is separate section here, so not hard to do)

@MatthieuDartiailh
Copy link
Member

To maximize compatibility I currently disable linking against VC2014_1. The runner may be missing the old dll no ?

@ksunden
Copy link
Contributor Author

ksunden commented Jul 4, 2024

adang1345/delvewheel#44 (comment)

Looks like I was right about the arm being the problem (since it's cross compiling the PATH is picking up x64 dlls)

@ksunden
Copy link
Contributor Author

ksunden commented Aug 14, 2024

Well, as it turns out, shortly after I made this PR, we discovered some problems with msvcp*.dll... After a fair bit of searching for solutions, and consultation with Microsoft Engineers, we decided to actually pass the proper flags to statically link msvcp, removing both the need to have it installed separately or problems with mixed versions.

Here is what we ended up doing:

matplotlib/matplotlib#28687

We have not removed delvewheel yet, though it only affects pypy now, since CPython bundles the vcruntime*.dll, which we still dynamically link to, but does not have to be in wheels.

MatthieuDartiailh added a commit that referenced this pull request Sep 3, 2024
* update copyright

* add 3.13 support and drop 3.7 support

* cis: test on 3.13

* prepare to build wheels on 3.13

* update all copyrights

* cis: stop testing on 3.7 and test on pypy 3.9 and 3.10

* cis: update linking strategy on windows

cf #179 (comment)

* exclude more setuptools versions when on pypy

* remove the option to disable FH4

* fix copyright update

* update release notes

* put a bound on the setuptools version used on Pypy

* off by one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants