Build aarch64 wheels #563
Open
Build aarch64 wheels #563
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dstansby
force-pushed
the
aarch64-wheels
branch
4 times, most recently
from
b0591df to
4788c35
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 62 62
Lines 2761 2761
=======================================
Hits 2759 2759
Misses 2 2 |
dstansby
force-pushed
the
aarch64-wheels
branch
4 times, most recently
from
182007e to
31ee5d5
Compare
dstansby
added
the
build all wheels
dstansby
removed
the
build all wheels
dstansby
force-pushed
the
aarch64-wheels
branch
from
689c3e4 to
ebccd68
Compare
jakirkham
reviewed
Nov 14, 2024
Comment on lines
+27
to
+29
| elif os.name == 'posix' and os.uname()[4] != 'aarch64': | ||
| # These flags aren't recognised by gcc on aarch64 | ||
| # (at least when we build the aarch64 wheens on GitHub actions) |
There was a problem hiding this comment.
Suggested change
| elif os.name == 'posix' and os.uname()[4] != 'aarch64': | |
| # These flags aren't recognised by gcc on aarch64 | |
| # (at least when we build the aarch64 wheens on GitHub actions) | |
| elif os.name == 'posix' and os.uname()[4] != 'aarch64': | |
| # These instructions are not available on `aarch64`. | |
| # So there is no need to disable them as they won't show up there. |
| CIBW_SKIP: "pp* *-musllinux_* *win32 *_i686 *_s390x" | ||
| CIBW_ARCHS_MACOS: native | ||
| CIBW_ARCHS_WINDOWS: native | ||
| CIBW_ARCHS_LINUX: "x86_64 aarch64" |
There was a problem hiding this comment.
So will this build both Linux wheels in the same job?
| @@ -18,14 +30,25 @@ jobs: | |||
| env: | |||
| CIBW_TEST_COMMAND: python -c "import numcodecs" | |||
| CIBW_BUILD: "cp310-* cp311-* cp312-* cp313-*" | |||
| CIBW_SKIP: "pp* *-musllinux_* *win32 *_i686 *_s390x" | |||
| CIBW_ARCHS_MACOS: native | |||
| CIBW_ARCHS_WINDOWS: native | |||
There was a problem hiding this comment.
Do we want to try the same thing for Windows ARM?
Suggested change
| CIBW_ARCHS_WINDOWS: native | |
| CIBW_ARCHS_WINDOWS: "AMD64 ARM64" |
Basing this off this doc
| @@ -18,14 +30,25 @@ jobs: | |||
| env: | |||
| CIBW_TEST_COMMAND: python -c "import numcodecs" | |||
| CIBW_BUILD: "cp310-* cp311-* cp312-* cp313-*" | |||
| CIBW_SKIP: "pp* *-musllinux_* *win32 *_i686 *_s390x" | |||
| CIBW_ARCHS_MACOS: native | |||
There was a problem hiding this comment.
Think we want to make sure we are building for x86_64 (native I believe) and arm64 (for M1, M2, etc.). Both we already build for. So it would be good to continue supporting them
Suggested change
| CIBW_ARCHS_MACOS: native | |
| CIBW_ARCHS_MACOS: "x86_64 arm64" |
| - name: Restrict number of wheel builds on PRs | ||
| if: ${{ !env.BUILD_ALL }} | ||
| run: | | ||
| echo "CIBW_BUILD=cp310-win_amd64 cp311-manylinux_x86_64 cp312-macosx_x86_64 cp312-macosx_arm64" >> $GITHUB_ENV |
There was a problem hiding this comment.
Should we add Python 3.13 in this list? Perhaps using the other macOS build?
Suggested change
| echo "CIBW_BUILD=cp310-win_amd64 cp311-manylinux_x86_64 cp312-macosx_x86_64 cp312-macosx_arm64" >> $GITHUB_ENV | |
| echo "CIBW_BUILD=cp310-win_amd64 cp311-manylinux_x86_64 cp312-macosx_x86_64 cp313-macosx_arm64" >> $GITHUB_ENV |
Comment on lines
+9
to
+19
| env: | ||
| # Build all wheels on either: | ||
| # - tagged release | ||
| # - pull request opened with "test all wheels" label | ||
| # - pull requests when "test all wheels" label is added | ||
| BUILD_ALL: | | ||
| ${{ | ||
| (github.event_name == 'push' && startsWith(github.event.ref, 'refs/tags/v')) | ||
| || contains(github.event.pull_request.labels.*.name, 'test all wheels') | ||
| || (github.event.action == 'labeled' && github.event.label.name == 'test all wheels') | ||
| }} |
There was a problem hiding this comment.
Ok so in this case we are relying on maintainers to add the label for the wheel builds
Is it worth considering an option users can opt-in with? Like commit message text or PR title?
dstansby
force-pushed
the
aarch64-wheels
branch
from
ebccd68 to
e590abd
Compare
|
Any thoughts on my comments above? |
|
Sorry, I've been distracted by other stuff - thanks for the comments, will try and get round to this soon! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
aarch64to this list for linux buildsThis speeds up runs on PRs, but allows the flexibiility of doing a full run of wheel building if needed by adding the label.
Fixes #312, fixes #288, and an updated version of #315
TODO: