go/version: unclear as to what counts as a valid Go version #65061

MadhavJivrajani · 2024-01-11T11:47:51Z

Go version

go version devel go1.22-e9b3ff15 Wed Jan 10 17:35:49 2024 +0000 darwin/amd64

Output of `go env` in your module/workspace:

❯ gotip env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/mjivrajani/Library/Caches/go-build'
GOENV='/Users/mjivrajani/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/mjivrajani/gocode/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/mjivrajani/gocode'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/mjivrajani/sdk/gotip'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/mjivrajani/sdk/gotip/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='devel go1.22-e9b3ff15 Wed Jan 10 17:35:49 2024 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/mjivrajani/gocode/src/github.com/MadhavJivrajani/go/src/go.mod'
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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/r7/f6kyp7vs0sv90105pjh1t8340000gp/T/go-build3111256134=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I was playing around with the new go/version package to see how it works.

I'm probably missing something, but I'm not entirely sure what a "valid" Go version entails - from the documentation of Compare:

Invalid versions, including the empty string, compare less than valid versions

However, it also says:

Custom toolchain suffixes are ignored during comparison: "go1.21.0" and "go1.21.0-bigcorp" are equal.

What did you see happen?

My assumption going into this was that a "valid" version is one that returns a true when passed into IsValid. However, IsValid seems to reject custom toolchain names and names with +auto attached, see here: https://go.dev/play/p/OnEbA0bOovs?v=gotip

What did you expect to see?

Maybe clarifying the docs would help better understand what a valid Go version is for folks other than me who might also make similar assumptions and I'm happy to send in a fix here! And thank you for introducing this package, we are already in the works of getting rid of custom parsing code in the Kubernetes codebase! ❤️

The text was updated successfully, but these errors were encountered:

mauri870 · 2024-01-11T12:28:45Z

Looks like the description of Compare is not really correct, I don't think we are ignoring the suffixes, if it finds a custom toolchain suffix we just return an empty version so comparison always fails:

https://go.googlesource.com/go/+/e9b3ff15f40d6b258217b3467c662f816b078477/src/internal/gover/gover.go#137

Same issue happens with x/tools/internal/versions

mauri870 · 2024-01-11T14:56:03Z

We might be able to fix this by returning the proper version if it is a custom toolchain, eg "go1.21.3-corp" returns "1.21.3" instead of an empty version. That would fix Compare but will also affect all other usages of internal/gover.Parse, so I'm not sure if it would break anything.

diff --git a/src/internal/gover/gover.go b/src/internal/gover/gover.go
index 2ad068464d..151ff674fa 100644
--- a/src/internal/gover/gover.go
+++ b/src/internal/gover/gover.go
@@ -133,7 +133,7 @@ func Parse(x string) Version {
 	// Parse patch if present.
 	if x[0] == '.' {
 		v.Patch, x, ok = cutInt(x[1:])
-		if !ok || x != "" {
+		if !ok || (x != "" && x[0] != '-') {
 			// 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

rsc · 2024-01-24T20:09:46Z

The mention of toolchains in that comment is just a bug. Custom toolchains are not allowed in this API.
This API is for things like deciding that go1.3 < go1.21.5.

What is the context where you need "go1.21.0-bigcorp" to work?

gopherbot · 2024-01-24T21:37:46Z

Change https://go.dev/cl/558335 mentions this issue: go/version: correct docs for toolchain names

rittneje · 2024-01-24T22:07:46Z

@rsc Why is it a bug? It was a feature we explicitly discussed in the proposal. #62039 (comment)

rsc · 2024-01-29T20:17:28Z

@rittneje Thanks for pointing that out. My memory is clearly faulty!

gopherbot · 2024-01-31T16:53:34Z

Change https://go.dev/cl/559796 mentions this issue: go/version: fix package to accept go1.21.0-bigcorp

rsc · 2024-01-31T19:58:05Z

Reopen for backport.

gopherbot · 2024-01-31T19:59:09Z

Change https://go.dev/cl/559802 mentions this issue: [release-branch.go1.22] go/version: fix package to accept go1.21.0-bigcorp

…gcorp The proposal discussion made clear that suffixes should be accepted, so that people who use custom VERSION files can still pass runtime.Version() to this code. But we forgot to do that in the CL. Do that. Note that cmd/go also strips space- and tab-prefixed suffixes, but go.dev/doc/toolchain only mentions dash, so this code only strips dash. Fixes #65061. Change-Id: I6a427b78f964eb41c024890dae30223beaef13eb Cq-Include-Trybots: luci.golang.try:go1.22-linux-amd64-longtest,go1.22-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/559796 TryBot-Bypass: Russ Cox <[email protected]> Reviewed-by: Mauri de Souza Meneguzzo <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/go/+/559802 LUCI-TryBot-Result: Go LUCI <[email protected]>

gopherbot · 2024-02-09T17:41:49Z

Change https://go.dev/cl/562838 mentions this issue: internal/versions: fix package to accept go1.21.0-bigcorp

timothy-king · 2024-02-09T18:18:28Z

Reopening for x/tools

Adds support for suffixes to x/tools copy of go/version. Brings in go.dev/cl/559796. Updates golang/go#65061 Change-Id: Iaa0c98a73d8ddd8a42f0c4d3df7d4d79eb7aeb0a Reviewed-on: https://go-review.googlesource.com/c/tools/+/562838 LUCI-TryBot-Result: Go LUCI <[email protected]> Run-TryBot: Tim King <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Bryan Mills <[email protected]>

The proposal discussion made clear that suffixes should be accepted, so that people who use custom VERSION files can still pass runtime.Version() to this code. But we forgot to do that in the CL. Do that. Note that cmd/go also strips space- and tab-prefixed suffixes, but go.dev/doc/toolchain only mentions dash, so this code only strips dash. Fixes golang#65061. Change-Id: I6a427b78f964eb41c024890dae30223beaef13eb Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/559796 TryBot-Bypass: Russ Cox <[email protected]> Reviewed-by: Mauri de Souza Meneguzzo <[email protected]> Reviewed-by: Bryan Mills <[email protected]>

dmitshur · 2024-05-30T21:01:21Z

This was done for x/tools in CL 562838.

This issue is in Go1.22 milestone, so closing again. (It was fixed by CL 559796.)

mauri870 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 11, 2024

rsc added NeedsFix The path to resolution is known, but the work has not been done. release-blocker and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 29, 2024

rsc added this to the Go1.22 milestone Jan 29, 2024

rsc self-assigned this Jan 29, 2024

gopherbot closed this as completed in 7973821 Jan 31, 2024

rsc reopened this Jan 31, 2024

mknyszek closed this as completed Feb 1, 2024

timothy-king reopened this Feb 9, 2024

dmitshur closed this as completed May 30, 2024

gabyhelp mentioned this issue Jul 29, 2024

go/version: IsValid reports false on inputs like "go1.8.5rc5", "go1.9.2rc2" #68634

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/version: unclear as to what counts as a valid Go version #65061

go/version: unclear as to what counts as a valid Go version #65061

MadhavJivrajani commented Jan 11, 2024

mauri870 commented Jan 11, 2024 •

edited

Loading

mauri870 commented Jan 11, 2024 •

edited

Loading

rsc commented Jan 24, 2024

gopherbot commented Jan 24, 2024

rittneje commented Jan 24, 2024

rsc commented Jan 29, 2024

gopherbot commented Jan 31, 2024

rsc commented Jan 31, 2024

gopherbot commented Jan 31, 2024

gopherbot commented Feb 9, 2024

timothy-king commented Feb 9, 2024

dmitshur commented May 30, 2024

go/version: unclear as to what counts as a valid Go version #65061

go/version: unclear as to what counts as a valid Go version #65061

Comments

MadhavJivrajani commented Jan 11, 2024

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

mauri870 commented Jan 11, 2024 • edited Loading

mauri870 commented Jan 11, 2024 • edited Loading

rsc commented Jan 24, 2024

gopherbot commented Jan 24, 2024

rittneje commented Jan 24, 2024

rsc commented Jan 29, 2024

gopherbot commented Jan 31, 2024

rsc commented Jan 31, 2024

gopherbot commented Jan 31, 2024

gopherbot commented Feb 9, 2024

timothy-king commented Feb 9, 2024

dmitshur commented May 30, 2024

Output of `go env` in your module/workspace:

mauri870 commented Jan 11, 2024 •

edited

Loading

mauri870 commented Jan 11, 2024 •

edited

Loading