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 support for Cirrus CI #1191

Merged
merged 18 commits into from Aug 30, 2022
Merged

Add support for Cirrus CI #1191

merged 18 commits into from Aug 30, 2022

Conversation

Jackenmen
Copy link
Contributor

@Jackenmen Jackenmen commented Jul 23, 2022

Resolves #145 and resolves #1240

For macOS arm64, there was a need for some changes to the test suite:

  • some of the tests were failing because they tried to build for CPython 3.6 (cp36-*) and on macOS arm64, you can only use CPython 3.8 and above (4bc6bd3, b39dc5b)
  • due to an issue with wheel 0.36.1 and earlier, generated wheel filenames had a wrong minimum macOS version (3c246c7)
  • due to Fix cp38-macosx universal2 and arm64 Python pkg URL #1169, cibuildwheel generated cp38-macosx_arm64 wheels without testing them which caused it to not delete the wheel when the test expected it to (2b35f3c)

Link to a Cirrus CI test suite build: https://cirrus-ci.com/build/6463822541094912
Link to a Cirrus CI minimal example build: https://cirrus-ci.com/build/6073962235953152
Link to a Cirrus CI macOS Intel example build: https://cirrus-ci.com/build/6736767930859520

@Jackenmen Jackenmen changed the title Skip Python 3.6 test on macOS arm64 Add support for Cirrus CI Jul 24, 2022
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Exciting! I'd love to have native macos-arm64 CI, and another native linux-aarch64 is no bad thing either!

I've tried to add the service to this repo, but it seems that it requires a pypa admin. @pradyunsg would you mind adding the free tier of CirrusCI to this repo? https://github.com/marketplace/cirrus-ci

.cirrus.yml Outdated Show resolved Hide resolved
test/test_dependency_versions.py Show resolved Hide resolved
test/test_testing.py Outdated Show resolved Hide resolved
unit_test/environment_test.py Outdated Show resolved Hide resolved
test/test_subdir_package.py Outdated Show resolved Hide resolved
test/test_subdir_package.py Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

Is this ready to review/go? Might be nice to get it in for the upcoming release?

@Jackenmen
Copy link
Contributor Author

Is this ready to review/go? Might be nice to get it in for the upcoming release?

Sadly as I mentioned in the Discord server, my account on Cirrus CI got suspended, most likely by an automated system, so I can't really do anything until their support responds. I emailed them about this 2 days ago, but no response yet. It's possible that if Pradyun added the Cirrus CI app to the PyPA org, it would be possible to see the build output but it's also equally if not more likely that it just wouldn't run the build when it sees me as the one triggering it.

@joerick
Copy link
Contributor

joerick commented Aug 16, 2022

@pradyunsg has added cirrus ci to this repo, so let's try again... perhaps try pushing an empty commit and see if cirrus picks it up?

@Jackenmen
Copy link
Contributor Author

My account has actually been unsuspended earlier today after I re-asked on their GitHub rather than their support email so their CI should just work now for me again. I'm going to merge main into this branch first and I'll update this PR to not make changes that are no longer necessary.

Note that Linux arm64 build still doesn't work due to cirruslabs/cirrus-ci-docs#1034, I think it might make sense to leave that work for a future PR.

@Jackenmen
Copy link
Contributor Author

Jackenmen commented Aug 17, 2022

Alright, all review comments should be addressed and the tests pass. I dropped Linux arm64 support - I don't think it's worth blocking over this and once this PR is merged, a new issue should be created for that with explanation what is a potential blocker for it.

I updated the PR description appropriately and included links to successful builds for example files. This PR should now be ready for review.

@Jackenmen Jackenmen marked this pull request as ready for review August 17, 2022 16:30
@mayeut
Copy link
Member

mayeut commented Aug 21, 2022

Note that Linux arm64 build still doesn't work due to cirruslabs/cirrus-ci-docs#1034, I think it might make sense to leave that work for a future PR.

Linux arm64 might also be solved by cirruslabs/cirrus-ci-docs#1027, whichever comes first. It makes to leave that work for a future PR.

@Jackenmen Jackenmen closed this Aug 21, 2022
@Jackenmen Jackenmen reopened this Aug 21, 2022
@Jackenmen
Copy link
Contributor Author

I was hoping to re-trigger CircleCI check due to https://status.circleci.com/incidents/3d31fnpthj7y but looks like that didn't work... Gonna try an empty commit.

@Jackenmen
Copy link
Contributor Author

Well, that definitely made the failing check disappear, I guess that's solved then 🤔

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

This all looks good to me!

@Jackenmen
Copy link
Contributor Author

I went ahead and made #1240 to track Linux arm64 support on Cirrus CI.

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I guess the intel Mac example is separate from the rest of the example because the arm64 one cross-compiles Intel wheels? At least for now, I'd guess the ideal workflow for most projects on Cirrus would be to have native wheels & run both runners - cross compiling is harder than native compiling. Eventually, as Intel runners become hard to find, then this workflow would be better.

Just a comment, looks great!

@Jackenmen
Copy link
Contributor Author

I guess the intel Mac example is separate from the rest of the example because the arm64 one cross-compiles Intel wheels?

The cirrus-ci-minimal example file does not do any cross-compilation since the default arch on Apple Silicon is only arm64 (which was done in cibuildwheel 2.9.0).

I'd guess the ideal workflow for most projects on Cirrus would be to have native wheels & run both runners - cross compiling is harder than native compiling.

Cirrus CI does not have an Intel runner. And in case of Apple Silicon, it's not really cross-compilation process per se since it uses Rosetta2 emulation and so it doesn't involve a typical cross-compilation toolchain.


<sup>¹ [Requires emulation](https://cibuildwheel.readthedocs.io/en/stable/faq/#emulation), distributed separately. Other services may also support Linux ARM through emulation or third-party build hosts, but these are not tested in our CI.</sup><br>
<sup>² [Uses cross-compilation](https://cibuildwheel.readthedocs.io/en/stable/faq/#universal2). It is not possible to test `arm64` and the `arm64` part of a `universal2` wheel on this CI platform.</sup><br>
<sup>³ [Uses cross-compilation](https://cibuildwheel.readthedocs.io/en/stable/faq/#universal2). Thanks to Rosetta 2 emulation, it is possible to test `x86_64` and both parts of a `universal2` wheel on this CI platform.</sup><br>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said in my previous comment, I'm not sure cross-compilation is the right term here, could anyone confirm whether "emulation" makes more sense here?

Suggested change
<sup[Uses cross-compilation](https://cibuildwheel.readthedocs.io/en/stable/faq/#universal2). Thanks to Rosetta 2 emulation, it is possible to test `x86_64` and both parts of a `universal2` wheel on this CI platform.</sup><br>
<sup[Uses Rosetta 2 emulation](https://cibuildwheel.readthedocs.io/en/stable/faq/#universal2). Thanks to the emulation, it is possible to test `x86_64` and both parts of a `universal2` wheel on this CI platform.</sup><br>

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, technically it's both! Cross-compilation is right, the wheel is built using cross-compilation, but we can still test the wheel using Rosetta 2 emulation.

I think the existing copy is fine as-is.

@Jackenmen
Copy link
Contributor Author

Linux arm64 might also be solved by cirruslabs/cirrus-ci-docs#1027, whichever comes first. It makes to leave that work for a future PR.

Good news, the linked issue got resolved and I've already made a configuration that so far appears to just work, I'm waiting for the tests on my fork to finish before pushing it here.

@Jackenmen
Copy link
Contributor Author

All tests ran in Cirrus CI succeeded, I've also updated the PR description with the new link to a build from examples/cirrus-ci-minimal.yml which also works fine.

It seems that one of the GitHub workflows failed due to an intermittent pip/PyPI issue but would have worked otherwise. I can't re-run it myself.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Great! The Cirrus aarch64 builder is is 1.5x faster than the Travis one, and 5x faster than emulation on GHA!

examples/cirrus-ci-minimal.yml Outdated Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
@joerick joerick merged commit 0117165 into pypa:main Aug 30, 2022
@joerick
Copy link
Contributor

joerick commented Aug 30, 2022

PR merged. 🎉

Stellar work on this @jack1142! Thank you.

@Jackenmen Jackenmen deleted the add_cirrus_ci_support branch August 30, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for native Linux arm64 builds on Cirrus CI Support for Cirrus CI
5 participants