Break reliance on GOROOT_FINAL

[JacobOaks]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Due to golang/go#62047, Go 1.23 won't support GOROOT_FINAL. This means that #971 will no longer fix #969,
meaning linked binaries will contain full GOROOT paths, making them non-reproducible.

Instead of using GOROOT_FINAL, this change directly sets GOROOT when invoking the linker, which will cause the linker to continue to write GOROOT into the binary, keeping builds consistent.

An existing test that asserts on paths containing GOROOT passes with this PR. This passes on Go 1.23rc1 as well (it fails without this PR).

I'm not a rules_go expert and I'm not married to this particular solution. If there's a clever way we can set -trimpath when invoking the compiler, that might be better - but we already use that flag to trim off the bazel sandbox I believe.

Which issues(s) does this PR fix?

Fixes #3983

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[sluongng]

I suspect this might require a minimum Go SDK version bump to 1.21, in rules_go setup as well as in the test.

Is it possible to make the behavior conditional based on the current SDK version?

[JacobOaks]

Hey @sluongng - can you elaborate on why you suspect that? As far as I can tell, rules_go seems to be set up for 1.21 already.

[sluongng]

It's from the issue you linked.

As of Go 1.21, in order to provide reproducible builds of the Go toolchain (golang/go#57120), cmd/dist started building releases with the -trimpath flag set, which incidentally caused GOROOT_FINAL to have no effect (golang/go#61921). (Development versions of toolchains are still built without -trimpath by default, so GOROOT_FINAL may affect those builds somewhat.)

So we could safely ignore this variable starting from 1.21.

However, some folks could be using rules_go with older Go SDK versions who wish to get the latest rules_go features still. It's best if we could avoid forcing folks to upgrade their Go SDK when upgrading rules_go by making rules_go code work with older versions as well as newer ones.

Effectively, I think you could undo your change in go/private/context.bzl and leave the variable as is.

In link.go, you could call a func manageGoRoot() (func()) func (return is the defer func) with 2 different implementations: one using

//go:build go1.21
// +build go1.21

and one using

//go:build !go1.21
// +build !go1.21

[sluongng]

On second thought, if this is only a breaking behavior in 1.23, I think we should make it a 1.23 forward feature instead of 1.21. 🤔

[JacobOaks]

Hey @sluongng - so I did try this solution on a couple pre-Go1.23 versions, and it worked on the most recent older versions too - but I agree it's probably a good idea to hide this behind 1.23+ just to be safe.

I tried to add build constraints, but the problem I ran into is that go_tool_binary targets (which the builder binary happens to be) do not support build constraints due to the way they invoke go build. So it seems like we have a couple options:

• Change go_tool_binary to materialize a temporary go module
• Switch off of something like runtime.Version(), which is probably quite brittle and undesirable, but seems to technically work?
• Verify that this change works on all Go versions supported by rules_go and drop the version-switching idea

Let me know if you have thoughts/other ideas.

[sluongng]

Hmm but we do have this though 🤔
https://github.com/bazelbuild/rules_go/blob/master/go/tools/builders/nogo_typeparams_go118.go

[sluongng]

ah it's part of nogo which is built after a transition? 🤔

Hmm...

[JacobOaks]

Yeah, I think nogo is a separate type of rule that does support build constraints.

I updated the PR to switch based off runtime.Version for now, which like I mentioned isn't quite as satisfying as build constraints would be, but does seem to technically work. Though I'm not familiar with rules_go enough to know if there are situations where the builder would have been built with a different version than is being used when link.go is invoking the linker.

[fmeum]

@tyler-french Could you test this?

[tyler-french]

We've tested this at Uber