Build aarch64-apple-darwin binaries on CI #6989

lnicola · 2020-12-21T19:07:46Z

This splits the dist matrix job into four and tries to make xtask dist more principled about target and artifact naming.

lnicola · 2020-12-21T19:09:29Z

I have no idea if this will work, but I couldn't figure how to trigger the action on my fork.

.github/workflows/release.yaml

richiksc · 2020-12-21T20:12:37Z

I have no idea if this will work, but I couldn't figure how to trigger the action on my fork.

@lnicola I guess you could get the action to trigger by merging this change into trigger-nightly on your fork?

lnicola · 2020-12-21T20:17:05Z

I think you can also trigger them manually, but actions don't usually run on forks.

lnicola · 2020-12-21T20:20:40Z

Oh, I had to push it to the master branch, then wait for a while.

richiksc · 2020-12-21T20:34:07Z

@lnicola Looks like it's building on your fork after pushing to trigger-nightly: https://github.com/lnicola/rust-analyzer/runs/1591171874

richiksc · 2020-12-21T20:53:37Z

Hmm, looks like the build failed during linking. I think it's because compilation for aarch64-apple-darwin needs to use the Xcode 12 macosx11.0 SDK, but right now, it's using the macosx10.15 SDK by default.

https://github.com/lnicola/rust-analyzer/runs/1591171874#step:9:477

= note: ld: warning: ignoring file /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/lib/libresolv.tbd, missing required architecture arm64 in file /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/lib/libresolv.tbd

There are two ways around this: you can manually select the macOS 11 SDK, according to shepmaster's instructions:

SDKROOT=$(xcrun -sdk macosx11.0 --show-sdk-path) \
MACOSX_DEPLOYMENT_TARGET=$(xcrun -sdk macosx11.0 --show-sdk-platform-version) \
cargo build --target=aarch64-apple-darwin

Or you can set GitHub Actions to run on the macos-11.0 runner, which is what I did on a simple Rust command-line tool project I contributed to. That should require no extra configuration to cross-compile for aarch64-apple-darwin.

jobs:
  dist:
    name: dist
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
-        os: [ubuntu-16.04, windows-latest, macos-latest]
+        os: [ubuntu-16.04, windows-latest, macos-11.0]

richiksc · 2020-12-21T20:54:26Z

Also, will the necessary code change to code/src/main.ts be in this PR or another PR if this gets merged? platform would need to be set to "mac-arm", I'm guessing.

lnicola · 2020-12-21T21:28:10Z

Thanks for looking it up -- I had to step out for a bit. I do wonder if the macos-11.0 binaries will be compatible with whatever macos-latest works on.

Also, will the necessary code change to code/src/main.ts be in this PR or another PR if this gets merged? platform would need to be set to "mac-arm", I'm guessing.

Yeah, I planned to do it in a different PR, after making sure that the binaries are built fine.

richiksc · 2020-12-21T23:36:33Z

@lnicola Looks like the binaries built on CI: https://github.com/lnicola/rust-analyzer/releases/tag/nightly

lnicola · 2020-12-22T04:12:00Z

I don't think they did, there's no ARM binary. Maybe it used the code from the wrong branch? EDIT: ah, the real nightly kicked in and overwritten the release.

Anyway, I suppose it would be better to restructure the workflow to build them in parallel.

richiksc · 2020-12-22T05:48:44Z

I don't think they did, there's no ARM binary. Maybe it used the code from the wrong branch? EDIT: ah, the real nightly kicked in and overwritten the release.

Anyway, I suppose it would be better to restructure the workflow to build them in parallel.

Yep, the real nightly kicked in. I wish there was a way for you to tag it to build as separate. Potentially add an on: push: tags: build-* or something and push a tag, and then have the release action push it to that tag.

I don't know if there's a way to build them in parallel in the same job. Wouldn't it take more time to set up another whole macOS runner just to build the additional binary?

lnicola · 2020-12-22T07:19:53Z

@richiksc if you don't mind, can you download a rust-analyzer-mac-arm file from https://github.com/lnicola/rust-analyzer/releases/tag/nightly and give it a spin? If you're having issues with Code, you can run it from the command line, e.g. rust-analyzer-mac-arm analysis-stats . in the RA repo (testing both of the x86-64 and aarch64 versions might be a nice benchmark).

I don't know if there's a way to build them in parallel in the same job. Wouldn't it take more time to set up another whole macOS runner just to build the additional binary?

In separate jobs, I meant. Not sure if it's better to use a single job or two, but it might make sense to have two. I tried to implement that (untested, I don't want to trigger another release until someone tests the latest one)

lnicola · 2020-12-22T07:52:38Z

xtask/src/dist.rs


+    let toolchain = get_toolchain(&target);
+    cmd!("cargo +{toolchain} build --manifest-path ./crates/rust-analyzer/Cargo.toml --bin rust-analyzer --target {target} --release").run()?;


I wonder if this works..

THe +{toolchain} syntax? Yeah, it should work

matklad

Nice, thanks for looking into this, @lnicola !

As a super-minor stylistic changes, I am not a fan of get_ / compute_ / find_ preffixes on function names (get_executable_extension). I feel that they add zero additional information, and can be removed.

It might be argueed that let ext = get_extension(target) reads better than let ext = extension(target), but that depends on how do you look at the code.

If you look at it as action, then, yes, first one is better. But you can also see this as a predicate/assetion -- ext is extension of target. In that second reading, get prefix is not required.

To be clear, this is absolutely irrelevant in practice :-)

matklad · 2020-12-22T08:51:14Z

xtask/src/dist.rs


+    let toolchain = get_toolchain(&target);
+    cmd!("cargo +{toolchain} build --manifest-path ./crates/rust-analyzer/Cargo.toml --bin rust-analyzer --target {target} --release").run()?;


THe +{toolchain} syntax? Yeah, it should work

matklad · 2020-12-22T08:52:48Z

xtask/src/dist.rs

