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

go/version: package for handling Go version strings #62039

Closed
mvdan opened this issue Aug 15, 2023 · 35 comments
Closed

go/version: package for handling Go version strings #62039

mvdan opened this issue Aug 15, 2023 · 35 comments

Comments

@mvdan
Copy link
Member

mvdan commented Aug 15, 2023

go.mod files have had language version directives for a while, like go 1.20. In Go 1.21, they started allowing more specific versions such as go 1.21.0 or go 1.21rc3.

Some tools like https://github.com/mvdan/gofumpt want to fetch the language version from go.mod, to for example see if a module can use new syntax in Go 1.21 or not. This was easy enough when the format was go 1.X, where X is an integer. Now it's harder - a bit of code would have to be written to consider the trailing patch release number, pre-release suffixes like RCs or betas, etc.

In #61692 (comment), @findleyr and I seemed to agree that we should expose some API to deal with these version strings, since they are particular to Go and don't follow a standard like semver.

For example, how about a function to allow converting a Go version to semver:

package modfile

func SemverVersion(version string) string

Then, SemverVersion("1.21rc3") == "v1.21-rc.3", for example. It could also accept a go prefix, like go1.21rc3, since that is used in some places like git tag names - those don't appear in go.mod files, but I think it would be useful too.

Also see mvdan/gofumpt#280 for the issue on gofumpt's side - it for now continues to panic if it sees any "complex" Go version it can't deal with.

cc @dominikh since I think he also needs something like this API for staticcheck.

@mvdan
Copy link
Member Author

mvdan commented Aug 15, 2023

Perhaps https://pkg.go.dev/golang.org/x/mod/semver would be a better package to place this in, since these Go versions appear in multiple places and not just go.mod files. For example, they are used for Go release archives and git tags as well. In that package, the name could be semver.FromGo, for example.

@findleyr
Copy link
Contributor

Thanks for writing this up. I think semver.FromGo (or perhaps semver.FromGoVersion) makes sense.

Would the API simply ignore the "go" prefix if present, i.e. accept both "1.21.0" and "go1.21.0"?

@mvdan
Copy link
Member Author

mvdan commented Aug 15, 2023

I think so, given that sometimes the prefix is present and some other times it's not.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Aug 15, 2023
@dmitshur
Copy link
Contributor

CC @rsc, @bcmills, @matloob.

@dmitshur
Copy link
Contributor

Per https://go.dev/s/proposal#scope, perhaps this is notable enough to go through the proposal process?

@findleyr findleyr changed the title x/mod: add API to parse Go distribution version strings proposal: x/mod: add API to parse Go distribution version strings Aug 15, 2023
@dmitshur dmitshur removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Aug 15, 2023
@dmitshur dmitshur modified the milestones: Unreleased, Proposal Aug 15, 2023
@mvdan
Copy link
Member Author

mvdan commented Aug 15, 2023

I don't have a strong opinion regarding this issue being a proposal or not, but note that some tools are broken by this change in 1.21 for the time being, so reaching a decision sooner than later would be best.

@findleyr
Copy link
Contributor

@mvdan the policy is that any new API in x/ repos must be a proposal.

Since this is unlikely to be controversial, I wouldn't expect it to spend very long in the proposal commitee. In the meantime, tools can write their own parser if necessary.

@rsc
Copy link
Contributor

rsc commented Aug 15, 2023

Converting from Go version to Semver typically requires converting back and I would rather not go down that road. (I tried, it was bad.) If we were going to do something here, we'd want something like cmd/go/internal/gover or the parts that I copied into https://cs.opensource.google/go/x/build/+/17290641:cmd/gorebuild/gover.go.

@mvdan
Copy link
Member Author

mvdan commented Aug 15, 2023

I don't personally need to convert back, but I can see that argument. All I wanted was to continue comparing with semver.Compare, so directly comparing Go-style versions is also perfectly fine for my use case.

@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

Proposed API:

package gover // import "golang.org/x/mod/gover"

// Go returns the version string for Go N.M ("goN.M")
func Go(N, M int) string

// Compare returns -1, 0, or +1 depending on whether
// x < y, x == y, or x > y, interpreted as Go versions.
// The versions x and y must begin with a "go" prefix: "go1.21" not "1.21".
// Malformed versions, including the empty string, compare less than well-formed versions and equal to each other.
// The language version "go1.21" compares less than the release candidate and eventual releases "go1.21rc1" and "go1.21.0".
func Compare(x, y string) int

// IsValid reports whether the version x is valid.
func IsValid(x string) bool

Note that if you want to use this with the go line arguments in go.mod you need to add the "go" prefix yourself. In general we are not entirely consistent about this, but the strings are a bit more self-describing with the prefix than without. And accepting either form leads to significant confusion (I made that mistake too).

@findleyr
Copy link
Contributor

findleyr commented Aug 16, 2023

Thanks for writing that up, @rsc.

I don't immediately see the need for the Go function, nor why it only accepts two numbers rather than three.

Frequently, when I use the x/mod/semver package, I wish that parse were exposed. I understand that the proposed API is consistent with x/mod/semver, but I'd personally prefer to replace IsValid with something like:

// A Version is a parsed Go version: major[.minor[.patch]][kind[pre]].
type Version struct {
	Major string // decimal
	Minor string // decimal or ""
	Patch string // decimal or ""
	Kind  string // "", "alpha", "beta", "rc"
	Pre   string // decimal or ""
}

// Parse parses the Go version v.
//
// The second result reports whether the version is valid.
func Parse(v string) (Version, bool)

@dmitshur
Copy link
Contributor

dmitshur commented Aug 16, 2023

Some comments on the suggested API.

Compare is the most important bit, and I think its API works well. It's very flexible, without all the complexity that arises when trying to return parsed version components to the caller.

IsValid seems medium-useful, and can be improved with if it comes with some definition of what a valid Go version is. A non-exhaustive set of questions about its behavior I don't understand without having to jump into the implementations: is "go1" valid? is "go1.0" not valid? what about "go1.19beta1" or "go1.9.2rc2"?

IsValid can also be implemented by callers with Compare(x, "") != 0 or similar, but it's still probably useful to provide it so the callers don't need to do that.

Finally, consider leaving out Go. It seems too trivial to be useful to callers. At least I don't understand the benefit of using the gover package for it compared to just creating the version string manually. That makes it confusing. (Can the version string for Go N.M ever be something other than "goN.M"?)

@rittneje
Copy link

There is one point about the new Compare func I'd like some clarification on. I'm guessing that in some cases people will pass runtime.Version() as one of the parameters in order to determine if the current toolchain is new enough. However, on rare occasion, we do have to make our own patch to the toolchain, and we also amend the VERSION file when we do this, so we end with a toolchain version like "go1.21.0-patch1".

Will such a thing be considered malformed, and thus break the feature in question? Is there a way we can format our patch version such that, in this example, it would sort above 1.21.0 and below 1.21.1?

@rsc
Copy link
Contributor

rsc commented Sep 19, 2023

Sorry, this fell off my radar. Added to my list to move to Active at tomorrow's meeting.

@rsc
Copy link
Contributor

rsc commented Sep 19, 2023

Responding to @findleyr, I'd rather not provide Parse. We don't even provide it in the cmd/go/internal/gover package (nor semver). It is too much exposed API that makes everything else too hard to change. We can add func Lang(version string) string to turn "go1.21.4" into "go1.21" though. I imagine that's the most common thing missing from the current API.

@rsc
Copy link
Contributor

rsc commented Sep 19, 2023

Responding to @dmitshur, Go turns out to be a very important function because the most common use of Compare is

if gover.Compare(version, gover.Go(1, 5)) >= 0 { // version >= Go 1.5

and that makes sure you don't end up with magic strings that may or may not have the right form (people will inevitably mistype "1.5" for "go1.5" or vice versa, depending on which is the accepted form).

@rsc
Copy link
Contributor

rsc commented Sep 19, 2023

Responding to @rittneje, there is no way to create a version string that compares between go1.21.0 and go1.21.1.
Go versions are simply not semver with its infinitely fine-grained detail.

It is fine to have go1.21.0-bigcorp for bigcorp's internal version of go1.21.0. That compares == go1.21.0 for the purposes of determining version ordering.

@rittneje
Copy link

@rsc Let's imagine that we have to make some patch of go1.21.0. So we modify the VERSION file to go1.21.0-bigcorp, so now that is what go version reports.

Now I want to enforce that our developers use the patch version, not the original version. If my go.mod says "go1.21.0-bigcorp", and a developer is using vanilla "go1.21.0", what happens? If as you say they are considered equal, that is not desirable.

In short, I think the following properties would be desirable in this use case:

  1. go1.21.0 < go1.21.0-X < go1.21.1
  2. go1.21.0-X == go1.21.0-X
  3. go1.21.0-X != go1.21.0-Y (no defined relative ordering)

@rittneje
Copy link

rittneje commented Sep 20, 2023

Thinking on it more, I guess we cannot assume anything about the relationship between 1.21.0-X and 1.21.1. The latter might include whatever custom changes from the former, or it might not.

  1. go1.21.0 < go1.21.1
  2. go1.21.0 < go1.21.0-X
  3. go1.21.0-X != go1.21.1 (no defined relative ordering)
  4. go1.21.0-X == go1.21.0-X
  5. go1.21.0-X != go1.21.0-Y (no defined relative ordering)

Of course, I'm not sure what Compare should do in the "no defined relative ordering" cases. The caller may be assuming that A < B implies that B > A and thus could provide the parameters in either order. :/

So I guess it should just document that these custom suffixes are going to be ignored for the comparison, and if that matters don't use it. (It's especially important to highlight they won't be considered "malformed".)

@rsc
Copy link
Contributor

rsc commented Oct 6, 2023

@mvdan, done.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2023

The trailing .0 is optional in the arguments, but note that Compare("go1.21", "go1.21.0") < 0.

@mvdan
Copy link
Member Author

mvdan commented Oct 6, 2023

Interesting. That makes sense today and I understand the logic.

However, we used to not have those .0 suffixes on final releases, and for those older Go releases the logic seems backwards. For example, Compare("go1.17", "go1.17rc1") < 0, when the reality back then was the opposite - go1.17 was the final release which followed all RCs.

@rsc
Copy link
Contributor

rsc commented Oct 11, 2023

@mvdan I believe the code understands that subtlety. I'd need to double-check.

@rsc
Copy link
Contributor

rsc commented Oct 11, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rittneje
Copy link

@rsc can the doc comments be amended to explicitly mention the stuff about custom patch versions? Namely, they are considered valid versions and are equal to the regular version without the custom suffix?

@rsc
Copy link
Contributor

rsc commented Oct 12, 2023

Sure, we can update those doc comments.

@rsc
Copy link
Contributor

rsc commented Oct 26, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal details are as follows.

New package with this API:

// Package version provides operations on [Go version strings].
//
// [Go version strings]: https://go.dev/doc/toolchain#version
package version // import "go/version"

// Go returns the version string for Go N.M ("goN.M").
// 
// The most common use of Go is with [Compare]:
//
//     if version.Compare(v, version.Go(1, 21)) >= 0 {
//         // v is Go 1.21 or newer
//     }
func Go(N, M int) string

// Lang returns the Go language version for version x.
// If x is not a valid version, Lang returns the empty string.
// For example:
//
//     Lang("go1.21rc2") = "go1.21".
//     Lang("go1.21.2") = "go1.21".
//     Lang("go1.21") = "go1.21".
//     Lang("bad") = ""
//     Lang("1.21") = ""
func Lang(x string) string

// Compare returns -1, 0, or +1 depending on whether
// x < y, x == y, or x > y, interpreted as Go versions.
// The versions x and y must begin with a "go" prefix: "go1.21" not "1.21".
// Invalid versions, including the empty string, compare less than 
// valid versions and equal to each other.
// The language version "go1.21" compares less than the 
// release candidate and eventual releases "go1.21rc1" and "go1.21.0".
// Custom toolchain suffixes are ignored during comparison:
// "go1.21.0" and "go1.21.0-bigcorp" are equal.
func Compare(x, y string) int

// IsValid reports whether the version x is valid.
func IsValid(x string) bool

@rsc rsc changed the title proposal: go/version: package for handling Go version strings go/version: package for handling Go version strings Oct 26, 2023
@rsc rsc modified the milestones: Proposal, Backlog Oct 26, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/538895 mentions this issue: go/version: add new package

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Nov 1, 2023
@rsc
Copy link
Contributor

rsc commented Nov 3, 2023

I deleted func Go from the API in the most recent version of the CL. The code in cmd/go was written carefully never to allocate, and the Go func breaks that rule. In particular the "idiom" version.Compare(v, version.Go(1, 2)) is an allocation on every call, which is terrible. I was worried about confusion about whether to write version.Compare(v, "go1.2") versus version.Compare(v, "1.2"), but that confusion should be fairly uncommon. (It's more common in the go command itself, because the 'go' line in go.mod says 'go 1.2' not 'go go1.2'.)

We can always decide to add func Go back later.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/547695 mentions this issue: doc: add release note for new go/version package

gopherbot pushed a commit that referenced this issue Dec 5, 2023
For #62039.

Change-Id: Id19a4c06489ad24dc44c7caf2405d155d96c6fcb
Reviewed-on: https://go-review.googlesource.com/c/go/+/547695
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Bypass: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
@rsc rsc reopened this Jan 31, 2024
@rsc rsc closed this as completed Jan 31, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/559799 mentions this issue: doc/go1.22: move go/version package mention into a separate heading

gopherbot pushed a commit that referenced this issue Jan 31, 2024
It's a new package in the standard library,
not a minor change to an existing package.

For #62039.
For #61422.

Change-Id: I7488304cd2bd6353f535cab192d015796840ba4e
Reviewed-on: https://go-review.googlesource.com/c/go/+/559799
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
For golang#62039.

Change-Id: Id19a4c06489ad24dc44c7caf2405d155d96c6fcb
Reviewed-on: https://go-review.googlesource.com/c/go/+/547695
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Bypass: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
It's a new package in the standard library,
not a minor change to an existing package.

For golang#62039.
For golang#61422.

Change-Id: I7488304cd2bd6353f535cab192d015796840ba4e
Reviewed-on: https://go-review.googlesource.com/c/go/+/559799
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

6 participants