Skip to content

go: Apple Silicon updates #65696

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

Closed
wants to merge 3 commits into from

Conversation

abarisain
Copy link

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Hello!

I have a M1 computer, and am running macOS 11.1 beta. I am fully aware that this is a highly unsupported combination.

Up until today, macOS 11.1 forced homebrew to build everything from source, even in Rosetta mode.
Unfortunately, the Go formula fails to build in Rosetta. Same as Homebrew/brew#7857 : Bootstrapped Go crashes.

This has since been fixed by the use of bottles, but as Go is a bootstrapped language, I believe that fixing Rosetta Go makes for preparation for when Go 1.16 will be released.

The first commit updates to to the lowest version that successfully compiles Go on Rosetta.
I noticed that homebrew uses the lowest possible Go version. As I'm not familiar with the rationale of that decision, I tried to preserve this by starting with 1.7 and stopping when I got a working build.
However, as Rosetta is a moving target, I think that Homebrew should play it safe and use 1.14. We could even use 1.15, but it's kind of weird to compile 1.15 with go 1.15.

The formula explicitly says not to update the bootstrapped Go unless it is needed. I'm uneasy with this change as it is not strictly required on Intel machines (which will bottle a Go that works just fine under Rosetta).
Apple might fix rosetta down the road: this is why I'm using 11.1, which has Rosetta improvements.

This PR also has a second commit, that sets GODEBUG=asyncpreemptoff=1. This works around an issue where Goroutines will get stuck on Rosetta.
On my Intel machine, it doesn't affect the build time (which is around 1 minute anyway), and doesn't leak into the compiled go as it is a runtime flag.

I attached my logs from my M1 computer. Note that "ibrew" is a simple alias for "arch -x86_64 /usr/local/bin/brew".
brew tests fails on my machines because I'm using macOS 11 (even on a .0 intel machine), but it also fails on master so I ignored this. Hope it's not a problem.
Other tests (audit, style, test) are all fine.
brew test go requires the same env var as the second commit adds, but this is just a quirk of running Go in Rosetta.

Note that I've successfully built a native arm Go (bootstrapped by 1.15) by applying the patch mentioned here: golang/go#42684 (comment)
With this PR merged, Go 1.16 will probably work with the formula.

Have a nice day,
output.txt

On macOS 11.1 beta 1, this is the lowest version of Go that can successfully compile Go 1.15.
This makes Go hang on Apple Silicon in Rosetta. This doesn't slow build times down meaningfully.
@abarisain abarisain force-pushed the arm-fix-go-bootstrap branch from 4d8b46b to 9e5510d Compare November 26, 2020 10:56
Formula/go.rb Outdated
@@ -54,6 +54,8 @@ def install

cd "src" do
ENV["GOROOT_FINAL"] = libexec
# Required for Apple Silicon Rosetta: Without it, Go will hang while building
ENV["GODEBUG"] = "asyncpreemptoff=1"
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to limit this… I don't think we have a way to switch for Rosetta only, so if MacOS.version >= :big_sur is probably good

Copy link
Author

@abarisain abarisain Nov 26, 2020

Choose a reason for hiding this comment

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

Apple has a way to detect rosetta, but I'm not familiar enough with Ruby to implement it myself:
https://developer.apple.com/documentation/apple_silicon/about_the_rosetta_translation_environment?language=objc

This might make sense as a general Homebrew helper, if you want to open that door. Note that we'd also need to check for arm as even if Homebrew runs natively, the bootstrapped compiler will need Rosetta for a while.

Copy link
Author

Choose a reason for hiding this comment

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

Added your suggestion! Not sure if I was supposed to wait for another maintainer.

url "https://storage.googleapis.com/golang/go1.7.darwin-amd64.tar.gz"
sha256 "51d905e0b43b3d0ed41aaf23e19001ab4bc3f96c3ca134b48f7892485fc52961"
url "https://golang.org/dl/go1.12.17.darwin-amd64.tar.gz"
sha256 "a2f58b4bf10f3e882c1a43eba27a2aba65fc815384fbdfc46331c13bca5f5f24"
Copy link
Member

Choose a reason for hiding this comment

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

I have no objection to bootstrapping with a recent compiler. We'll probably need a recent version for ARM anyway

Copy link
Author

Choose a reason for hiding this comment

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

