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/go,runtime: runtime.GOROOT returns an invalid, non-empty string when built with -trimpath #51461

Closed
rittneje opened this issue Mar 3, 2022 · 24 comments
Labels
NeedsInvestigation release-blocker
Milestone

Comments

@rittneje
Copy link

@rittneje rittneje commented Mar 3, 2022

Reopening #51409 because I don't think "it's a counterfeiter bug" is a sufficient explanation. How can counterfeiter (or any application) cause the go tool to lose track of the implied GOROOT?

What version of Go are you using (go version)?

$ go version
go version go1.17.7 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/tmp/.gocache"
GOENV="/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/Users/rittneje/test/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/Users/rittneje/test"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.7"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/Users/rittneje/test/src/gorootbug/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1092411943=/tmp/go-build -gno-record-gcc-switches"

Note that the GOROOT environment variable is not set. (It is being calculated automatically from the go command's location.)

What did you do?

package gorootbug

//go:generate /usr/local/gotools/bin/counterfeiter io.Reader

Then ran go generate.

What did you expect to see?

It should work.

What did you see instead?

$ go generate -x
/usr/local/gotools/bin/counterfeiter io.Reader
Writing `FakeReader` to `gorootbugfakes/fake_reader.go`... 
go/build: go list io: exit status 2
go: cannot find GOROOT directory: go


bug.go:3: running "/usr/local/gotools/bin/counterfeiter": exit status 1

If I explicitly set GOROOT to itself, then it works.

$ GOROOT=$(go env GOROOT) go generate -x
/usr/local/gotools/bin/counterfeiter io.Reader
Writing `FakeReader` to `gorootbugfakes/fake_reader.go`... Done
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

How can counterfeiter (or any application) cause the go tool to lose track of the implied GOROOT?

Most likely? If it explicitly sets a non-empty GOROOT variable in the process environment to a non-directory or a nonexistent directory, then cmd/go won't override that explicit value with the implicit one.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

Sorry, but this really doesn't look like a cmd/go bug from the information we have so far. If you can get counterfeiter to dump the subprocess's environment, maybe we can investigate from there.

@bcmills bcmills added the WaitingForInfo label Mar 3, 2022
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Mar 3, 2022

where did you obtain the go binary from?

@rittneje
Copy link
Author

@rittneje rittneje commented Mar 3, 2022

@seankhliao @bcmills I have tracked this issue down to a bug in Go. When you pass -trimpath to go build (or similar), it corrupts runtime.GOROOT() in the resulting binary.

package main

import (
	"fmt"
	"runtime"
)

func main() {
	fmt.Println(runtime.GOROOT())
}

With go run, it prints /usr/local/go as expected. With go run -trimpath, it incorrectly prints go.

@ianlancetaylor ianlancetaylor changed the title cmd/go: cannot find GOROOT directory cmd/go: -trimpath changes value returned by runtime.GOROOT() Mar 4, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 4, 2022

It has always worked that way, but it does seem problematic.

@ianlancetaylor ianlancetaylor added NeedsInvestigation and removed WaitingForInfo labels Mar 4, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Mar 4, 2022
@rittneje
Copy link
Author

@rittneje rittneje commented Mar 4, 2022

@ianlancetaylor One thing I do want to point out is that for this specific use case, the issue actually manifests itself within the go/build package, specifically here:

"GOROOT="+ctxt.GOROOT,

In the case of build.Default, it would make more sense not to bother setting GOROOT at all, rather than trying to override it. Then when it invokes the go list command, it'll just figure out GOROOT as usual, which will work regardless of whether -trimpath was applied to the binary that is using build.Default, or even if the binary was compiled on another machine with a totally different GOROOT. In fact, it seems like runtime.GOROOT() is not useful in general, and robust applications ought to use go env GOROOT instead.

I created a separate issue (#51473) to discuss deprecating runtime.GOROOT().

@bcmills bcmills changed the title cmd/go: -trimpath changes value returned by runtime.GOROOT() cmd/go,runtime: runtime.GOROOT returns an invalid, non-empty string when built with -trimpath Mar 4, 2022
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 4, 2022

At the very least, runtime.GOROOT() should probably return the empty string instead of an invalid path when the binary is built with -trimpath — that would make it much more obvious that the value isn't actually “the root used during the Go build” as documented.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 4, 2022

for this specific use case, the issue actually manifests itself within the go/build package

Thanks, that's helpful and straightforward to reproduce. Filed separately as #51483.

@bcmills bcmills self-assigned this Mar 4, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 4, 2022

Change https://go.dev/cl/390024 mentions this issue: cmd/link: avoid stamping runtime.defaultGOROOT when paths are being trimmed

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 4, 2022

Change https://go.dev/cl/390023 mentions this issue: cmd/compile/internal/syntax: don't try to parse files in GOROOT/.git

gopherbot pushed a commit that referenced this issue Mar 8, 2022
This test was failing locally in my clone of the go repo due to a Git
branch ending in ".go", which the test found and was attempting to
parse as a file. It's fragile to try to parse .go files in
GOROOT/.git, and wasteful to scan GOROOT/pkg and other non-source
directories; instead, let's only parse the directories we actually
expect to contain source files.

(I was running the test for #51461.)

Change-Id: I5d4e31ec2bcd9b4b6840ec32ad9b12bf44f349a5
Reviewed-on: https://go-review.googlesource.com/c/go/+/390023
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391806 mentions this issue: internal/testenv: add GOROOT and use it to fix tests broken with -trimpath

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391810 mentions this issue: cmd/go: include the "-trimpath" flag in the stamped build settings

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391808 mentions this issue: cmd/go/internal/test: ensure that build.ToolDir is accurate in tests

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391807 mentions this issue: go/internal/srcimporter: use the 'go' command from the Importer's GOROOT

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391812 mentions this issue: runtime/{debug,pprof}: do not require a GOROOT/src prefix in tests

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391799 mentions this issue: runtime: add logging in TestCtrlHandler and skip it

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2022

Change https://go.dev/cl/391815 mentions this issue: runtime/debug: do not require a GOROOT/src prefix in TestStack

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 15, 2022

Change https://go.dev/cl/393155 mentions this issue: internal/buildcfg: initialize GOROOT to runtime.GOROOT

gopherbot pushed a commit that referenced this issue Mar 16, 2022
When paths are trimmed, the reported file locations begin with the
package import path (not GOROOT/src).

Updates #51461

Change-Id: Idbd408a02e8d03329d10e30b0b08263e69e66285
Reviewed-on: https://go-review.googlesource.com/c/go/+/391812
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 16, 2022
We have no guarantee in general that there is any 'go' command in
$PATH at all, let alone the correct one. However, we can expect that
if a 'go' command is not in scope, the Importer should have a correct
GOROOT setting: otherwise, it would not be able to import anything
from 'std' at all.

Given that information, when we run `go tool cgo` we should use
GOROOT/bin/go specifically, not whatever 'go' we find in $PATH.

This fixes a failure in go/types.TestStdlib that manifests as a
timeout in when the 'go' command is not present in $PATH, due to
repeated retries for every package that transitively depends on
runtime/cgo.

For #51461

Change-Id: I30cc4613f6f02a04e83c8d55657ef01888c7770f
Reviewed-on: https://go-review.googlesource.com/c/go/+/391807
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 17, 2022

Change https://go.dev/cl/393574 mentions this issue: internal/buildcfg: extract logic specific to cmd/go

gopherbot pushed a commit that referenced this issue Mar 17, 2022
When paths are trimmed, the reported file locations begin with the
package import path (not GOROOT/src).

Updates #51461.

Change-Id: Ia6814f970aee11f3d933e75c75136d679d19e220
Reviewed-on: https://go-review.googlesource.com/c/go/+/391815
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
cmd/go/internal/cfg duplicates many of the fields of
internal/buildcfg, but initializes them from a Go environment file in
addition to the usual process environment.

internal/buildcfg doesn't (and shouldn't) know or care about that
environment file, but prior to this CL it exposed hooks for
cmd/go/internal/cfg to write data back to internal/buildcfg to
incorporate information from the file. It also produced quirky
GOEXPERIMENT strings when a non-trivial default was overridden,
seemingly so that 'go env' would produce those same quirky strings in
edge-cases where they are needed.

This change reverses that information flow: internal/buildcfg now
exports a structured type with methods — instead of top-level
functions communicating through global state — so that cmd/go can
utilize its marshaling and unmarshaling functionality without also
needing to write results back into buildcfg package state.

The quirks specific to 'go env' have been eliminated by distinguishing
between the raw GOEXPERIMENT value set by the user (which is what we
should report from 'go env') and the cleaned, canonical equivalent
(which is what we should use in the build cache key).

For #51461.

Change-Id: I4ef5b7c58b1fb3468497649a6d2fb6c19aa06c70
Reviewed-on: https://go-review.googlesource.com/c/go/+/393574
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
In the beginning the Go compiler was in C, and C had a function
'getgoroot' that returned GOROOT from either the environment or a
generated constant. 'getgoroot' was mechanically converted to Go
(as obj.Getgoroot) in CL 3046.

obj.Getgoroot begat obj.GOROOT. obj.GOROOT begat objabi.GOROOT,
which begat buildcfg.GOROOT.

As far as I can tell, today's buildcfg.GOROOT is functionally
identical to runtime.GOROOT(). Let's reduce some complexity by
defining it in those terms.

While we're thinking about buildcfg.GOROOT, also check whether it is
non-empty: if the toolchain is built with -trimpath, the value of
GOROOT might not be valid or meaningful if the user invokes
cmd/compile or cmd/link directly, or via a build tool other than
cmd/go that doesn't care as much about GOROOT. (As of CL 390024,
runtime.GOROOT will return the empty string instead of a bogus one
when built with -trimpath.)

For #51461.

Change-Id: I9fec020d5fa65d4aff0dd39b805f5ca93f86c36e
Reviewed-on: https://go-review.googlesource.com/c/go/+/393155
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
…mpath

This fixes many (but not all) of the tests that currently fail
(due to a bogus path reported by runtime.GOROOT) when run with
'go test -trimpath std cmd'.

Updates #51461

Change-Id: Ia2cc05705529c4859e7928f32eeceed647f2e986
Reviewed-on: https://go-review.googlesource.com/c/go/+/391806
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
This fixes a build failure due to inability to locate the "vet" tool
when the test binary is built with -trimpath.

Updates #51461

Change-Id: I81838cc8842e4ff7900cab81af60501ebba86ff1
Reviewed-on: https://go-review.googlesource.com/c/go/+/391808
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
The -trimpath flag has a strong effect on the resulting binary:
in particular, it determines whether runtime.GOROOT can report
a meaningful path in the absence of an explicit GOROOT environment variable.

For #51461

Change-Id: Id0d55572c0a0a4e2e4724363ed80dfa05b202186
Reviewed-on: https://go-review.googlesource.com/c/go/+/391810
Trust: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2022

Change https://go.dev/cl/399157 mentions this issue: cmd/go: set GOROOT explicitly for 'go generate' subprocesses

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2022

Change https://go.dev/cl/399156 mentions this issue: cmd/go: fix TestScript/build_trimpath_goroot when built with a mismatched GOROOT_FINAL

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2022

Change https://go.dev/cl/399214 mentions this issue: doc/go1.19: document cmd/go changes involving -trimpath

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 12, 2022

Reopening for release notes.

@bcmills bcmills reopened this Apr 12, 2022
gopherbot pushed a commit that referenced this issue Apr 12, 2022
…ched GOROOT_FINAL

Fixes #52236.
Updates #51461.

Change-Id: Ie91e0256afd45e9bbd60fd8cdc696363027ab696
Reviewed-on: https://go-review.googlesource.com/c/go/+/399156
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 12, 2022
Code generators may reasonably expect to find the GOROOT for which the
code is being generated.

If the generator invokes 'go run' (which ought to be reasonable to do)
and the user has set 'GOFLAGS=trimpath' (which also ought to be
reasonable), then either 'go generate' or 'go run' needs to set GOROOT
explicitly.

I would argue that it is more appropriate for 'go generate' to set
GOROOT than for 'go run' to do so, since a user may reasonably invoke
'go run' to reproduce a user-reported bug in a standalone Go program,
but should not invoke 'go generate' except to regenerate code for a Go
package.

Updates #51461.

Change-Id: Iceba233b4eebd57c40cf5dcd4af9031d210dc9d8
Reviewed-on: https://go-review.googlesource.com/c/go/+/399157
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 12, 2022
Updates #51461.

Change-Id: Ie878a9f630062d62027de895750a070b50428a9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/399214
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 18, 2022

Change https://go.dev/cl/400814 mentions this issue: doc/go1.19: move the description of the runtime.GOROOT change from 'cmd/go' to 'runtime'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants