10000 BLD: Include MSVCP140 runtime statically by QuLogic · Pull Request #28687 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

BLD: Include MSVCP140 runtime statically #28687

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

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

QuLogic
Copy link
Member
@QuLogic QuLogic commented Aug 8, 2024

PR summary

This may prevent conflicts with other wheels that use the runtime at a different version, and may be an alternative to #28679.

PR checklist

@QuLogic QuLogic added this to the v3.9.2 milestone Aug 8, 2024
@QuLogic QuLogic added the CI: Run cibuildwheel Run wheel building tests on a PR label Aug 8, 2024
Copy link
Member
@ianthomas23 ianthomas23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the Windows wheels from https://github.com/matplotlib/matplotlib/actions/runs/10314253959 locally against both a ContourPy clean install and nightly wheels (without any use of delvewheel) and they pass the full test suite and the simple import-to-exception reproducer. I've also tested them in github runners at https://github.com/ianthomas23/mpl-test/actions/runs/10318743303; it is the last 3 tests that use the new statically-linked mpl wheels.

On that basis I would happy to see this merged and included in a new release and nightly wheels whenever that is possible.

@ksunden
Copy link
Member
ksunden commented Aug 9, 2024

For the record, this appears to make delvewheel entirely unnecessary: It does not appear to do anything, either bundling dlls nor editing __init__.py files.

I'm wondering if we go this route do we wish to remove that step entirely as it is a no-op, or do we wish to keep it as a "if we ever need it again, we want it to just work and not have to remember we need it"?

Personally I lean towards removing the thing we are not using, but willing to be convinced otherwise.

This should prevent conflicts with other wheels that use the runtime at
a different version.
@QuLogic
Copy link
Member Author
QuLogic commented Aug 9, 2024

For the record, this appears to make delvewheel entirely unnecessary: It does not appear to do anything, either bundling dlls nor editing __init__.py files.

This is only true for CPython; on PyPy, it bundles VCRUNTIME140_1.dll.

@QuLogic QuLogic marked this pull request as ready for review August 9, 2024 23:19
@QuLogic
Copy link
Member Author
QuLogic commented Aug 9, 2024

I modified @BertJorissen's example to test all Python versions and then take the wheels from this PR, and they all passed: https://github.com/QuLogic/mplpb11/actions/runs/10326876201

@QuLogic
Copy link
Member Author
QuLogic commented Aug 9, 2024

On the latest v3.9.x build, we have:

$ objdump -x _c_internal_utils.cp313-win_amd64.pyd | rg 'DLL Name' | sort
	DLL Name: api-ms-win-crt-heap-l1-1-0.dll
	DLL Name: api-ms-win-crt-math-l1-1-0.dll
	DLL Name: api-ms-win-crt-runtime-l1-1-0.dll
	DLL Name: api-ms-win-crt-string-l1-1-0.dll
	DLL Name: KERNEL32.dll
	DLL Name: msvcp140-cb1364a8f14ec1d3687d6faef0fd327e.dll
	DLL Name: ole32.dll
	DLL Name: python313.dll
	DLL Name: SHELL32.dll
	DLL Name: USER32.dll
	DLL Name: VCRUNTIME140.dll

On this PR, we have:

$ objdump -x _c_internal_utils.cp313-win_amd64.pyd | rg 'DLL Name' | sort
	DLL Name: api-ms-win-crt-heap-l1-1-0.dll
	DLL Name: api-ms-win-crt-math-l1-1-0.dll
	DLL Name: api-ms-win-crt-runtime-l1-1-0.dll
	DLL Name: api-ms-win-crt-string-l1-1-0.dll
	DLL Name: KERNEL32.dll
	DLL Name: ole32.dll
	DLL Name: python313.dll
	DLL Name: SHELL32.dll
	DLL Name: USER32.dll
	DLL Name: VCRUNTIME140_1.dll
	DLL Name: VCRUNTIME140.dll

That is, we have dropped msvcp140-cb1364a8f14ec1d3687d6faef0fd327e.dll, and added VCRUNTIME140_1.dll. According to @zooba (who suggested this method in the first place), we can't avoid that using this method, but it should be fine, as Python 3.8+ included that DLL itself, and we require 3.9.

PyPy3.9 doesn't include VCRUNTIME140_1.dll, but it also doesn't include VCRUNTIME140.dll (as CPython does), and says to install the vcredist on its download page. delvewheel does currently bundle the former, but I'm not sure if we should disable that and leave PyPy to the vcredist install.

@QuLogic
Copy link
Member Author
QuLogic commented Aug 9, 2024

We also saved a bit on the wheel sizes:

$ du -bc *.whl
7959294	matplotlib-3.9.1.post2.dev46+g3ed3d7b19a-cp310-cp310-win_amd64.whl
7969068	matplotlib-3.9.1.post2.dev46+g3ed3d7b19a-cp311-cp311-win_amd64.whl
7972949	matplotlib-3.9.1.post2.dev46+g3ed3d7b19a-cp312-cp312-win_amd64.whl
7973076	matplotlib-3.9.1.post2.dev46+g3ed3d7b19a-cp313-cp313-win_amd64.whl
7954828	matplotlib-3.9.1.post2.dev46+g3ed3d7b19a-cp39-cp39-win_amd64.whl
7980386	matplotlib-3.9.1.post2.dev46+g3ed3d7b19a-pp39-pypy39_pp73-win_amd64.whl
47809601	total

vs:

$ du -bc *.whl
7819858	matplotlib-3.10.0.dev46+gb9ddba6d6c-cp310-cp310-win_amd64.whl
7829606	matplotlib-3.10.0.dev46+gb9ddba6d6c-cp311-cp311-win_amd64.whl
7833037	matplotlib-3.10.0.dev46+gb9ddba6d6c-cp312-cp312-win_amd64.whl
7833265	matplotlib-3.10.0.dev46+gb9ddba6d6c-cp313-cp313-win_amd64.whl
7814134	matplotlib-3.10.0.dev46+gb9ddba6d6c-cp39-cp39-win_amd64.whl
7845889	matplotlib-3.10.0.dev46+gb9ddba6d6c-pp39-pypy39_pp73-win_amd64.whl
46975789	total

or just under 140K/2% less each.

@ianthomas23
Copy link
Member

PyPy3.9 doesn't include VCRUNTIME140_1.dll, but it also doesn't include VCRUNTIME140.dll (as CPython does), and says to install the vcredist on its download page. delvewheel does currently bundle the former, but I'm not sure if we should disable that and leave PyPy to the vcredist install.

I would be inclined to remove use of delvewheel.

@ksunden
Copy link
Member
ksunden commented Aug 12, 2024

I'll go ahead and merge, we can revisit possibly removing delvewheel entirely as a followup, particularly if we have problems on PyPy

@ksunden ksunden merged commit 7be8675 into matplotlib:v3.9.x Aug 12, 2024
41 checks passed
@QuLogic QuLogic deleted the static-msvc branch August 12, 2024 21:27
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Aug 15, 2024
As of matplotlib#28687, our extensions depend on `VCRUNTIME140_1.dll`, and this
was allowed because Python 3.8+ started shipping that file. The original
report in matplotlib#18292 was for Python 3.7, which didn't ship the DLL, but we
require Python 3.10 now, so it's safe again.

Since we can use that dependency, there's no need to disable the option
that started requiring it in the first place. As noted in the original
blog post [1], this will make our extensions smaller, and slightly
faster.

[1] https://devblogs.microsoft.com/cppblog/making-cpp-exception-handling-smaller-x64/
@ianthomas23
Copy link
Member

Can someone confirm if and/or how these changes propagate through to the nightly wheels at https://anaconda.org/scientific-python-nightly-wheels/matplotlib?

Certainly the changes are in the main branch:

CIBW_CONFIG_SETTINGS_WINDOWS: >-
setup-args="--vsenv"
setup-args="-Db_vscrt=mt"
setup-args="-Dcpp_link_args=['ucrt.lib','vcruntime.lib','/nodefaultlib:libucrt.lib','/nodefaultlib:libvcruntime.lib']"

The latest nightly wheel at time of writing for 3.12 is matplotlib-3.10.0.dev548+g9f49b07ba9-cp312-cp312-win_amd64.whl and I can see that it was uploaded in gha https://github.com/matplotlib/matplotlib/actions/runs/10359513569/job/28676108425. But the new compilation flags do not appear to be used (see the python -m pip wheel line):

Building cp312-win_amd64 wheel
CPython 3.12 Windows 64bit

Installing Python cp312...
                                                              ✓ 9.33s
Setting up build environment...
                                                              ✓ 6.35s
Installing build tools...
                                                              ✓ 0.36s
Running before_build...
                                                              ✓ 1.24s
Building wheel...
  
  + python -m pip wheel 'C:\Users\runneradmin\AppData\Local\Temp\cibw-sdist-1qoik8g_\matplotlib-3.10.0.dev548+g9f49b07ba9' '--wheel-dir=C:\Users\runneradmin\AppData\Local\Temp\cibw-run-taxp7jhy\cp312-win_amd64\built_wheel' --no-deps --config-settings=setup-args=--vsenv
  Processing c:\users\runneradmin\appdata\local\temp\cibw-sdist-1qoik8g_\matplotlib-3.10.0.dev548+g9f49b07ba9