I haven't tried to build an ARM go with this compiler. I can get back to you with the lowest version that compiles the patched Go trunk later!

Copy link
Author

Choose a reason for hiding this comment

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

I can confirm that 1.12.17 can build a native arm64 go.

Building Go cmd/dist using /Users/arnaud/go1.4. (go1.12.17 darwin/amd64)
Building Go toolchain1 using /Users/arnaud/go1.4.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for host, darwin/amd64.
Building packages and commands for target, darwin/arm64.
---
Installed Go for darwin/arm64 in /Users/arnaud/Desktop/goarm
Installed commands in /Users/arnaud/Desktop/goarm/bin

~/D/g/src git:() ➜ ../bin/darwin_arm64/go version
go version devel +7716a2fbb7 Sun Nov 22 11:44:49 2020 -0500 darwin/arm64

bin/go is a amd64 build for some reason, but it's probably something that should wait until 1.16 is released, they might change some stuff.

Copy link
Member

Choose a reason for hiding this comment

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

bin/go is a amd64 build for some reason

Then it's not a native arm64 go, is it? I don't understand that contradiction.

Copy link
Author

@abarisain abarisain Nov 28, 2020

Choose a reason for hiding this comment

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

I failed to mention this in my original PR, but this PR itself isn't sufficient to build a native arm Go: this will have to wait for 1.16.

Which is why it's probably best if it's not merged until 1.16 (but this can be used as a reference of what needs to be done). I wanted to explore why bootstraped Go was crashing (as reported in Homebrew/brew#7857), and prepare some of the investigative work needed for 1.16.

Then it's not a native arm64 go, is it? I don't understand that contradiction.

I don't get that either, but if you build with this command, you get:

  • amd64 go in bin/go
  • arm64 go in bin/darwin_arm64/go

bin/darwin_arm64/ contains a fully working distribution that works perfectly if you move it somewhere and set it as your GOROOT: I've been using that for a couple days now.
Not sure why an amd go gets built in bin/ but I'm not really familiar with Go's make.bash and bootstrap.bash. It may be a simple build script difference between bootstrap.bash and make.bash?
It might require another change in the buildscript on arm though.

Edit:

Ah, I think that this happens because we're actually cross compiling to another arch, as shown by those two log lines:

Building packages and commands for host, darwin/amd64.
Building packages and commands for target, darwin/arm64.

It's annoyingly hard not to confuse arm64 and amd64 if you're not paying extra attention when reading. Thought those lines were the same at first

@abarisain
Copy link
Author

Force pushed to rewrite the commit names (a newline broke)

…g sur

Ideally this should only be set when on native arm or rosetta.
@abarisain abarisain requested a review from fxcoudert November 27, 2020 22:45
@fxcoudert
Copy link
Member

  • Re ENV["GODEBUG"] = "asyncpreemptoff=1": if the fix is for Rosetta only, I don't think we actually want it. Our bottles are built on Intel systems, so that's not an issue there. Rosetta users should install from bottles.
  • If we wanted to support build-from-source-on-Rosetta we'd be spending a lot of time supporting weird quirks and bugs.

@abarisain
Copy link
Author

abarisain commented Nov 28, 2020

Agreed, I'm not looking to build intel stuff from source on rosetta.

But this will prevent a native arm go from being compiled on arm as this env var is required for the boostrapped Go to run under Rosetta. Clearly, if this workaround is applied, it should only be if we're running a native arm homebrew.
I guess we could bootstrap Go with an official Go 1.16 arm build once it's out, but I find it a little weird to compile 1.16 with 1.16.

This is purely work that is to prepare for go 1.16: That can be delayed and reevaluated for when Go 1.16 is actually out, as this change alone won't produce a woring arm Go 1.15. I just wanted to give more info and a potential fix as the big "homebrew arm" issue stated that bootstrapped go crashed and that was it (maybe we can close this pr and update the issue with a link to this thread?).

Since HB fixed 11.1 not using 11.0 bottles, I don't really need this fix to be merged until 1.16 is out.

Thanks for your time!

@fxcoudert
Copy link
Member

I'm closing, we'll deal with this when we have a working ARM version (1.16)

@felixbuenemann
Copy link
Contributor

The ENV["GODEBUG"] = "asyncpreemptoff=1" hack is no longer needed on 11.1 Beta 20C5061b.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 5, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants