-
Notifications
You must be signed in to change notification settings - Fork 28
Add windows arm64 support #183
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
Conversation
.github/workflows/publish.yml
Outdated
| - cp311*win_arm64 | ||
| - cp312*win_arm64 | ||
| - cp313*win_arm64 | ||
| - cp314*win_arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One job should be enough, given that pyerfa builds abi3 weels
| - cp311*win_arm64 | |
| - cp312*win_arm64 | |
| - cp313*win_arm64 | |
| - cp314*win_arm64 | |
| - cp3*-win_arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Though the problem I ran into is that when I run cp3*-win_arm64 it tried to build for python 3.9 and fails when building numpy which is only available on arm64 for 3.11+ so I specified 3.11-3.14 individually. Should I replace it instead with just one version like cp311*win_arm64 or maybe cp314*win_arm64? Or is it possible to do something kind of like cp31+([1-9])*win_arm64 that specifies any python after 3.11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you're right, it won't work on 3.9 or 3.10. However, it's still important to use a single line here, as the underlying workflow will create as many jobs as there are lines, and we ultimately don't need more than the one wheel. This should do:
| - cp311*win_arm64 | |
| - cp312*win_arm64 | |
| - cp313*win_arm64 | |
| - cp314*win_arm64 | |
| - cp3{11,12,13,14}-win_arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably doesn't really matter here, but it looks like another option that works is cp31[1-9]*win_arm64 in case you wanted it to build/test for versions after 3.14 later without having to go back and update the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The report will still need to be updated for cibw to target newer versions. Also, this won't work for 3.20 and later.
In any case, 3.15 might introduce some new ABI we'll want to use too (PEP 803 and 809), which will warrant a revisit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved just in case someone else is like me and sees the cp3* and wonders why it isn't used here. (I don't want to re-run just to add a comment...)
Co-authored-by: Clément Robert <[email protected]>
avalentino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
mhvk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too. I won't merge quite yet in case @astrofrog wants to have a look, but if not, let's get this in. Thanks, @finnagin!
|
|
||
| build_and_publish: | ||
|
|
||
| uses: OpenAstronomy/github-actions-workflows/.github/workflows/publish.yml@28e947497bed4d6ec3fa1d66d198e95a1d17bc63 # v2.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this hash doesn't match the version in comment, but points ahead of it, because version 2.2.3 isn't released yet.
https://github.com/OpenAstronomy/github-actions-workflows
(pyerfa maintainers: this class of issue can be treated as security problems and as such, can be linted against using zizmor. Let me know if you're interested and I can set it up for this repo, as I previously did for astropy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that! @neutrinoceros should we wait for a new release of OpenAstronomy/github-actions-workflows and then bump the hash/version to that v2.2.3 release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes ! It's not difficult to release, we just need someone to pick it up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.3.0 is out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I'll go ahead and update the hash/version number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be merged now?
|
OK, let's get this in. Thanks, @finnagin! @neutrinoceros - you mentioned you had set things up for astropy such that one does not accidentally use unreleased versions in CI; if it is little work, I guess we might we well do the same here. |
|
@mhvk I started with liberfa/erfa#104 (since erfa is a submodule to pyerfa, this is the first necessary step). |
Closes #182
This would add windows arm64 tests to
ci_workflows.ymland builds topublish.yml.