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

github: build on macos-13 for x86_64 #4243

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented Aug 9, 2024

We all noticed that x86 macOS binaries are no longer being provided on release, due to macos-11 runners going the way of the Dodo a while back. Nobody alterted us to this, funny enough.

After some quick discussion, we concluded some things:

  • x86 macOS runners are likely oversubscribed, and hurt CI latency badly
  • macos-12 is also deprecated; macos-13 is the best x86 runner available
  • GitHub probably isn't going to expand macOS runner capacity; macos-13 will one day go away
  • Some people are still using jj on Intel Macs. We didn't get alerted because they do their own builds for now, but may not always do that.
  • We can just try to build on macos-13 and make it optional for merges.

So that's what this does. It might be mergeable outright, but we can also use it to measure build latency impacts.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@thoughtpolice thoughtpolice marked this pull request as draft August 9, 2024 00:31
@ilyagr
Copy link
Contributor

ilyagr commented Aug 9, 2024

Thank you Austin!

It looks like the macos-13 runners are not marked as required and ran in 6 minutes. I think we can just merge this?

@martinvonz
Copy link
Member

It looks like the macos-13 runners are not marked as required and ran in 6 minutes. I think we can just merge this?

But we probably want to mark them as required once this is in, right?

@ilyagr
Copy link
Contributor

ilyagr commented Aug 9, 2024

But we probably want to mark them as required once this is in, right?

I'm not sure. Austin was worried that these tests might be slow to get runners, since Intel Mac runners might be (or may become) a scarce resource. If that's the case, it might make sense to keep them optional.

For me, the important thing is that the tests run regularly so that we eventually become aware if they stop working. For example, we'll now know when GitHub removes these runners before a release fails to build.

@thoughtpolice thoughtpolice force-pushed the aseipp/push-zsqppmzpsnrs branch from 2d17a2b to 799aa7d Compare August 16, 2024 18:44
@thoughtpolice thoughtpolice marked this pull request as ready for review August 16, 2024 18:44
@thoughtpolice thoughtpolice self-assigned this Aug 16, 2024
@thoughtpolice thoughtpolice changed the title github: try building on macos-13 (x86) github: build on macos-13 for x86_64 Aug 16, 2024
@thoughtpolice thoughtpolice force-pushed the aseipp/push-zsqppmzpsnrs branch from 799aa7d to 7fccdbe Compare August 16, 2024 18:48
@thoughtpolice
Copy link
Member Author

thoughtpolice commented Aug 16, 2024

Let's just go ahead and move forward with adding macos-13 everywhere and see how it goes. I'll remind Martin to update the "required" list in a week or two if nothing goes wrong or feels bad.

We all noticed that x86 macOS binaries are no longer being provided on release,
due to `macos-11` runners going the way of the Dodo a while back. Nobody
alterted us to this, funny enough.

After some quick discussion, we concluded some things:

- x86 macOS runners are likely oversubscribed, and hurt CI latency badly
- `macos-12` is also deprecated; `macos-13` is the best x86 runner available
- GitHub probably isn't going to expand macOS runner capacity; `macos-13` will
one day go away
- Some people are still using `jj` on Intel Macs. We didn't get alerted because
they do their own builds for now, but may not always do that.
- We can just try to build on `macos-13` and make it optional for merges.

So that's what this does. It might be mergeable outright, but we can also use it
to measure build latency impacts.

Signed-off-by: Austin Seipp <[email protected]>
@thoughtpolice thoughtpolice force-pushed the aseipp/push-zsqppmzpsnrs branch from 7fccdbe to 376f39a Compare August 16, 2024 18:52
@thoughtpolice thoughtpolice merged commit 5eab5c8 into main Aug 16, 2024
31 checks passed
@thoughtpolice thoughtpolice deleted the aseipp/push-zsqppmzpsnrs branch August 16, 2024 19:23
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.

4 participants