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 ARM Big Sur CI #67577

Merged
merged 1 commit into from
Dec 26, 2020
Merged

Add ARM Big Sur CI #67577

merged 1 commit into from
Dec 26, 2020

Conversation

fxcoudert
Copy link
Member

This will add Apple Silicon to our CI for all homebrew-core pull requests. We currently have 1046 ARM bottles, so at this point we need to enable it in CI.

However, there are also a number of formulas that currently fail to build on ARM, and we don't want that to block everything else. The approach I propose is simple and (I think) efficient: make it so ARM is skipped during testing if CI-skip-arm label is applied. This requires maintainer intervention, but allow us to make a conscious decision.

I need help, however, to achieve this: I don't know how to make the 11-arm entry in the test matrix be conditional on needs.tap_syntax.outputs.skip-arm.

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Dec 23, 2020
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@Rylan12 Rylan12 added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Dec 23, 2020
@SeekingMeaning
Copy link
Contributor

SeekingMeaning commented Dec 24, 2020

I don't know how to make the 11-arm entry in the test matrix be conditional on needs.tap_syntax.outputs.skip-arm.

AFAIK this isn't possible—we'll have to either (1) duplicate the tests job (not to be confused with the tests.yml workflow) with ARM Big Sur by itself or (2) make a new workflow similar to the tests-linux.yml and wheezy_tests.yml workflows

@Rylan12
Copy link
Member

Rylan12 commented Dec 24, 2020

Why do we even need a skip-arm label? If it can't be built on arm, shouldn't we be able to say something like: depends_on: :intel or something like that (not sure what terminology is appropriate)?

AFAIK this isn't possible—we'll have to either (1) duplicate the tests job (not to be confused with the tests.yml workflow) with ARM Big Sur by itself or (2) make a new workflow similar to the tests-linux.yml and wheezy_tests.yml workflows

There's a third option: have the arm CI job simply skip each step individually. The job will be listed as a success but no steps in the job would have actually been run.

@SeekingMeaning
Copy link
Contributor

SeekingMeaning commented Dec 24, 2020

There's a third option: have the arm CI job simply skip each step individually. The job will be listed as a success but no steps in the job would have actually been run.

Eh, that seems too hacky

Here's what I propose: SeekingMeaning@903e48c049

@richiksc
Copy link
Contributor

@fxcoudert I know this is slightly off-topic, but how were you able to add your Apple Silicon Macs to GitHub Actions as a self-hosted runner? It seems that GitHub Actions only allows you to select the x64 architecture for macOS when creating a self-hosted runner.

@MikeMcQuaid
Copy link
Member

This will add Apple Silicon to our CI for all homebrew-core pull requests. We currently have 1046 ARM bottles, so at this point we need to enable it in CI.

However, there are also a number of formulas that currently fail to build on ARM, and we don't want that to block everything else. The approach I propose is simple and (I think) efficient: make it so ARM is skipped during testing if CI-skip-arm label is applied. This requires maintainer intervention, but allow us to make a conscious decision.

I need help, however, to achieve this: I don't know how to make the 11-arm entry in the test matrix be conditional on needs.tap_syntax.outputs.skip-arm.

I'd like to propose an alternate approach in Homebrew/homebrew-test-bot#543

I suggest we set this new variable for tests.yml but don't set it on dispatch-build-bottle.yml. This allows us to prevent bottles from regressing or not being pulled while not wasting CI time building where we've not yet had a successful build yet.

@@ -65,7 +85,7 @@ jobs:
if: github.event_name == 'pull_request' && needs.tap_syntax.outputs.run-tests
strategy:
matrix:
version: ['11.0', '10.15', '10.14']
version: ['11-arm', '11.0', '10.15', '10.14']
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be a pain but: could this be updated to 11-arm64 on these workers and, even better still, use 11 instead of 11.0 for Big Sur at some point too (this could be separate tasks and I can open an issue for tracking if easier). This will allow simplifying some code in Homebrew/brew.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we need these to be homogeneous at some point, but it's not gonna happen until we are updating the nodes, because it's too much of a pain to do

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, no rush but when the nodes are next updated it'd be much appreciated if you could handle this for the ARM ones (and @Rylan12 @jonchang @Bo98 or whomever handles the Big Sur ones).

Copy link
Member

Choose a reason for hiding this comment

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

(it'd be OK to apply both labels for now so code changes aren't needed to have it keep working)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Our runners are already running 11.1 so the names are already out of date 😅. Also agree that this doesn't need to happen immediately. Might be good to get some PRs open to get ready for the next time the nodes are updated.

In terms of things that need to be modified in the brew repo, it looks like it's pretty much just dispatch-build-bottle, right? Looks like the only part that would need to change is the label mapping. And then, once this is complete, this mapping can be removed as well.

I won't be able to do the actual renaming as I'm not an organization owner. Happy to help out with the dispatch-build-bottle and whatever other code changes are needed, though.

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

HOMEBREW_CHANGE_ARCH_TO_ARM just means that brew will re-exec itself without rosetta 2, right?

If so, LGTM ✅👍

@fxcoudert
Copy link
Member Author

Yes, the github actions runner software has to run under Rosetta 2, because it's not yet supported on aarch64-darwin. So brew has to re-exec under arm, which is conditional on HOMEBREW_CHANGE_ARCH_TO_ARM.

@fxcoudert
Copy link
Member Author

I'm gonna hold on merging this until the ARM bottling queue is quieter, because I've submitted 600 jobs to it 😇

@Rylan12
Copy link
Member

Rylan12 commented Dec 25, 2020

I'm gonna hold on merging this until the ARM bottling queue is quieter, because I've submitted 600 jobs to it 😇

Makes sense. I wouldn't wait too long, though (once the queue is reasonable) because normal version bumps are starting to remove arm bottles. Don't want to duplicate too much work.

@fxcoudert
Copy link
Member Author

@Rylan12 yeah I know, I have a script that alerts me to bottle removals and rebuilds them :)

@fxcoudert
Copy link
Member Author

We are at 3168 ARM bottles this morning (70% of the number we have on Intel Big Sur).

@fxcoudert fxcoudert merged commit 2b7c02a into Homebrew:master Dec 26, 2020
@fxcoudert fxcoudert deleted the arm-tests branch December 26, 2020 10:41
@fxcoudert
Copy link
Member Author

The problem with that approach is that we're not allowing to test new fixes for ARM. Take this PR #67716 or this one #67758, they would need to run on ARM to test the fix being introduced, but they're not. We need a way to opt-in to ARM testing for a given PR.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 26, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants