runtime: deprecate func GOROOT

[rittneje]

The runtime.GOROOT() function is currently implemented as follows:

1. If the GOROOT environment variable is set, return its value.
2. Else, return the value that was burned into the binary at compile time.

Unfortunately, this implementation cannot be relied upon to have any semantic meaning in general.

If the intent is to know what GOROOT the binary was compiled with, then consulting the GOROOT environment variable is wrong. Instead, the burned in value should be added to debug.BuildInfo.

If the intent is to know the GOROOT on the current machine, then consulting the burned in value is wrong, as it cannot handle the following (non-exhaustive) scenarios:

1. The binary may have been compiled on another machine, with a totally irrelevant GOROOT.
2. The go installation may have been moved since the binary was compiled.
3. The PATH environment variable may have changed since the binary was compiled.
4. The current machine might not even have a go installation.

Instead, the go env GOROOT command should be used. In particular, this change needs to be made for go/build.Default to function properly in all cases.

[mvdan]

I think this is an interesting proposal, and I agree with the current confusion causing problems at times.

I assume that, if a program wants to try both approaches, they could have code that first tries one approach (like go env GOROOT), and then falls back to the other (like debug.BuildInfo).

[mvdan]

Instead, the go env GOROOT command should be used. In particular, this change needs to be made for go/build.Default to function properly in all cases.

I fully agree with your suggestion to use go env GOROOT, but I'm worried about changing go/build.Default.GOROOT. Note that Default is filled at init time, so it has a noticeable flat cost added to any program that ends up importing the package. Most of that cost currently comes from many calls to os.Getenv; adding an execution of go env would make it significantly slower, making many Go programs take a few milliseconds longer to start.

So when it comes to go/build, I think we should either keep it as-is with a warning in its documentation, or fully deprecate the Context.GOROOT field, instead pointing people to better alternatives like go env GOROOT to just query that one field, or to https://pkg.go.dev/golang.org/x/tools/go/packages for a whole-sale replacement of the Context.Import API.

[rittneje]

I fully agree with your suggestion to use go env GOROOT, but I'm worried about changing go/build.Default.GOROOT. Note that Default is filled at init time, so it has a noticeable flat cost added to any program that ends up importing the package

One possible solution would be to add a private flag to go/build.Context, such as isDefault bool. If true, then it won't set any environment variables when calling go list, since at best this accomplishes nothing, and at worst causes bugs.

go/src/go/build/build.go

Lines 1182 to 1188 in 81767e2

	cmd.Env = append(os.Environ(),
	"GOOS="+ctxt.GOOS,
	"GOARCH="+ctxt.GOARCH,
	"GOROOT="+ctxt.GOROOT,
	"GOPATH="+ctxt.GOPATH,
	"CGO_ENABLED="+cgo,
	)

[mvdan]

Would that help with the added init cost, though, if its init func may still call go list?

[rittneje]

@mvdan I'm not suggesting that the init func for go/build call go list. I'm saying that when go/build calls go list within importGo (which already happens today), it should not attempt to set any of the environment variables in the case of build.Default.

Alternatively, at the top of importGo, it should call go env GOROOT rather than relying on the (possibly broken) value of build.Default.GOROOT.

[mvdan]

Ah, I see. I was thinking mainly of end users directly reading build.Default.GOROOT. Presumably we need to do something about that one way or another.

[bcmills]

The one case I can think of where runtime.GOROOT() may still be helpful is for tests, where it may be easier to invoke runtime.GOROOT() than to shell out to go env GOROOT. The latter may not be accurate anyway if the go in $PATH is not the one used to compile the test. (Consider ~/go-devel/bin/go test ., where ~/go-devel is a GOROOT but is not listed in $PATH.)

At least in principle, it is possible for the go command to ensure that GOROOT is set accurately in the test process's environment when it is run. Then again, if we do that then the test could run os.Getenv("GOROOT") just as easily as runtime.GOROOT(). 🤔

[rittneje]

@bcmills I'm not sure what the use cases are for looking up GOROOT while unit testing, but one thing to keep in mind is that it is possible to compile a test binary with -c and then run it later. Consequently, it would run into the same general issues I mentioned above.

[bcmills]

