Add Windows ARM64 wheel distribution - #3735
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Windows ARM64 wheel distribution support for PyPI releases.
Changes:
- Adds a Windows ARM64 cibuildwheel job.
- Adjusts Windows header handling and raises the setuptools minimum.
- Documents Windows AMD64 and ARM64 distributions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Summary |
|---|---|
lib/py/setup.py |
Adjusts Windows header inclusion for cibuildwheel builds. |
lib/py/pyproject.toml |
Raises the minimum setuptools version. |
doc/ReleaseManagement.md |
Documents Windows ARM64 wheels. |
.github/workflows/pypi.yml |
Adds ARM64 wheel builds; the ARM64 pattern must start at cp311 rather than requesting unsupported cp310 builds. |
Suppressed comments (1)
lib/py/setup.py:40
- Because cibuildwheel sets
CIBUILDWHEEL=1for both matrix entries, this condition removescompat/win32from the Windows AMD64 build as well as the ARM64 build. That regresses the compatibility path for older MSVC toolchains and contradicts the stated intent to retain the shim; restrict the exception to the ARM64 job by checking the selected cibuildwheel architecture as well.
if os.environ.get('CIBUILDWHEEL') != '1':
include_dirs.append('compat/win32')
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/py/pyproject.toml:2
- This minimum still permits setuptools 65.4.1 through 68.x, but
CHANGES.md:315documents thatpip installfails with setuptools < 69 because of the PEP 517setup.pyflow. A consumer or build environment pinned to the declared minimum can therefore remain broken; raise the lower bound to the first known working version.
requires = ["setuptools>=65.4.1", "wheel"]
|
the MSVC build on CI failed: https://github.com/apache/thrift/actions/runs/32422316847/job/96597632215 |
That's odd, because I haven't touched any of the C++ sources, and looking at the build logs, the |
|
then maybe it's just flaky and you can retry it to see if it works. |
I don't see the option to re-run the workflow myself, I have squashed and force pushed the commits, so let's try again when someone is able to approve the workflows. Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.github/workflows/pypi.yml:115
- This adds a new platform to the release matrix, which is a non-trivial change under the repository contribution requirements, but the PR metadata has no THRIFT JIRA ticket or title prefix. Please add or link the required JIRA issue before merging.
- platform: windows-arm64
runner: windows-11-arm
archs: ARM64
build: "cp310-win_* cp311-win_* cp312-win_* cp313-win_* cp314-win_*"
| if os.environ.get('CIBUILDWHEEL') != '1': | ||
| include_dirs.append('compat/win32') |
There was a problem hiding this comment.
The Python versions we are building wheels for need Visual C++ 14 or higher to build, which includes a proper stdint.h -- therefore, the shim included from compat/win32 is never going to be needed for any (AMD64 or ARM64) builds when compiled from cibuildwheel.
fishy
left a comment
There was a problem hiding this comment.
looks good to me, but we'll only know whether it works when we actually cut a release next time.
As the title says. It's mostly straightforward.
One of the slightly hacky bits is the change in
lib/py/setup.pyto not includecompat/win32/stdint.hwhen building in CI. That shim is not needed anyway for most versions of Visual C++/Build Tools found in the wild today. However I did not get rid of it altogether because someone, somewhere might have a version that does need it. The AMD64 build seems to work fine in any case, but on ARM64 thearm64_neon.hfile chokes on it. Let me know if there is a better idea to handle this.[skip ci]anywhere in the commit message to free up build resources.