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

x/mod: semver.Max does more than compute the max #32700

Closed
bcmills opened this issue Jun 19, 2019 · 8 comments
Closed

x/mod: semver.Max does more than compute the max #32700

bcmills opened this issue Jun 19, 2019 · 8 comments
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 19, 2019

Currently, semver.Max canonicalizes its arguments according to semver.Canonical:
https://play.golang.org/p/04IP9gNhFWH

To me, the name Max does not evoke “canonicalize”, and the action of canonicalizing is potentially fairly destructive: for example, if I have a version with a +incompatible build suffix, that version is canonical according to module.CanonicalVersion, but will not be preserved by a call to semver.Max.

@rsc points out that the canonicalization avoids ambiguity: v0.1.0+a and v0.1.0+b have the same precedence, so if we call semver.Max("v0.1.0+a", "v0.1.0+b") we would otherwise have to pick one arbitrarily, and semver.org says “Build metadata SHOULD be ignored when determining version precedence.”

Instead, I propose that Max should always return one of its arguments. Build metadata must comprise only the usual dot-separated ASCII identifiers, so if we have two versions with equal precedence it seems to me that we could break the tie using the usual rule for comparing pre-release suffixes.

CC @jayconrod @hyangah
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. modules labels Jun 19, 2019
@gopherbot gopherbot added this to the Unreleased milestone Jun 19, 2019
@bcmills bcmills mentioned this issue Dec 20, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/212200 mentions this issue: cmd/go: avoid erroneous canonicalization when trying to resolve imports using replacements

@gopherbot
Copy link

Change https://golang.org/cl/212202 mentions this issue: cmd/go/internal/modfetch/codehost: replace a dubious call to semver.Max

gopherbot pushed a commit that referenced this issue Dec 20, 2019
cmd/go: avoid erroneous canonicalization when trying to resolve impor…
751aea8 
…ts using replacements

Updates #32700
Fixes #33795

Change-Id: I16897a0a2f3aa2f0b0bf8cf8252f3f39eef2e7ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/212200
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Dec 20, 2019
cmd/go/internal/modfetch/codehost: replace a dubious call to semver.Max
3f51350 
The documentation for RecentTag indicates that it returns an actual
tag, not a canonicalized prefix+version blob equivalent to a tag,
so the canonicalization due to semver.Max seems like a bug here.

Fortunately, RecentTag is not currently ever actually used as a tag,
so the removal of metadata does not result in a user-facing bug.
Nonetheless, it may be a subtle source of confusion for maintainers
in the future.

Updates #32700

Change-Id: I525423c1c0c7ec7c36c09e53b180034474f74e5a
Reviewed-on: https://go-review.googlesource.com/c/go/+/212202
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bcmills bcmills mentioned this issue May 13, 2020
Closed
@dsymonds
Copy link
Contributor

Alternatively, replace Max with Less, which just returns a bool. It is trivial to wrap to get the existing Max semantics in existing code, and avoids the issue by not even trying to return one arg or the other.

@bcmills
Copy link
Contributor Author

bcmills commented May 14, 2020

@dsymonds, we already have Compare, which is even more ergonomic than Less in general. But it's true that we could just remove Max instead of providing a version of it with dubious semantics.

@dsymonds
Copy link
Contributor

Oh, I overlooked Compare. Yeah, Max seems extra pointless; callers who care can use Compare, then if they actually care about canonicalisation they can use Canonical explicitly.

@gopherbot
Copy link

Change https://golang.org/cl/269357 mentions this issue: semver: deprecate semver.Max

@gopherbot
Copy link

Change https://golang.org/cl/269338 mentions this issue: cmd/go: migrate away from semver.Max

gopherbot pushed a commit to golang/mod that referenced this issue Nov 12, 2020
semver: deprecate semver.Max
3bfbaad 
There are a number of calls to semver.Max in the wild, so let's not
delete it yet.

For golang/go#32700

Change-Id: I515b05f174195d1b82a931b92e21b83c370ef7c7
Reviewed-on: https://go-review.googlesource.com/c/mod/+/269357
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Nov 12, 2020
cmd/go: migrate away from semver.Max
e75aef8 
For #32700

Change-Id: Ib5cd7004e4558bebebc5f9e7c9263d720c590845
Reviewed-on: https://go-review.googlesource.com/c/go/+/269338
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@bcmills
Copy link
Contributor Author

bcmills commented Jul 30, 2021

Max was deprecated in CL 269357. At this point I don't see a pressing need to delete it, so I don't think there is anything left to do for this issue.

@bcmills bcmills closed this as completed Jul 30, 2021
@golang golang locked and limited conversation to collaborators Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dsymonds @bcmills @gopherbot