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

cmd: drop support for GOROOT_FINAL #62047

Closed
bcmills opened this issue Aug 15, 2023 · 7 comments
Closed

cmd: drop support for GOROOT_FINAL #62047

bcmills opened this issue Aug 15, 2023 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go Proposal Proposal-Accepted
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Aug 15, 2023

Prior to Go 1.21, setting the GOROOT_FINAL variable during the make scripts changed the GOROOT value embedded in the tools installed in $GOROOT/bin and $GOROOT/pkg/tool. That allowed those tools (primarily cmd/go) to locate GOROOT even if it (or just its binaries) was symlinked or copied to a different install location.

In Go 1.9 (#18678), to reduce confusion and boilerplate from explicit GOROOT settings, cmd/go was changed to use os.Executable to locate its own binary and then derive GOROOT from the fact that cmd/go is normally installed in $GOROOT/bin. cmd/go then passes that path to other tools explicitly as needed. As long as the go command invoked by the user is actually resolved in $GOROOT/bin (possibly via a symlink), it will infer the correct GOROOT regardless of what value may happen to be baked in.

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

So GOROOT_FINAL is only really helpful if all of the following occur:

  • The toolchain is being built only for a non-release version,
  • GOROOT is moved after the make script is run,
  • the cmd/go binary is copied or hard-linked (not symlinked!) to some location outside of GOROOT/bin, and
  • the copied cmd/go binary is invoked without setting GOROOT explicitly.

I believe that the intersection of those conditions is “basically never”:


On the other hand, GOROOT_FINAL has long been a source of bugs and complexity in the Go project (see https://github.com/golang/go/issues?q=is%3Aissue+GOROOT_FINAL+in%3Atitle). In particular, tests of the toolchain itself may need to take GOROOT_FINAL into account when they inspect or run artifacts built during the test.

I propose that we:

  • Drop support for custom GOROOT_FINAL settings in cmd/dist, cmd/go, and the make scripts.
  • Either replace the use of GOROOT_FINAL in cmd/link with an explict flag (--trimgoroot?), or change cmd/go to always set GOROOT_FINAL to be equal to either GOROOT (if -trimpath is off) or the literal string "$GOROOT" (if -trimpath is on).
  • Remove public-facing documentation that refers to GOROOT_FINAL.
    • (Note that the documentation for cmd/link already does not mention its use of GOROOT_FINAL.)
@bcmills bcmills added Proposal GoCommand cmd/go compiler/runtime Issues related to the Go compiler and/or runtime. labels Aug 15, 2023
@bcmills bcmills added this to the Proposal milestone Aug 15, 2023
@bcmills bcmills moved this to Incoming in Proposals Aug 15, 2023
@rittneje
Copy link

We use GOROOT_FINAL to prevent our build server's path from being written into the resulting toolchain. It sounds like now make.bash will end up building the toolchain with -trimpath set (by default?), which achieves the same goal. Is that right?

@bcmills
Copy link
Contributor Author

bcmills commented Aug 16, 2023

@rittneje, correct.

The recommended way to prevent build server paths from being written is to use -trimpath, which is already what cmd/dist (and therefore make.bash) does by default for release versions as of Go 1.21.

@rsc
Copy link
Contributor

rsc commented Oct 3, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Oct 3, 2023
@rsc
Copy link
Contributor

rsc commented Oct 11, 2023

Have all remaining concerns about this proposal been addressed?
We haven't seen or heard any objections from packagers, at least that I'm aware of.

@rsc rsc moved this from Active to Likely Accept in Proposals Oct 26, 2023
@rsc
Copy link
Contributor

rsc commented Oct 26, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Remove all use of GOROOT_FINAL in the tree, since it stopped working in Go 1.21 and no one has complained. Specifically:

  1. Drop support for custom GOROOT_FINAL settings in cmd/dist, cmd/go, and the make scripts.
  2. In cmd/link, use GOROOT as GOROOT_FINAL; if -trimpath is set, keep doing what we already do, namely use the package paths as in math/abs.go or cmd/compile/main.go).
  3. Remove public-facing documentation that refers to GOROOT_FINAL.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Remove all use of GOROOT_FINAL in the tree, since it stopped working in Go 1.21 and no one has complained. Specifically:

  1. Drop support for custom GOROOT_FINAL settings in cmd/dist, cmd/go, and the make scripts.
  2. In cmd/link, use GOROOT as GOROOT_FINAL; if -trimpath is set, keep doing what we already do, namely use the package paths as in math/abs.go or cmd/compile/main.go).
  3. Remove public-facing documentation that refers to GOROOT_FINAL.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Nov 2, 2023
@rsc rsc changed the title proposal: cmd: drop support for GOROOT_FINAL cmd: drop support for GOROOT_FINAL Nov 2, 2023
@rsc rsc modified the milestones: Proposal, Backlog Nov 2, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539975 mentions this issue: cmd: remove support for GOROOT_FINAL

@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Feb 1, 2024
@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Feb 1, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Feb 21, 2024
JacobOaks added a commit to JacobOaks/rules_go that referenced this issue Jul 8, 2024
Due to golang/go#62047, Go 1.23 won't support `GOROOT_FINAL`.
This means that bazel-contrib#971 will no longer fix bazel-contrib#969,
meaning linked binaries will contain full GOROOT paths, making them non-reproducible.

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

This works on Go 1.23rc1 as well.

I'm not a rules_go expert and I'm not married to this particular solution,
I just wanted to bring attention to the issue.
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.
fmeum pushed a commit to bazel-contrib/rules_go that referenced this issue Jul 16, 2024
* Clear GOROOT when linking

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 sets `GOROOT` when invoking the linker,
which will cause the linker to continue to write `GOROOT` into the binary,
keeping builds consistent.

This works on Go 1.23rc1 as well.

I'm not a rules_go expert and I'm not married to this particular solution,
I just wanted to bring attention to the issue.
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.

* switch based on runtime.Version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go Proposal Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests

6 participants