-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add aarch64 wheel build support #63
Conversation
.github/workflows/python-package.yml
Outdated
python-version: ${{ matrix.python-version }} | ||
- name: Build and Test Wheel Arm64 | ||
run: | | ||
sudo chmod +x run_manylinux_aarch64_build.sh |
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 we remove the sudo?
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 we remove the sudo?
Now run_manylinux_aarch64_build.sh is not getting used. So, this is not required.
run_manylinux_aarch64_build.sh
Outdated
""" | ||
|
||
|
||
DOCKER_IMAGE=${DOCKER_IMAGE:="quay.io/pypa/manylinux2014_aarch64:latest"} |
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.
Is the change as simple as changing DOCKER_IMAGE
? Can we just pass this as an argument to the existing script?
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.
Is the change as simple as changing
DOCKER_IMAGE
? Can we just pass this as an argument to the existing script?
Done.
""" | ||
|
||
|
||
DOCKER_IMAGE=${DOCKER_IMAGE:="quay.io/pypa/manylinux2014_aarch64:latest"} |
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.
I believe the existing publish script will work as long as you have pre-built the right images. It includes a glob pattern to capture pre-built binary wheels, so that should just capture the aarch64 ones.
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.
I believe the existing publish script will work as long as you have pre-built the right images. It includes a glob pattern to capture pre-built binary wheels, so that should just capture the aarch64 ones.
Done. Please share your feedback regarding the changes.
Hi @odidev this looks great! I made a few comments, but the change looks straight forward enough (unless I missed something). I think we can simplify the logic a little bit. I made some comments regarding that. Also please rebase on the current master to avoid the conflict in Also, don't forget to add a note in the CHANGELOG.rst |
Signed-off-by: odidev <odidev@puresoftware.com>
cfb2e34
to
ab2bbf2
Compare
Done. |
Add aarch64 wheel build support.
Related to #62 @Erotemic Could you please review this PR?