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: GOTOOLCHAIN=version+auto doesn't use the named version in go install package@version #69051

Open
cespare opened this issue Aug 24, 2024 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cespare
Copy link
Contributor

cespare commented Aug 24, 2024

Go version

go1.21.13 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN='/home/caleb/bin'
GOCACHE='/home/caleb/.cache/go-build'
GOENV='/home/caleb/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/caleb/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/caleb/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/caleb/apps/oldgo/go1.21.13'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/caleb/apps/oldgo/go1.21.13/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.13'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2590561090=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I believe this reproduces with more recent Go versions as well, but since the bug is about upgrading Go toolchain versions it's easier to demonstrate with an older version.

Here's what happens when I use the go command bundled with 1.21.13:

$ GOTOOLCHAIN=local go version
go version go1.21.13 linux/amd64
$ mkdir -p /tmp/x && GOBIN=/tmp/x GOTOOLCHAIN=go1.23.0+auto go install honnef.co/go/tools/cmd/[email protected] && go version /tmp/x/staticcheck
go: honnef.co/go/[email protected] requires go >= 1.22.1; switching to go1.22.6
/tmp/x/staticcheck: go1.22.6

Here's what happens when I use the go command bundled with 1.23.0:

$ GOTOOLCHAIN=local go version
go version go1.23.0 linux/amd64
$ mkdir -p /tmp/x && GOBIN=/tmp/x GOTOOLCHAIN=go1.23.0+auto go install honnef.co/go/tools/cmd/[email protected] && go version /tmp/x/staticcheck
/tmp/x/staticcheck: go1.23.0

What did you see happen?

With GOTOOLCHAIN=go1.23.0+auto, Go 1.21.13 built the binary with Go 1.22.6.

What did you expect to see?

I would expect both invocations to build staticcheck using Go 1.23.0.

honnef.co/go/tools/cmd/[email protected] has go 1.22.1 in its go.mod. So it makes sense that I've I'm using Go 1.21.13, if I ran GOTOOLCHAIN=auto go install honnef.co/go/tools/cmd/[email protected], it would use Go 1.22.6 since that's the latest patch release of the previous Go version and that's the minimum version of the three options documented at https://go.dev/doc/toolchain#switch.

However, as documented at https://go.dev/doc/toolchain#intro, the <name>+auto form of the GOTOOLCHAIN var essentially tells the Go tool to use the maximum of <name> and whatever Go version auto would use:

The alternate form +auto sets the default toolchain to use before deciding whether to switch further. For example GOTOOLCHAIN=go1.21.3+auto directs the go command to begin its decision with a default of using Go 1.21.3 but still use a newer toolchain if directed by go and toolchain lines.

As far as I can tell, go install is entirely ignoring the <name> part of <name>+auto and just behaving the same as if I'd used GOTOOLCHAIN=auto.

@seankhliao seankhliao changed the title cmd/go: GOTOOLCHAIN=<name>+auto doesn't seem to work with go install cmd/go: GOTOOLCHAIN=version+auto doesn't use the named version in go install Aug 24, 2024
@prattmic
Copy link
Member

cc @matloob @samthanawalla

@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 27, 2024
@prattmic prattmic added this to the Backlog milestone Aug 27, 2024
@matloob
Copy link
Contributor

matloob commented Sep 27, 2024

cc @rsc

So it looks like what's happening here is that we start toolchain selection, determine the minVersion of go1.23.0(https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/toolchain/select.go;l=209;drc=c71b5ff76a1b1d79b53e268b83fb721e0af4614b) then when we call goInstallVersion (https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/toolchain/select.go;l=172;drc=c71b5ff76a1b1d79b53e268b83fb721e0af4614b) to determine if we're in the process of running a go install package@version, we determine that we are.

When that happens we try to do the required switch that's determined (https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/toolchain/select.go;l=682;drc=c71b5ff76a1b1d79b53e268b83fb721e0af4614b) and to do that we load the do a query for the requested package@version and try to do a switch if we get a TooNewError. But the TooNewError is based on the requested module's version being newer than the local toolchain version-- it doesn't take into account the minVersion we had determined earlier. So we end up switching to the version required by the module being installed (1.22.6).

That by itself would be okay, because we run the toolchain switching logic again in the child and determine a toolchain version of 1.23, but then we see that we were invoked as a target toolchain, and that the version we switched to is the local version of the binary (https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/toolchain/select.go;l=235;drc=c71b5ff76a1b1d79b53e268b83fb721e0af4614b) so we throw away the 1.23 version we determined in the second round of toolchain selection and continue execution of the go command at the local 1.22.6 version.

I would need to consult with other team members on this but it sounds to me like we should either change the logic in goInstallVersion to take the minVersion into account, or to change the logic that verifies that if we were invoked as the target we satisfy the required version to not continue with toolchain switching if the previous target doesn't match the new determined toolchain version we'd want to switch to. The latter sounds simpler to me, but I need to check if there's a good reason we're doing what we're doing.

@matloob matloob changed the title cmd/go: GOTOOLCHAIN=version+auto doesn't use the named version in go install cmd/go: GOTOOLCHAIN=version+auto doesn't use the named version in go install package@version Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants
@cespare @prattmic @matloob @gabyhelp and others