That's true, but if you compile a test binary with -c and then run it later, you are also responsible for replicating any conditions necessary for the test to succeed (working directory, environment, etc.).

Even so, I suspect that most tests that depend on runtime.GOROOT() only do so via go/build (for example, because they are testing analyzers for Go source code), and fixing #51483 would make those tests more robust either way.

[rittneje]

Gotcha. Actually, as an optimization, since we know that $GOROOT takes precedence if it's set, then go/build can just check the env var first before shelling out to go env GOROOT. Then maybe such test cases can mandate the env var gets set for stabler results (as in the example you mentioned).

I guess another approach would be to have go test manipulate the PATH variable for the sub-process, by prefixing it with $(go env GOROOT)/bin. Not sure if that would be too surprising, but it does have the nice side effect of ensuring that any go commands that get spawned by the test do the more expected thing.

[rsc]

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]

@bradfitz reports that he has uses of runtime.GOROOT in GitHub actions where built binaries want to find the toolchain that built them, so they can invoke it to do other things.

It seems like we probably need to enumerate the uses people have for runtime.GOROOT and what they should do instead. It doesn't help to just say "Deprecated". We have to say what to use instead.

[bradfitz]

For example:

bradfitz@tsdev:~/src/tailscale.com$ git grep runtime.GOROOT
net/tlsdial/tlsdial_test.go:    cmd := exec.Command(filepath.Join(runtime.GOROOT(), "bin", "go"),
tstest/archtest/qemu_test.go:                   cmd := exec.Command(filepath.Join(runtime.GOROOT(), "bin", "go"),
tstest/integration/integration.go:      goBin := filepath.Join(runtime.GOROOT(), "bin", "go"+exe())
version/modinfo_test.go:        goTool := filepath.Join(runtime.GOROOT(), "bin", "go"+exe())

[rittneje]

@bradfitz I'm not sure which repos those are, but this seems to at least partially align with with what @bcmills mentioned above re: tests. In that situation, we can make go test automatically set the GOROOT env var when running the test binary, and then the unit tests just look at that env var. No need for runtime.GOROOT() specifically, although it would have the same effect.

With regards to non-test binaries that intend to find the toolchain that built them, this feels somewhat ill-defined, for the reasons mentioned above, as well as issues with -trimpath. And to consult GOROOT in this case seems strange, because it could point to any arbitrary toolchain installation.

In short:

1. If you need the specific GOROOT with which the binary was compiled, fetch the burned in value (from debug.BuildInfo). But it should be noted that (a) it will not work if compiled with -trimpath, and (b) it might refer to a non-existent folder on the current machine. So if you are trying to use GOROOT to invoke the go toolchain, your binary won't be portable. Users have no control over the GOROOT selection, other than to recompile. Also, it should be noted that even this isn't guaranteed to give you the right toolchain, as the user may have changed the installation at that path after the fact.
2. If you need the currently configured GOROOT, run go env GOROOT. As an optimization, you may check for the GOROOT env var first. The resulting binary will be portable, but the result will have no guaranteed relation to the GOROOT your binary was compiled with. Users should set GOROOT or PATH appropriately to select different GOROOTs.
3. As a "light" version of (2), you may exclusively rely on the GOROOT env var, and fail if it is unset.
4. If you are in complete control of the execution environment (such as with a GitHub action), there is no need for the standard library to participate at all. Either configure your PATH appropriately (so that exec'ing go does the right thing), or set some arbitrary environment variable of your choosing (possibly GOROOT but it doesn't matter) with the path of the desired Go installation and have your application read it.

[bcmills]

@bradfitz, thanks for those examples. I cloned the tailscale repo and ran go test -trimpath ./net/tlsdial ./tstest/... ./version with a go built from head, and those tests already fail in that mode.

(They happen to pass when run with golang.org/dl/go1.18 or golang.org/dl/gotip because the wrappers for those binaries set GOROOT explicitly in the process environment: https://cs.opensource.google/go/dl/+/master:internal/version/version.go;l=67;drc=f798e20c9ec1dfe45a3a432d168172504ecf3b88)

~/src/tailscale.com$ ASSUME_NO_MOVING_GC_UNSAFE_RISK_IT_WITH=$(go version | grep -o 'devel .* +0000') go test -trimpath ./net/tlsdial ./tstest/... ./version -count=1
--- FAIL: TestFallbackRootWorks (0.00s)
    tlsdial_test.go:54: mkcert: fork/exec bin/go: no such file or directory,
FAIL
FAIL    tailscale.com/net/tlsdial       0.027s
ok      tailscale.com/tstest    0.021s
--- FAIL: TestInQemu (0.00s)
    --- FAIL: TestInQemu/386 (0.00s)
        qemu_test.go:69: failed:
        qemu_test.go:73: output didn't contain "I am linux/386":
FAIL
FAIL    tailscale.com/tstest/archtest   0.017s
--- FAIL: TestOneNodeUpNoAuth (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:39475
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestAddPingRequest (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:43717
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestOneNodeUpAuth (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:45951
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestCollectPanic (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:40909
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestTwoNodes (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:36241
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestNoControlConnWhenDown (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:36325
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestControlTimeLogLine (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:37683
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestNodeAddressIPFields (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:41095
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestStateSavedOnStart (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:37661
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestOneNodeExpiredKey (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:40891
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestLogoutRemovesAllPeers (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:45759
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
--- FAIL: TestOneNodeUpWindowsStyle (0.01s)
    integration_test.go:518: DERP httpsrv listener: 127.0.0.1:38921
    integration.go:63: failed to find go at bin/go
    stuntest.go:64: STUN server shutdown
FAIL
FAIL    tailscale.com/tstest/integration        0.076s
?       tailscale.com/tstest/integration/testcontrol    [no test files]
ok      tailscale.com/tstest/integration/vms    0.092s
?       tailscale.com/tstest/integration/vms/gen        [no test files]
ok      tailscale.com/tstest/natlab     0.009s
--- FAIL: TestFindModuleInfo (0.00s)
    modinfo_test.go:22: failed to build tailscaled: fork/exec bin/go: no such file or directory
FAIL
FAIL    tailscale.com/version   0.014s
FAIL

There is also no guarantee in general that even a non-empty runtime.GOROOT() reports the GOROOT with which the test binary was actually built. (For example, the test could be built with go test -c and the GOROOT subsequently rebuilt at an entirely different version.)

That's of course fine for tailscale's own code if you're ok with those failure modes, but may be further evidence that we should discourage that pattern in general. 😅

A more robust approach might be to simply look for go in the usual $PATH and either skip or fail the test if go env GOVERSION does not match runtime.GOVERSION(). (See https://go.dev/play/p/5Vg4frf8pIk for a sketch. Maybe we should publish that as a supported test-helper somewhere?)

[rsc]

Is "finding the go command" the only thing we need GOROOT for? Probably?
We have internal/testenv.GoToolCmd so clearly we need it frequently.
Maybe we can solve that more limited problem?

Possibilities:

1. New API and a new environment variable that 'go test' would start setting when running the test binary.
2. New API using $GOROOT, which 'go test' would start setting when running the test binary.
3. Have 'go test' put the Go bin directory at the front of $PATH before running the test binary. (No new API!)

Number 3 would fit well with non-test code like go/build and go/packages that already assume that looking for "go" in your PATH is the one you want.

Thoughts on finding the go command?

And thoughts on any other needs from GOROOT?

[bcmills]

1. New API and a new environment variable that 'go test' would start setting when running the test binary.

That wouldn't necessarily even require a new environment variable: a flag might suffice. I think the API is the tricky part.

Maybe:

package testing

// GOROOT reports the GOROOT with which the test was built, if known.
// This function may report a non-empty GOROOT even if runtime.GOROOT
// returns the empty string (such as if the test was built with -trimpath).
func GOROOT() (string, bool)

[bcmills]

2. New API using $GOROOT, which 'go test' would start setting when running the test binary.

Having go test set $GOROOT would hide erroneous existing uses of runtime.GOROOT: having the variable set would mean that runtime.GOROOT always reports the test's real GOROOT, which would mask cases where a corresponding production binary would be unable to locate it.

I think this option is not really much better than deciding not to deprecate runtime.GOROOT() at all, which IMO is unfortunate because it interacts so badly with -trimpath.

[bcmills]

3. Have 'go test' put the Go bin directory at the front of $PATH before running the test binary. (No new API!)

I like that approach a lot overall. It looks like GOROOT/bin normally contains only go and gofmt — neither of which should pollute the search namespace very much — and I suspect that a large fraction of tests that run go and/or gofmt already assume that the binary matches the Go version used to build and run the test itself.

That would also make it fairly trivial to identify the GOROOT location in general, since the test binary could just exec go env GOROOT.

[rsc]

It sounds like people like "Have 'go test' put the Go bin directory at the front of $PATH before running the test binary." so let's start with that. @bcmills, want to write a CL for that?

Then tests and things like go/packages that exec "go" will work fine without runtime.GOROOT.

At that point, what is left for use cases for runtime.GOROOT?

[danderson]

@rsc making sure I understand your last response: would go test still obey the GOROOT environment variable when figuring out what Go bin directory to add to $PATH?

[bcmills]

@danderson, the go command and runtime.GOROOT would continue to respond to the GOROOT environment variable as they already do. (We would just discourage the use of runtime.GOROOT specifically, since it is overly sensitive to the presence of that variable.)

[rsc]

Are there any other problems with deprecating runtime.GOROOT, once we've guaranteed that "go in the PATH" is the right go during a test?

[gopherbot]

Change https://go.dev/cl/404134 mentions this issue: cmd/go: place GOROOT/bin at the beginning of PATH in 'go generate' and 'go test'

[gopherbot]

Change https://go.dev/cl/564142 mentions this issue: runtime: deprecate GOROOT

[gopherbot]

Change https://go.dev/cl/564275 mentions this issue: gopls/internal/test/marker: remove runtime.GOROOT use in format.txt case

[dmitshur]

A fix CL is available but hasn't been reviewed before Go 1.23 freeze. Moving to the next milestone.

[mvdan]

https://pkg.go.dev/time#LoadLocation still relies on runtime.GOROOT to read $GOROOT/lib/time/zoneinfo.zip. I reckon this should be redesigned, either to remove that lookup path entirely, or to detect the Go install actually present on the machine via e.g. go env GOROOT. Otherwise, it's far too easy for a developer to see their binary load timezones correctly via GOROOT, but when the binary is run on another machine or deployed, it suddenly stops working.

The cost of go env GOROOT would be noticeably higher, but as long as it's behind a sync.Once, I think it's fine. Loading the zip file is probably a non-trivial amount of work anyway.

[mvdan]

I should also note that time/zoneinfo_read.go is the only Go standard library file, aside from tests and go/build, which currently relies on runtime.GOROOT. So at least this is not a widespread problem.

[ianlancetaylor]

We originally introduced the fallback to $GOROOT/lib/time for Windows and Plan 9 (https://golang.org/cl/5656101, #2964). Since then it looks like maybe iOS needs it also. For iOS in particular I'm not sure that running go env GOROOT is going to be useful.

[rittneje]

To add to that, time.LoadLocation currently only mentions $GOROOT/lib/time/zoneinfo.zip. It doesn't say anything about the burned-in GOROOT that runtime.GOROOT() will fall back to, but I would not be surprised if people are currently relying on that behavior.

As @ianlancetaylor alluded to, the application may be running on a system that doesn't even have a Go installation, so running go env GOROOT is not an option in general. Even if it were, that could still be a breaking change because today it uses the burned-in GOROOT, which may be different than the go that is currently in your PATH.

For now I think time.LoadLocation needs to continue relying on runtime.GOROOT() despite its flaws. Its documentation should be amended to mention this fallback explicitly, and also mention that -trimpath makes it not work. (runtime.GOROOT() should also mention that restriction.)

[willfaught]

Out of curiosity, why does https://pkg.go.dev/time#LoadLocation need to read $GOROOT/lib/time/zoneinfo.zip? What GOOS/GOARCH or mode uses it instead of the OS zoneinfo/tzdata?

[rittneje]

@willfaught See @ianlancetaylor's comment above, and burrowers/garble#878.

[mvdan]

For environments where go env GOROOT will fail because Go is not installed, that's fine - we can skip loading tzdata from GOROOT just like we currently do when the embedded GOROOT path does not actually exist on disk. Why is that a problem? We already have other parts of the standard library which execute go env for similar purposes.

Personally I would prefer if we nudged everyone towards https://pkg.go.dev/time/tzdata in the longer term, but assuming that we want to preserve the current zip loading logic from GOROOT, I think we should switch it to go env GOROOT the sooner the better.

I would not be surprised if people are currently relying on that behavior.

I would also not be surprised by this either, but I think those users would continue to be satisfied by the switch to go env GOROOT.

[rittneje]

@mvdan Today I can compile an application that calls time.LoadLocation for Windows, put it on a Windows machine, set $GOROOT, and arrange for $GOROOT/lib/time/zoneinfo.zip to exist, without any need to install a Go toolchain on that Windows machine. Therefore changing time.LoadLocation to invoke go env GOROOT would be a breaking change.

[DeedleFake]

Proposal for a proposal: Add runtime/env to expose go env's lookup functionality programmatically, updating go env to use the new package itself for consistency. Then just use that to find the equivalent $(go env GOROOT) in time.LoadLocation().

[mvdan]

@rittneje fair enough, but that has nothing to do with the embedded GOROOT value. The logic could be to query the GOROOT env var first, and if empty, fall back to go env GOROOT.

@DeedleFake that is a separate proposal so it's best to not mix the two.

[rittneje]

@mvdan Let's say I have Windows machine with Go installed at C:\Go. I then use that to compile my application (without -trimpath), copy it to a second Windows machine, and arrange for C:\Go\lib\time\zoneinfo.zip to exist on the second machine. Today that will work. But using os.Getenv("GOROOT") won't work (because the second machine never set that env var), nor will running go env GOROOT (because the second machine does not have a Go installation).

@DeedleFake This can already be achieved in practice via os.Getenv("GOROOT") followed by exec.LookPath("go"). Unfortunately, it does not address this backwards compatibility issue, because in the absence of the GOROOT env var, runtime.GOROOT() returns the burned in GOROOT, not the Go installation.

[mvdan]

@rittneje that seems to me like a rare use case which relies on the embedded path, which is now being discouraged via this deprecation. I'm not arguing we need to break it right away, and it could be a phased approach via a GODEBUG, but I definitely think we need to stop supporting such a use case.

[ianlancetaylor]

If we are going to keep checking runtime.GOROOT, then are there really that many cases where runtime.GOROOT fails but go env GOROOT succeeds?

[rittneje]

@ianlancetaylor I'm not sure what you mean by "keep checking runtime.GOROOT". When $GOROOT is not set, runtime.GOROOT() and go env GOROOT answer completely different questions:

• runtime.GOROOT() gives the Go installation that compiled the application, possibly on another machine. And if you do -trimpath, then it yields the empty string.
• go env GOROOT gives the current Go installation on this machine, assuming it has one at all.

In my view, as the documentation for time.LoadLocation only mentions $GOROOT and not anything about it trying to find "the current Go installation", running go env GOROOT as a fallback is not necessary. The only question is whether we need to account for the possibility that people have been relying on it falling back to the burned in GOROOT when $GOROOT is not set, even though the documentation does not mention this.

In short, there are really three choices:

1. Leave it the way that it is.
2. Change time.LoadLocation to use os.Getenv("GOROOT") instead of runtime.GOROOT().
3. Add some godebug setting to opt into or out of (2).

[aarzilli]

Using go env GOROOT instead of runtime.GOROOT has potentially surprising implications now that ~everyone has multiple toolchains installed.

[ianlancetaylor]

@rittneje For your point 2, the runtime.GOROOT function will already return the environment if it is set. If the environment variable is not set, it will return the value from the tools used to build the program. If we're OK with checking the environment variable, then I think the only question is whether it is harmful to also check the value from the tools.

[rittneje]

@ianlancetaylor I do not personally believe it is actively harmful for time.LoadLocation to fall back to the burned in GOROOT. I do however thing it is surprising, given that it is not documented.

Meanwhile, if someone really wants to augment time.LoadLocation to try to find the Go installation instead of using the burned in GOROOT, that ought to be its own proposal.