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

go.rb: use latest stable release for Go source installs #71370

Closed
1 task done
kevinburke1 opened this issue Feb 17, 2021 · 14 comments
Closed
1 task done

go.rb: use latest stable release for Go source installs #71370

kevinburke1 opened this issue Feb 17, 2021 · 14 comments
Labels
bug Reproducible Homebrew/homebrew-core bug outdated PR was locked due to age

Comments

@kevinburke1
Copy link

kevinburke1 commented Feb 17, 2021

brew gist-logs <formula> link OR brew config AND brew doctor output

Not relevant for this issue.


  • The brew doctor above contains no "Warning" lines.

What were you trying to do (and why)?

Since Go 1.4, you need a working Go compiler to compile Go from source, a process called bootstrapping. To build Go 1.16 from source, we download and bootstrap with Go 1.7. However, there's no reason Go 1.7 must be used, and it's possible that using 1.7 generates inefficient machine code compared with the most recent release. I personally use the second most recent released version to bootstrap the current version.

Commit 2cff232 suggests "using the most stable binary release to bootstrap" but this comment was later removed and we never updated from Go 1.7 to a more recent released version.

I would recommend bootstrapping Go 1.16 with Go 1.15.8 instead, which should generate a quicker compilation, and going forwards to use the second most recent major release (ie. 1.15.x) to bootstrap the current major release (1.16 or 1.16.x)

If you have read "Reflections on Trusting Trust" you will be aware there's a bootstrapping problem - if you insert a backdoor at any point into the bootstrap mechanism, you can propagate the backdoor forward through all compiled versions. We could get around this by trying to download the C sources, using C to build Go 1.4, and then bootstrap up from there. However as I understand it Go 1.4 does not build on modern Mac versions. At some point I think we need to trust the artifact being downloaded from golang.org, and if we are going to do that we might as well trust Go 1.15.8.

Finally, if this is the wrong place for this discussion, please let me know. The template makes me seem like I am doing it wrong, but the other places all seemed like less-good versions to create this issue.

What happened (include all command output)?

Not relevant for this issue.

What did you expect to happen?

Not relevant for this issue.

Step-by-step reproduction instructions (by running brew commands)

brew install --build-from-source go

@kevinburke1 kevinburke1 added the bug Reproducible Homebrew/homebrew-core bug label Feb 17, 2021
@carlocab
Copy link
Member

Which version of go do the binary distributions released at https://golang.org bootstrap with? If we know, we should use that.

That said, their docs on building from source don't take a position on how new the go compiler you're bootstrapping with is, so I imagine it doesn't matter too much. If it did, I suspect there would at least be some caveats about bootstrapping with go1.4 (and they do say they update it so that it builds on newer systems).

Finally, if this is the wrong place for this discussion, please let me know. The template makes me seem like I am doing it wrong, but the other places all seemed like less-good versions to create this issue.

I agree that this isn't quite the right place for this. This discussion is probably best had at the (surprise) discussions page.

@kevinburke1
Copy link
Author

The description on the "New Issue" page for "Discussions" suggests:

Have a question? Not sure if your issue affects everyone reproducibly? The quickest way to get help is on Homebrew's GitHub Discussions!

The description makes it seem like Discussions is for getting support with different formulas. I don't have a question, and my issue affects everyone reproducibly, so I was not sure that it would be a good fit.

@carlocab
Copy link
Member

We should fix that; this issue tracker is for bugs in homebrew/core formula. Everything else should go to the discussions page.

That said, I think I'm going to disagree with the "reproducibly" claim: there are no problems here that someone else can reproduce. It may well be the case that it's better to bootstrap with a newer go compiler, but there's no concrete evidence of this thus far.

@kevinburke1
Copy link
Author

This failed build seems to indicate they're using Go 1.13.4 to bootstrap the Go 1.15 darwin builder. They haven't set up builders for Go 1.16 yet, I don't think.

https://build.golang.org/log/5e5d23f0d609ad291d7be138ef1ff147a6c613b1

Mirror here in case the log file gets cleaned up. https://gist.github.com/kevinburkemeter/353ba6bcef46e7d1a75a961d8920e01e

@kevinburke1
Copy link
Author

The note in the builders repo suggests that they do this because of Mac signing issues. https://github.com/golang/build/blob/master/env/darwin/macstadium/image-setup-notes.txt#L15

Download Go 1.13.4 or newer tarball via curl (bootstrap version of Go must be signed and notorized in macOS version >= 10.15).

On Linux they're still building using Go 1.4. https://build.golang.org/log/c1a857441c980cec731eb2ebc670dbafc27e3941

@MikeMcQuaid
Copy link
Member

However, there's no reason Go 1.7 must be used, and it's possible that using 1.7 generates inefficient machine code compared with the most recent release.

which should generate a quicker compilation

Can you please verify/benchmark this before proposing we make any changes? Like all performance work: unless you're using a benchmark you're running a real risk of making things worse rather than better.

@Bo98
Copy link
Member

Bo98 commented Feb 18, 2021

We will be using 1.16 stable for ARM builds of Go. I don't see any particular reason why we should not align Intel to be the same.

As for the future, there doesn't seem to be any particular guidance from Go except to use a "recent binary release" to bootstrap.

Other formulae either generally follow a "last stable release" policy (usually based on upstream advice): rust, openjdk, etc.
Or use the same version: clozure-cl, ldc, sometimes ghc, etc.

While I see some of the reasons like:

it's possible that using 1.7 generates inefficient machine code

rather speculative and thus not very strong reasons to move, I equally don't see strong reasons to be sticking on 1.7 when it's the only formula (to my knowledge) to forbid bootstrap updates. There is nothing special about 1.7, and there never has been.

@carlocab
Copy link
Member

There is nothing special about version 1.7 in particular, but I do think the fact that we've used it over many versions of go without any problems is a point in its favour. If it's not broken...

@MikeMcQuaid
Copy link
Member

We will be using 1.16 stable for ARM builds of Go. I don't see any particular reason why we should not align Intel to be the same.

👍🏻

@SMillerDev
Copy link
Member

I agree that we can switch to 1.16 for maintainability reasons. I'm not so convinced by the other claims.

kevinburke1 added a commit to kevinburke1/homebrew-core that referenced this issue Feb 19, 2021
There is nothing special about Go 1.7, and a more recent version of Go
is both supported and should compile more quickly - on my machine this
saves about 5 seconds. See discussion in Homebrew#71370 for more on why make
this change.
@fxcoudert
Copy link
Member

Fix in progress, closing here.

@svengreb
Copy link
Contributor

I guess it's not a coincidence, but Ross Cux published a new proposal to “adopt Go 1.16 as bootstrap toolchain for Go 1.18“. Just wanted to mention this as reference.

@carlocab
Copy link
Member

Thanks for sharing.

One important thing about that post it that it seems to explicitly recommend against using go1.17 or go1.18 to bootstrap, which is what we were planning to do, I think.

@Bo98
Copy link
Member

Bo98 commented Feb 25, 2021

which is what we were planning to do, I think

We didn't really agree on much about the future. Just that we should align Intel and ARM, which we've now done. I didn't touch the comment about not updating, so that remains the policy.

It'll probably be the case that we stick with Go 1.16 until the next big macOS change comes around that nudges us towards updating.

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

Successfully merging a pull request may close this issue.

8 participants