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.

Sign up for GitHub

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

Build aarch64 wheels #563

Closed
wants to merge 7 commits into from
Closed

Build aarch64 wheels #563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d8ae726
Build aarch64 wheels
dstansby Aug 20, 2024
b28aa1c
Add QEMU setup
dstansby Aug 20, 2024
46a39c8
Skip sse and avx flags on aarch64
dstansby Aug 21, 2024
eb280d6
Explicitly specify architectures
dstansby Aug 23, 2024
573cff7
Fix build_all conditional
dstansby Aug 23, 2024
6bd9cf6
Use environment variable for build_all
dstansby Aug 23, 2024
e590abd
Add changelog entry
dstansby Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions .github/workflows/wheel.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,21 @@ concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

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')
}}
Comment on lines +9 to +19
Copy link
Member

Choose a reason for hiding this comment

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

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?


jobs:
build_wheels:
name: Build wheel on ${{ matrix.os }}
name: Build wheels on ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
Expand All @@ -18,14 +30,25 @@ jobs:
env:
CIBW_TEST_COMMAND: python -c "import numcodecs"
CIBW_BUILD: "cp311-* cp312-* cp313-*"
CIBW_SKIP: "pp* *-musllinux_* *win32 *_i686 *_s390x"
CIBW_ARCHS_MACOS: native
Copy link
Member

Choose a reason for hiding this comment

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

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"

CIBW_ARCHS_WINDOWS: native
Copy link
Member

Choose a reason for hiding this comment

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

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

CIBW_ARCHS_LINUX: "x86_64 aarch64"
Copy link
Member

Choose a reason for hiding this comment

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

So will this build both Linux wheels in the same job?

# note: CIBW_ENVIRONMENT is now set in pyproject.toml

steps:
- uses: actions/checkout@v4
with:
submodules: true

- 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
Copy link
Member

Choose a reason for hiding this comment

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

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


- name: Set up QEMU
if: ${{ matrix.os == 'ubuntu-latest' }}
uses: docker/setup-qemu-action@v3

- uses: pypa/[email protected]

- uses: actions/upload-artifact@v4
Expand Down
14 changes: 13 additions & 1 deletion docs/release.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ Improvements
Import errors caused by optional dependencies (ZFPY, MsgPack, CRC32C, and PCodec)
are still silently caught.
By :user:`David Stansby <dstansby>`, :issue:`550`.

* Build aarch64 wheels on linux.
By :user:`David Stansby <dstansby>`, :issue:`531`

0.14.1
------
Expand Down Expand Up @@ -116,13 +117,24 @@ Enhancements
* Cleaned up the table of contents in the documentation to list codecs by category
:user:`David Stansby <dstansby>`, :issue:`458`.

Fix
~~~


Maintenance
~~~~~~~~~~~
* Change format() and old string formatting to f-strings.
By :user:`Dimitri Papadopoulos Orfanos <DimitriPapadopoulos>`, :issue:`439`.
* Remove pin on Sphinx
By :user:`Elliott Sales de Andrade <QuLogic>`, :issue:`552`.

* Restrict the number of wheels built on pull requests.
By :user:`David Stansby <dstansby>`, :issue:`531`

* Add an option to build all wheels on pull requests by adding
the 'build all wheels' label.
By :user:`David Stansby <dstansby>`, :issue:`531`


.. _release_0.13.0:

Expand Down
4 changes: 3 additions & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
if have_cflags:
# respect compiler options set by user
pass
elif os.name == 'posix':
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)
Comment on lines +27 to +29
Copy link
Member

Choose a reason for hiding this comment

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

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.

if disable_sse2:
base_compile_args.append('-mno-sse2')
elif have_sse2:
Expand Down
Loading