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.

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 #639

Closed
wants to merge 1 commit into from
Closed

Build aarch64 wheels #639

wants to merge 1 commit into from

Conversation

janaknat
Copy link
Contributor

@janaknat janaknat commented Oct 15, 2022

@misl6
Copy link
Member

misl6 commented Oct 16, 2022

Love that feature!

@janaknat , I see some duplicated code from .github/workflows/create.yml, can you merge your feature into the create.yml file in order to avoid duplication?

@janaknat
Copy link
Contributor Author

@misl6 Done. Moved the code to .github/workflows/create.yml.

@janaknat
Copy link
Contributor Author

@misl6 Ping.

Comment on lines 31 to 38
- name: Set up QEMU for aarch64 wheels
if: runner.os == 'Linux'
uses: docker/setup-qemu-action@v1
with:
platforms: arm64
Copy link
Member

Choose a reason for hiding this comment

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

QEMU only needs to be set up for aarch64 builds, not also x86_64 ones.
We're doing something similar here: https://github.com/kivy/kivy/blob/master/.github/workflows/manylinux_wheels.yml

(Same applies for the Build and test aarch64 wheels) step.

@janaknat
Copy link
Contributor Author

@misl6 Added checks.

Comment on lines 71 to 73
CIBW_TEST_REQUIRES: 'pytest'
CIBW_BEFORE_TEST: 'yum install ant -y && cd {project} && ant all'
CIBW_TEST_COMMAND: 'cd {project}/tests/ && CLASSPATH=../build/test-classes:../build/classes python -m pytest -v'
Copy link
Member

Choose a reason for hiding this comment

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

Optimization: we can expand the tests also to the x86_64 version, so there's no need to keep a separate step for aarch64 wheels.

@janaknat
Copy link
Contributor Author

pp39-manylinux_aarch64 passes. Required the extras [dev,ci] to be installed for testing.

@janaknat
Copy link
Contributor Author

@misl6 Both x86 and aarch64 Linux wheels are built and tested in a single step.

@janaknat
Copy link
Contributor Author

@misl6 Ping!

1 similar comment
@janaknat
Copy link
Contributor Author

@misl6 Ping!

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Hi @janaknat ,

I'm sorry for the late reply, I've left a couple of comments that need to be addressed before the merge.

Thank you for your support!

Comment on lines 28 to 31
cibw_skip: '*-manylinux_i686'
cibw_archs: 'x86_64'
- os: ubuntu-latest
architecture: 'x86'
cibw_archs: 'i686'
Copy link
Member

Choose a reason for hiding this comment

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

We want to skip the build for *-manylinux_i686 wheels, as we removed support in a previous version.
Can you please change the config to only build x86_64 and aarch64 wheels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will remove i686 (x86).

Comment on lines 68 to 74
- name: Build and test Linux wheels
if: ${{ matrix.os == 'ubuntu-latest' }}
env:
CIBW_SKIP: '*musllinux* ${{ matrix.cibw_skip }}'
CIBW_ARCHS: '${{ matrix.cibw_archs }}'
CIBW_TEST_REQUIRES: 'pytest'
CIBW_TEST_EXTRAS: 'dev,ci'
CIBW_BEFORE_TEST: 'yum install ant -y && cd {project} && ant all'
CIBW_TEST_COMMAND: 'cd {project}/tests/ && CLASSPATH=../build/test-classes:../build/classes python -m pytest -v'
run: python -m cibuildwheel --output-dir dist
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep the tests separate from the build phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, it was built in this step and tested later on in a separate step. We want to use that approach? This current approach is to build the wheels and if successful, the CIBW_TEST_COMMAND is run to test the wheels.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, but unfortunately (as shown here: https://github.com/pypa/cibuildwheel/blob/main/README.md#how-it-works), tests are run into the same container / runner session.
Keeping it separate allows us to be sure that the build scripts/installations are not influencing the tests.
(A new and clean virtualenv is created and used for tests, but it's not enough sometime)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can test the wheels in a separate step for x86_64. For aarch64, that won't be possible because github natively does not support arm64 architecture.

Copy link
Member

Choose a reason for hiding this comment

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

For aarch64 tests, qemu will help 😀

When setting up the test for aarch64, we can add:

- name: Set up QEMU for aarch64 tests
  if: ${{ matrix.test_arch == 'aarch64' }} # here we need to add `test_arch` in the matrix.
  uses: docker/setup-qemu-action@v1
  with:
      platforms: arm64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@misl6 True, but that only sets up the QEMU for aarch64 binary. Any commands run further will be on the host architecture. To run the tests, we will have to use a docker image (manylinux2014_aarch64), setup python, pytest, ant. Then install the generated wheels, run 'ant all' and then the unit tests. cibuildwheels sets all of that up for us when we specify what we need using CIBW_TEST_REQUIRES. Unfortunately, cibuildwheel does not have a 'skip build and test only' option. We'll probably end up re-implementing the testing logic of cibuildwheel ourselves, unless there is another option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@misl6 Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @janaknat !
We're doing something similar here (that is actually used to build dependencies, as we can cache them, but the same approach could be used to process tests): https://github.com/kivy/kivy/blob/a94505ef854acae0046ab8d822a4c96c011bd269/.github/workflows/manylinux_wheels.yml#L73-L77

Also, build and test x86 wheels in one step, similar to aarch64.
@misl6
Copy link
Member

misl6 commented May 8, 2023

Hi @janaknat !

This PR has been superseded by #653 (introduces a self-hosted runner for Ubuntu 22.04 LTS (arm64)) which simplifies the build and test process (and also reduces the build time significantly).

Everything started on top of your PR 😀. I squashed the two commits by not removing you as a co-author. (Hope is appreciated)

Thank you!

@misl6 misl6 closed this May 8, 2023
@janaknat
Copy link
Contributor Author

@misl6 Thank you for helping with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants