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

Add armv6 and armv7 to multiarch CI and fix build issues #2779

Merged
merged 2 commits into from
Apr 21, 2024

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Mar 28, 2024

This PR mainly does 3 2 things

  • Fixes arm32 specific warnings across the codebase Has been moved out to be done in a later PR
  • Fixes compilation on arm32 (both armv6 and armv7) under the meson build system. As a result of the meson PR, the NEON optimizations are enabled by default on armv7 now (in the legacy buildconfig it is controlled via an opt in flag). Now that it is much easier to pass per file flags, the flags needed to build with sse2neon.h are only passed to the files that need it. Therefore, I made it so that only the files that require the sse2neon.h header include it. If any file that shouldn't be including it tries to include it, the compilation fails.
  • And now coming to the good stuff, we finally have testing on CI to verify that we support both armv6 and armv7 based raspberrypi-like systems! The first level of testing tests that a wheel can be built and unit test can be run on armv6 and armv7. The second level of testing tests that an armv7 wheel can simply be renamed to lie that it's an armv6 wheel, and still pass tests on armv6 hardware. This is important because piwheels does exactly this to package pygame-ce wheels over at https://www.piwheels.org/project/pygame-ce/

@ankith26 ankith26 requested a review from a team as a code owner March 28, 2024 14:44
@ankith26 ankith26 force-pushed the ankith26-more-multiarch branch 2 times, most recently from 300d8a7 to 638d580 Compare March 28, 2024 16:26
@ankith26 ankith26 changed the title Add armv6, armv7 and riscv64 to multiarch CI Add armv6 and armv7 to multiarch CI Mar 28, 2024
@ankith26 ankith26 force-pushed the ankith26-more-multiarch branch 2 times, most recently from b87a028 to e536372 Compare March 28, 2024 17:05
@ankith26 ankith26 added ARM ARM stuff CI Issue with the Continuous Integration (CI), the actions/bots that test things labels Mar 29, 2024
@ankith26 ankith26 force-pushed the ankith26-more-multiarch branch 8 times, most recently from e64b23c to 6ed6020 Compare April 14, 2024 06:38
@ankith26 ankith26 force-pushed the ankith26-more-multiarch branch 11 times, most recently from 7a82be1 to 7e49dc1 Compare April 18, 2024 17:07
@ankith26
Copy link
Member Author

This PR is now done from my side. I have edited the opening comment in this PR to describe the PR in more detail

@ankith26 ankith26 added this to the 2.5.0 milestone Apr 18, 2024
@ankith26 ankith26 changed the title Add armv6 and armv7 to multiarch CI Add armv6 and armv7 to multiarch CI and fix build issues Apr 18, 2024
- bumps the multiarch action
- bumps to bookworm
- armv6 and armv7 testing added (image based on raspbian)
- also adds the ability to test an armv7 built wheel on armv6
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

CI stuff is cool but a little over my head.

Meson change makes sense.

Code changes make sense.

After this PR does it mean people on Pi will get NEON acceleration by default?

@ankith26
Copy link
Member Author

After this PR does it mean people on Pi will get NEON acceleration by default?

Basically whichever pi has NEON support in hardware will have NEON acceleration by default (so like all pi except pi 1 and older pi zero). And whichever pi doesn't, runtime detection that we have ensures that the NEON instructions won't be invoked (which will crash with illegal instruction error). It was a long standing speculation that this wouldn't work, but now this PR has tests that pass and therefore show that this approach works.

@Starbuck5
Copy link
Member

Basically whichever pi has NEON support in hardware will have NEON acceleration by default (so like all pi except pi 1 and older pi zero). And whichever pi doesn't, runtime detection that we have ensures that the NEON instructions won't be invoked (which will crash with illegal instruction error). It was a long standing speculation that this wouldn't work, but now this PR has tests that pass and therefore show that this approach works.

Very nice!

This does sound like something it would be to good to verify with a tester after our next dev release though, just to be sure.

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

OK, LGTM 👍

@ankith26 ankith26 merged commit dfaa76e into main Apr 21, 2024
39 checks passed
@ankith26 ankith26 deleted the ankith26-more-multiarch branch April 21, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARM ARM stuff CI Issue with the Continuous Integration (CI), the actions/bots that test things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants