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: use macos-14, add new aarch64-apple-darwin release binary #2895

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented Jan 30, 2024

GitHub announced these new Apple Silicon based runners today. Let's take them for a spin.

Let's also add an entry in the release matrix to build and publish aarch64- apple-darwin binaries, too. This doesn't migrate the old release matrix entry; it still uses a macos-11 runner. This means the x86 binaries should work on a few older macOS versions if users need it.

This should also close #1887, since these runners are no longer considered to be in beta.

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 self-assigned this Jan 30, 2024
@ilyagr
Copy link
Contributor

ilyagr commented Jan 30, 2024

Thanks!

Have you double-checked that the release workflow works? You can e.g. create a release in your fork. If not, I can try to check it.

The x86 version of jj does work with Rosetta, but an aarch64 binary is nicer. For example, cargo binstall should then work even if Rosetta is not installed.

For future: Is there a way to run tests on aarch64 as well? We could also consider a universal binary, but

Aside: cargo-binstall uses https://github.com/cross-rs/cross for cross-compilation. I was wondering whether to use that for an aarch64-linux binary, but I would guess it's slow (it runs cargo in a VM, I think).

@thoughtpolice
Copy link
Member Author

thoughtpolice commented Jan 30, 2024

Have you double-checked that the release workflow works? You can e.g. create a release in your fork. If not, I can try to check it.

No, I was going to take it for a spin later before bugging Martin to approve this, but if you can kick the tires on it for me, I'd appreciate it!

For future: Is there a way to run tests on aarch64 as well? We could also consider a universal binary, but

You mean macOS x86_64? This PR migrates the actual macOS CI build (used for every PR) to use aarch64, I believe macos-14 is only aarch64, so it is running the tests on aarch64 by definition from now on. Which I think is fine.

What we might be able to do is kill two birds with one stone and run a parallel build job that runs the job's shell under Rosetta and thus builds an x86_64 binary and tests it. That would give us pretty good confidence that we have all bases covered. EDIT: But it would probably be really annoyingly slow, if I had to guess, so maybe not worth it.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 30, 2024

I think the two "build"s in release.yml need to have different names. Only aarch64 seems to currently be built.

https://github.com/ilyagr/jj/actions/runs/7717709786/job/21037387946
https://github.com/ilyagr/jj/releases/tag/0.13.1-aarch64test

(The builds are not quite finished as I'm posting this)

@thoughtpolice thoughtpolice force-pushed the aseipp/push-qsxtmxzslxkn branch from 1b5a578 to 6a2023c Compare January 30, 2024 21:40
@thoughtpolice
Copy link
Member Author

Good catch, pushed a fix.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 30, 2024

@ilyagr
Copy link
Contributor

ilyagr commented Jan 30, 2024

You could also put this in the changelog ("Official darwin-aarch64 binaries are now available")

GitHub announced these new Apple Silicon based runners today. Let's take them
for a spin.

Let's also add an entry in the release matrix to build and publish `aarch64-
apple-darwin` binaries, too. This doesn't migrate the old release matrix entry;
it still uses a `macos-11` runner. This means the x86 binaries should work on a
few older macOS versions if users need it.

Signed-off-by: Austin Seipp <[email protected]>
@thoughtpolice thoughtpolice force-pushed the aseipp/push-qsxtmxzslxkn branch from 6a2023c to e7520d4 Compare January 30, 2024 21:56
@ilyagr

This comment was marked as outdated.

Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

The binaries seem to work, except for a new bug I discovered: #2896. That bug is already present in jj v0.13

You mean macOS x86_64? This PR migrates the actual macOS CI build (used for every PR) to use aarch64, I believe macos-14 is only aarch64, so it is running the tests on aarch64 by definition from now on. Which I think is fine.

Indeed, the tests run on aarch64 now. I'm not sure whether it's good enough to only run aarch64 tests at this point, but it might be OK. We could also add a macos-13 runner.

I'll approve this, but we'll need @martinvonz to look at this regardless. This needs changes to required CI checks (one got renamed).

@martinvonz
Copy link
Member

I'll approve this, but we'll need @martinvonz to look at this regardless. This needs changes to required CI checks (one got renamed).

Thanks for reviewing and testing!

@martinvonz
Copy link
Member

And thanks for the PR, @thoughtpolice! As you can see, I've updated the branch protections so you can merge it.

@thoughtpolice thoughtpolice merged commit 6be5cf3 into main Jan 30, 2024
15 checks passed
@thoughtpolice thoughtpolice deleted the aseipp/push-qsxtmxzslxkn branch January 30, 2024 22:41
ilyagr added a commit to ilyagr/jj that referenced this pull request May 3, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 3, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 3, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 6, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 6, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 20, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 24, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 24, 2024
ilyagr added a commit to ilyagr/jj that referenced this pull request May 25, 2024
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.

Move macos CI runner back to macos-latest when Ventura is out of beta
3 participants