+    }
+}
+
+fn get_executable_extension(target: &str) -> String {


Suggested change

fn get_executable_extension(target: &str) -> String {

fn get_executable_suffix(target: &str) -> String {

https://doc.rust-lang.org/std/env/consts/constant.EXE_SUFFIX.html

extensioni s a thing without ..

matklad · 2020-12-22T08:55:42Z

xtask/src/dist.rs

+    }
+}
+
+fn get_release_artifact_name(target: &str) -> String {


So the plan here is to add full target tripple to the executable name? With two different macs that makes sense actually, 👍.

For the transition, we should adjust vscode, and broadcast the change to the clients issue. We might also just publish to sets of artifacts? THat is, run cp rust-analyzer-windows.exe rust-analyzer-x86_64-pc-windows-msvc.exe in `.yaml.

So the plan here is to add full target tripple to the executable name? With two different macs that makes sense actually, +1.

We also had requests for Windows ARM binaries, so using the target tuple seems reasonable.

For the transition, we should adjust vscode, and broadcast the change to the clients issue. We might also just publish to sets of artifacts? THat is, run cp rust-analyzer-windows.exe rust-analyzer-x86_64-pc-windows-msvc.exe in `.yaml.

I did it the other way around, but in xtask.

matklad · 2020-12-22T09:22:41Z

r=me

lnicola · 2020-12-22T09:23:20Z

As a super-minor stylistic changes, I am not a fan of [...]

No problem, I shortened those, maybe even too much. I left get_target unchanged since it was different. Arguably, it could be called pick_target or something else, but I'd rather we passed RA_TARGET explicitly.. actually let me to just that. EDIT: no, I already tried it -- the problem is that I don't know how to determine the current target tuple (as a fallback, in case you want to run cargo xtask dist locally without bothering with the env variable). ~~I'll use CARGO_BUILD_TARGET.~~ That doesn't work either unless we're in a build script.

lnicola · 2020-12-22T09:40:58Z

I triggered another test run (sorry), to make sure the workflow changes still work.

lnicola · 2020-12-22T11:50:40Z

https://github.com/lnicola/rust-analyzer/releases/tag/nightly 🎉

bors r=matklad

lnicola · 2020-12-22T12:01:45Z

bors seems stuck again (I think I've seen it merge workflows changes once), I'll merge manually.

6989: Build aarch64-apple-darwin binaries on CI r=matklad a=lnicola This splits the `dist` matrix job into four and tries to make `xtask dist` more principled about target and artifact naming. Co-authored-by: Laurențiu Nicola <[email protected]>

bors · 2020-12-22T13:19:23Z

Build succeeded:

richiksc · 2020-12-22T17:54:42Z

@richiksc if you don't mind, can you download a rust-analyzer-mac-arm file from https://github.com/lnicola/rust-analyzer/releases/tag/nightly and give it a spin? If you're having issues with Code, you can run it from the command line, e.g. rust-analyzer-mac-arm analysis-stats . in the RA repo (testing both of the x86-64 and aarch64 versions might be a nice benchmark).

@lnicola I wasn't able to test this earlier due to my timezone. I'd love to test the binary today but I can't see a release on your fork anymore - was it deleted for some reason? I can definitely run the benchmark in the rust-analyzer repo if necessary.

lnicola · 2020-12-22T18:06:00Z

Oof, I deleted the branch and they're gone. I triggered a new build, but they don't seem to have a lot of macos-11.0 runners, so it's gonna take a while (~40 minutes) to run. I'll ping you.

I can definitely run the benchmark in the rust-analyzer repo if necessary.

It's not necessary, of course :-).

lnicola · 2020-12-22T19:05:47Z

@richiksc I think it's ready

richiksc · 2020-12-22T20:10:41Z

@lnicola Hmm, the release seems to have both binaries with the old naming scheme ("rust-analyzer-mac") and the new naming scheme ("rust-analyzer-x86_64-apple-darwin") - are you building it twice?

matklad · 2020-12-22T20:12:10Z

both names are essential, to give time for non-vs-code clients to migrate to new names

…

On Tue, 22 Dec 2020 at 23:10, Richik SC ***@***.***> wrote: @lnicola <https://github.com/lnicola> Hmm, the release seems to have both binaries with the old naming scheme ("rust-analyzer-mac") and the new naming scheme ("rust-analyzer-x86_64-apple-darwin") - are you building it twice? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6989 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M2IR2EO7ECPLYA2X6DSWD4NFANCNFSM4VETS3JA> .

lnicola · 2020-12-22T20:16:55Z

are you building it twice

No, it's just making a copy.

richiksc · 2020-12-22T20:22:28Z

@lnicola Ok, I've ran the benchmark.

Here's the results (13" MacBook Pro, M1, Late 2020, 16GB RAM):

rust-analyzer-aarch64-apple-darwin (ARM Native)

% ./rust-analyzer-aarch64-apple-darwin analysis-stats .
Database loaded 641.27ms
Crates in this dir: 36
Total modules found: 572
Total declarations: 11043
Total functions: 8630
Item Collection: 7.37s
30/8630 100% processing: logger::flushTotal expressions: 236758                nExpressions of unknown type: 2669 (1%)                             
Expressions of partially unknown type: 2053 (0%)
Type mismatches: 857
Inference: 14.35s
Total: 21.72s

rust-analyzer-x86_64-apple-darwin (Rosetta 2)

% ./rust-analyzer-x86_64-apple-darwin analysis-stats . 
Database loaded 913.53ms
Crates in this dir: 36
Total modules found: 572
Total declarations: 11043
Total functions: 8630
Item Collection: 11.22s
30/8630 100% processing: logger::flushTotal expressions: 236758                nExpressions of unknown type: 2669 (1%)                             
Expressions of partially unknown type: 2053 (0%)
Type mismatches: 857
Inference: 22.46s
Total: 33.68s

As you can see, a significant 35% reduction in analysis time just by being ARM native.

This is the second run of the x86_64 binary, as the first time, it just hung. I'm guessing it was because Rosetta 2 was performing AOT translation of it. It reported Database loaded 22s after killing it with ^C.

lnicola · 2020-12-22T20:24:37Z

Then it works! Can you also use it with Code? You can set the server path in the settings.

richiksc · 2020-12-22T20:33:03Z

Then it works! Can you also use it with Code? You can set the server path in the settings.

@lnicola Yep, works great on the arm64 native Insiders build of VSCode!

Edit: this is the rust-analyzer extension v0.2.424 downloaded from the VSCode Extension Marketplace - I didn't load the nightly vsix from your release.

lnicola force-pushed the apple-arm branch from a5fe9ef to b30b0dd Compare December 21, 2020 19:11

lnicola mentioned this pull request Dec 21, 2020

Ship binaries for Apple Silicon Macs (aarch64-apple-darwin) #6732

Closed

richiksc suggested changes Dec 21, 2020

View reviewed changes

.github/workflows/release.yaml Show resolved Hide resolved

lnicola force-pushed the apple-arm branch from b30b0dd to 980da6b Compare December 21, 2020 19:42

richiksc approved these changes Dec 21, 2020

View reviewed changes

lnicola force-pushed the apple-arm branch 2 times, most recently from 359beb5 to 45e4f25 Compare December 22, 2020 07:37

lnicola commented Dec 22, 2020

View reviewed changes

matklad approved these changes Dec 22, 2020

View reviewed changes

lnicola force-pushed the apple-arm branch from 45e4f25 to 22c13f8 Compare December 22, 2020 09:20

lnicola force-pushed the apple-arm branch from 22c13f8 to 08154eb Compare December 22, 2020 09:37

lnicola force-pushed the apple-arm branch 4 times, most recently from 55173a3 to 3f7e904 Compare December 22, 2020 10:17

Build aarch64-apple-darwin binaries on CI

e881815

lnicola force-pushed the apple-arm branch from 3f7e904 to e881815 Compare December 22, 2020 11:18

lnicola merged commit 02d8cf8 into rust-lang:master Dec 22, 2020

lnicola deleted the apple-arm branch December 22, 2020 12:01


		let toolchain = get_toolchain(&target);
		cmd!("cargo +{toolchain} build --manifest-path ./crates/rust-analyzer/Cargo.toml --bin rust-analyzer --target {target} --release").run()?;

	fn get_executable_extension(target: &str) -> String {
	fn get_executable_suffix(target: &str) -> String {

Build aarch64-apple-darwin binaries on CI #6989

Build aarch64-apple-darwin binaries on CI #6989

Uh oh!

Conversation

lnicola commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Dec 21, 2020

Uh oh!

Uh oh!

richiksc commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Dec 21, 2020

Uh oh!

lnicola commented Dec 21, 2020

Uh oh!

richiksc commented Dec 21, 2020

Uh oh!

richiksc commented Dec 21, 2020

Uh oh!

richiksc commented Dec 21, 2020

Uh oh!

lnicola commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richiksc commented Dec 21, 2020

Uh oh!

lnicola commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richiksc commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

matklad Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

matklad Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

matklad Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

matklad Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

lnicola Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

lnicola Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

matklad commented Dec 22, 2020

Uh oh!

lnicola commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Dec 22, 2020

Uh oh!

lnicola commented Dec 22, 2020

Uh oh!

lnicola commented Dec 22, 2020

Uh oh!

bors bot commented Dec 22, 2020

Uh oh!

richiksc commented Dec 22, 2020

Uh oh!

lnicola commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Dec 22, 2020

lnicola commented Dec 21, 2020 •

edited

Loading

richiksc commented Dec 21, 2020 •

edited

Loading

lnicola commented Dec 21, 2020 •

edited

Loading

lnicola commented Dec 22, 2020 •

edited

Loading

richiksc commented Dec 22, 2020 •

edited

Loading

lnicola commented Dec 22, 2020 •

edited

Loading

lnicola commented Dec 22, 2020 •

edited

Loading

lnicola commented Dec 22, 2020 •

edited

Loading

richiksc commented Dec 22, 2020 •

edited

Loading