Installing build dependencies: started
    Installing build dependencies: finished with status 'done'
    Getting requirements to build wheel: started
    Getting requirements to build wheel: finished with status 'done'
    Installing backend dependencies: started
    Installing backend dependencies: finished with status 'done'
    Preparing metadata (pyproject.toml): started
    Preparing metadata (pyproject.toml): finished with status 'done'
  Building wheels for collected packages: matplotlib
    Building wheel for matplotlib (pyproject.toml): started
    Building wheel for matplotlib (pyproject.toml): finished with status 'done'
    Created wheel for matplotlib: filename=matplotlib-3.10.0.dev548+g9f49b07ba9-cp312-cp312-win_amd64.whl size=7795952 sha[256](https://github.com/matplotlib/matplotlib/actions/runs/10359513569/job/28676108425#step:5:266)=c9b1db3dd57b9ce2c6c1e920d496192fbbbd3f3f849df8b663124c0e66d1c09e
    Stored in directory: c:\users\runneradmin\appdata\local\pip\cache\wheels\1d\8f\a3\78586ff923c54b655f8b92bad2fea456736530302a2803c5e9
  Successfully built matplotlib
                                                             ✓ 63.18s
Repairing wheel...
  
  + delvewheel repair -w C:\Users\runneradmin\AppData\Local\Temp\cibw-run-taxp7jhy\cp312-win_amd64\repaired_wheel C:\Users\runneradmin\AppData\Local\Temp\cibw-run-taxp7jhy\cp312-win_amd64\built_wheel\matplotlib-3.10.0.dev548+g9f49b07ba9-cp312-cp312-win_amd64.whl
  C:\Users\runneradmin\AppData\Local\Temp\cibw-run-taxp7jhy\cp312-win_amd64\build\venv\Lib\site-packages\delvewheel\_wheel_repair.py:344: UserWarning: mpl_toolkits does not contain __init__.py. If it is a namespace package, use the --namespace-pkg option. Otherwise, create an empty __init__.py file to silence this warning.
    warnings.warn(
  repairing C:\Users\runneradmin\AppData\Local\Temp\cibw-run-taxp7jhy\cp312-win_amd64\built_wheel\matplotlib-3.10.0.dev548+g9f49b07ba9-cp312-cp312-win_amd64.whl
  finding DLL dependencies
  copying DLLs into matplotlib.libs
  mangling DLL names
  patching matplotlib\__init__.py
  patching mpl_toolkits\__init__.py
  repackaging wheel
  fixed wheel written to C:\Users\runneradmin\AppData\Local\Temp\cibw-run-taxp7jhy\cp312-win_amd64\repaired_wheel\matplotlib-3.10.0.dev548+g9f49b07ba9-cp312-cp312-win_amd64.whl

and delvewheel repairs the wheel which is what it used to do but should now be a no-op here. I am looking at this because this wheel is still failing my reproducer tests https://github.com/ianthomas23/mpl-test/actions/runs/10417044468 and I was expecting the forward-ported changes to have made their way through to the nightly wheels by now.

@QuLogic
Copy link
Member Author
QuLogic commented Aug 16, 2024

The Windows wheel is correctly built for the ones that made it: https://github.com/matplotlib/matplotlib/actions/runs/10415168089/job/28845271052#step:4:305 but the full wheel builds have not yet passed since the merge up due to issues on macOS: https://github.com/matplotlib/matplotlib/actions/runs/10415168089/job/28845271328

@ianthomas23
Copy link
Member

Thanks. I am, understandably I hope, getting rather sensitive about this!

@QuLogic
Copy link
Member Author
QuLogic commented Aug 17, 2024

So it's not specifically macOS, but that's just the fastest builder to hit the problem, which is building kiwisolver on PyPy (which we don't do on Windows for nightlies yet due to missing dependencies, so that's why it passed.)

Since kiwisolver only has PyPy 3.9 wheels, and we are building nightlies on PyPy 3.10, it triggers a source build, and we are hitting a bug in setuptools: pypa/distutils#283 I debugged that and suggested a fix there, but not sure when that might be out.

I opened a PR on kiwisolver to enable PyPy 3.10 wheels: nucleic/kiwi#182

And a PR here to avoid the buggy setuptools: #28735

@ianthomas23
Copy link
Member

Published Matplotlib nightly wheels do now include the fix, and ContourPy CI is all passing.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 13, 2024
Convert to wheel.mk.

Wrappers 1.4.7 | Solver 1.4.2 | 03/09/2024

    no library changes only fixes to the build infrastructure

Wrappers 1.4.6 | Solver 1.4.2 | 03/09/2024

    drop support for Python 3.7 PR #183
    add support for Python 3.13 PR #183
    update linking strategy on Windows when building wheels PR #183 This is in line with Matplotlib strategy matplotlib/matplotlib#28687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Run cibuildwheel Run wheel building tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0