go/version: unclear as to what counts as a valid Go version #65061
Comments
|
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: Same issue happens with |
mauri870
added
the
NeedsInvestigation
|
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 |
|
The mention of toolchains in that comment is just a bug. Custom toolchains are not allowed in this API. What is the context where you need "go1.21.0-bigcorp" to work? |
|
Change https://go.dev/cl/558335 mentions this issue: |
|
@rsc Why is it a bug? It was a feature we explicitly discussed in the proposal. #62039 (comment) |
rsc
added
NeedsFix
|
@rittneje Thanks for pointing that out. My memory is clearly faulty! |
|
Change https://go.dev/cl/559796 mentions this issue: |
|
Reopen for backport. |
|
Change https://go.dev/cl/559802 mentions this issue: |
…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 <rsc@golang.org> Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/559802 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
Change https://go.dev/cl/562838 mentions this issue: |
|
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Run-TryBot: Tim King <taking@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com>
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 <rsc@golang.org> Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com> Reviewed-by: Bryan Mills <bcmills@google.com>
Go version
go version devel go1.22-e9b3ff15 Wed Jan 10 17:35:49 2024 +0000 darwin/amd64
Output of
go envin your module/workspace:What did you do?
I was playing around with the new
go/versionpackage 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:However, it also says:
What did you see happen?
My assumption going into this was that a "valid" version is one that returns a
truewhen passed intoIsValid. However,IsValidseems to reject custom toolchain names and names with+autoattached, see here: https://go.dev/play/p/OnEbA0bOovs?v=gotipWhat 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: