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.

Sign up for GitHub

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

Jump to bottom

cmd/go: GOTOOLCHAIN=<name>+[auto|path] is incorrectly rejected as "invalid minimum toolchain" #61068

Closed
dmitshur opened this issue Jun 29, 2023 · 4 comments
Closed

cmd/go: GOTOOLCHAIN=<name>+[auto|path] is incorrectly rejected as "invalid minimum toolchain" #61068

dmitshur opened this issue Jun 29, 2023 · 4 comments
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.21

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jun 29, 2023 •
edited

The Go toolchain selection section in https://go.dev/doc/toolchain documents that GOTOOLCHAIN may have "<name>+auto" or "<name>+path" form. These are currently rejected in go1.21rc2 and at tip:

$ go version
go version devel go1.21-da5d8fdd0c Thu Jun 29 15:35:27 2023 +0000 darwin/arm64
$ GOTOOLCHAIN=go1.21rc2+auto go version 
go: invalid GOTOOLCHAIN "go1.21rc2+auto": invalid minimum toolchain "go1.21rc2"

A quick look at the code suggests at least part of the problem is that gover.FromToolchain is being called with the wrong argument:

go/src/cmd/go/internal/toolchain/select.go

Lines 132 to 134 in 03cd8a7

min, suffix, plus := strings.Cut(gotoolchain, "+") // go1.2.3+auto
if min != "local" {
v := gover.FromToolchain(gotoolchain)

Instead of the intended gover.FromToolchain(min). I haven't looked if there's more to it or not.

CC @rsc, @bcmills, @matloob.
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go release-blocker labels Jun 29, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Jun 29, 2023
@gopherbot
Copy link

Change https://go.dev/cl/507595 mentions this issue: cmd/go: accept + sign in GOTOOLCHAIN value as documented

@iwdgo
Copy link
Contributor

iwdgo commented Jul 4, 2023

The reported behavior is explicitly described in the code documentation. With something like "go1.21rc2", x contains "rc2" and an empty version is returned. An empty version triggers the error message. One could argue that go1.21 does not exist before go1.21rc2 restricting this case to somebody using the patch (rc) rather then the release.

go/src/cmd/go/internal/gover/gover.go

Lines 168 to 175 in 03cd8a7

if !ok || x != "" {
// Note that we are disallowing prereleases (alpha, beta, rc) for patch releases here (x != "").
// Allowing them would be a bit confusing because we already have:
// 1.21 < 1.21rc1
// But a prerelease of a patch would have the opposite effect:
// 1.21.3rc1 < 1.21.3
// We've never needed them before, so let's not start now.
return version{}

Good to know, as go1.21 is not released, a dev version is returned.

$ GOTOOLCHAIN=go1.21+auto goissue version
go version devel go1.21-5b72f45dd1 Fri Jun 30 22:02:00 2023 +0000 windows/amd64

@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 4, 2023

@iwdgo That snippet talks about "prereleases (alpha, beta, rc) for patch releases", which isn't relevant here. It's true that GOTOOLCHAIN=go1.20.5rc3+auto shouldn't work, but this bug is about GOTOOLCHAIN=go1.20.5+auto not working even though it should.


# Works as expected without +auto suffix.
$ GOTOOLCHAIN=go1.20.5 go version
go version go1.20.5 darwin/arm64
$ GOTOOLCHAIN=go1.20rc3 go version 
go version go1.20rc3 darwin/arm64

# Fails to work with +auto suffix.
$ GOTOOLCHAIN=go1.20.5+auto go version
go: invalid GOTOOLCHAIN "go1.20.5+auto": invalid minimum toolchain "go1.20.5"
$ GOTOOLCHAIN=go1.20rc3+auto go version
go: invalid GOTOOLCHAIN "go1.20rc3+auto": invalid minimum toolchain "go1.20rc3"

@gopherbot
Copy link

Change https://go.dev/cl/508820 mentions this issue: cmd/go: fix GOTOOLCHAIN parsing for +auto

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 12, 2023
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
@rsc @bradfitz
cmd/go: fix GOTOOLCHAIN parsing for +auto
b995362 
The call from toolchain.Select to gover.FromToolchain was passing the
wrong argument but this was masked by gover.IsValid being a little bit
too lax.

Fix and test IsValid, which then breaks the existing gotoolchain_local
test, and then fix toolchain.Select to fix the test.

Fixes golang#61068.

Change-Id: I505ceb227457d6a51bd5e47f009b2fb1010c0d1f
Reviewed-on: https://go-review.googlesource.com/c/go/+/508820
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
3 participants
@dmitshur @gopherbot @iwdgo