Open rittneje opened 2 years ago
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
).
Instead, the
go env GOROOT
command should be used. In particular, this change needs to be made forgo/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.
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.
https://github.com/golang/go/blob/81767e23c2f0e3edf0a329d9f00f5683c9851692/src/go/build/build.go#L1182-L1188
Would that help with the added init cost, though, if its init func may still call go list
?
@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
.
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.
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()
. 🤔
@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.
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.
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.
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
@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.
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())
@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:
-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.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.GOROOT
env var, and fail if it is unset.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.@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)
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?)
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:
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?
- 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)
- 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
.
- 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
.
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?
@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?
@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.)
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?
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'
https://go.dev/cl/404134 contains the suggested change to go test
. For similar reasons I also applied it to go generate
.
It sounds like CL 404134 will be in Go 1.19. Let's see how that goes and circle back to this. Holding issue for Go 1.19.
Placed on hold. — rsc for the proposal review group
Change https://go.dev/cl/409176 mentions this issue: doc/go1.19: add a release note for CL 404134
I think this is also affecting go run
?
I have something that executes a different version of go
With
gotip version
go version devel go1.19-8a56c7742d Wed Jun 1 02:37:46 2022 +0000 linux/amd64
/usr/bin/go version
go version go1.18.2 linux/amd64
exec.Command("/usr/bin/go", "install", "golang.org/dl/go1.18.2")
gotip run .
gets me
# internal/goarch
compile: version "devel go1.19-8a56c7742d Wed Jun 1 02:37:46 2022 +0000" does not match go tool version "go1.18.2"
# internal/goos
compile: version "devel go1.19-8a56c7742d Wed Jun 1 02:37:46 2022 +0000" does not match go tool version "go1.18.2"
...
While gotip build -o out . && ./out
will execute successfully
Changing the PATH env for the exec.Cmd
doesn't seem to help.
@seankhliao, I think it's more intuitive for go run
to just resolve whatever is already in $PATH
.
go run
is analogous to running O=$(mktemp); go build -o $O && $O; unset O
, which would itself just resolve whatever is already in $PATH
. (Once the binary is built, it's out of cmd/go
's hands.)
(That said, gotip run
is already a bit funky because it sets GOROOT
explicitly no matter which subcommand it runs; see https://cs.opensource.google/go/dl/+/1107b86e:internal/version/version.go.)
Change https://go.dev/cl/450714 mentions this issue: cmd/fix: disallow cgo errors in tests
@bcmills your change for Go 1.19 is great. I was able to delete some code that tried to figure out the right go
to call.
Could we document it as part of go test
and go generate
, though? I don't love depending on undocumented behavior of these tools.
@cespare, good point. I've filed #57050 for that documentation.
@rsc It's been a few releases. Can this proposal be moved out of "on hold"?
Reviving. The proposal is to deprecate GOROOT in favor of running "go" from PATH when you want to run the go command, and using 'go env GOROOT' when you need to know the GOROOT.
// GOROOT returns the root of the Go tree. It uses the
// GOROOT environment variable, if set at process start,
// or else the root used during the Go build.
//
// Deprecated: The root used during the Go build will not be
// meaningful if the binary is copied to another machine.
// Use the system path to locate the “go” binary, and use
// “go env GOROOT” to find its GOROOT.
func GOROOT() string
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 It might also be good to explicitly mention what runtime.GOROOT()
returns if $GOROOT
is not set and the binary was compiled with -trimpath
since that is another sharp edge.
@rittneje, note that that was settled in #51461 and it now returns the empty string in that case. I think that's the appropriate behavior going forward.
@bcmills Yeah I think that behavior is fine, it's just not currently mentioned in the documentation.
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group
Mark runtime.GOROOT deprecated:
// GOROOT returns the root of the Go tree. It uses the
// GOROOT environment variable, if set at process start,
// or else the root used during the Go build.
//
// Deprecated: The root used during the Go build will not be
// meaningful if the binary is copied to another machine.
// Use the system path to locate the “go” binary, and use
// “go env GOROOT” to find its GOROOT.
func GOROOT() string
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
Mark runtime.GOROOT deprecated:
// GOROOT returns the root of the Go tree. It uses the
// GOROOT environment variable, if set at process start,
// or else the root used during the Go build.
//
// Deprecated: The root used during the Go build will not be
// meaningful if the binary is copied to another machine.
// Use the system path to locate the “go” binary, and use
// “go env GOROOT” to find its GOROOT.
func GOROOT() string
Change https://go.dev/cl/564142 mentions this issue: runtime: deprecate GOROOT
Change https://go.dev/cl/564275 mentions this issue: gopls/internal/test/marker: remove runtime.GOROOT use in format.txt case
A fix CL is available but hasn't been reviewed before Go 1.23 freeze. Moving to the next milestone.
The
runtime.GOROOT()
function is currently implemented as follows:GOROOT
environment variable is set, return its value.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 todebug.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:
GOROOT
.PATH
environment variable may have changed since the binary was compiled.Instead, the
go env GOROOT
command should be used. In particular, this change needs to be made forgo/build.Default
to function properly in all cases.