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

x/website/internal/dl: version sorting doesn't sort pre-release versions #47367

Closed
nd opened this issue Jul 24, 2021 · 5 comments
Closed

x/website/internal/dl: version sorting doesn't sort pre-release versions #47367

nd opened this issue Jul 24, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nd
Copy link
Contributor

nd commented Jul 24, 2021

What did you do?

Ran curl "https://golang.org/dl/?mode=json&include=all" in console

What did you expect to see?

go1.17rc1 before go1.17beta1

What did you see instead?

go1.17rc1 after go1.17beta1

nd added a commit to nd/delve that referenced this issue Jul 24, 2021
@dmitshur dmitshur changed the title dl: site returns rc versions after beta x/website/internal/dl: API lists rc versions after beta Jul 24, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jul 24, 2021
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 24, 2021
@dmitshur dmitshur changed the title x/website/internal/dl: API lists rc versions after beta x/website/internal/dl: version sorting doesn't sort pre-release versions Jul 24, 2021
@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 24, 2021
@dmitshur
Copy link
Contributor

Thanks for the report.

The dl package implements fileOrder sorting, and versionLess is used to compare two versions. When both are pre-release versions with same major, minor components, the pre-release part of the version isn't being sorted.

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 16, 2021

When both are pre-release versions with same major, minor components, the pre-release part of the version isn't being sorted.

Hm, are you sure?

In parseVersion, the tail return value points to "beta1" or "rc1":

func parseVersion(v string) (maj, min int, tail string) {
        if i := strings.Index(v, "beta"); i > 0 {
                tail = v[i:]
                v = v[:i]
        }
        if i := strings.Index(v, "rc"); i > 0 {
                tail = v[i:]
                v = v[:i]
        }
        p := strings.Split(strings.TrimPrefix(v, "go1."), ".")
        maj, _ = strconv.Atoi(p[0])
        if len(p) < 2 {
                return
        }
        min, _ = strconv.Atoi(p[1])
        return
}

Then in versionLess, the two tails are compared with >=:

func versionLess(a, b string) bool {
        // Put stable releases first.
        if isStable(a) != isStable(b) {
                return isStable(a)
        }
        maja, mina, ta := parseVersion(a)
        majb, minb, tb := parseVersion(b)
        if maja == majb {
                if mina == minb {
                        return ta >= tb
                }
                return mina >= minb
        }
        return maja >= majb
}

This is then tested to be the case in TestFileOrder.

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 16, 2021

Ahh, actually what's happening is this, from filesToReleases:

                        if len(unstable) != 0 {
                                // Only show one (latest) unstable version,
                                // consider the older ones to be archived.
                                archive = append(archive, *r)
                                return
                        }

Basically, since archives are currently first, and since there's only one unstable release, it's placed in the wrong spot for include=all, leading to the illusion that the sort was done wrong.

In other words, the problem of this issue is fixed by #50201 and https://golang.org/cl/371934.

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 16, 2021

I'll add a test case to that CL and make sure it works for the case here.

@gopherbot
Copy link

Change https://golang.org/cl/371934 mentions this issue: internal/dl: show newer versions first in json listing

passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
When include=all is used, archived versions (old) and unstable versions
(new) are appended to the list of returned versions. This makes the
order of the list rather awkward: current, old, new. This commit makes
the ordering a bit more natural: new, current, old. However, even like
that, releases will be out of order, because, for example, 1.18rc1 is
new, while 1.17.6 is current, but 1.18beta1 is old. So, this commit
takes care to sort the entire list properly to avoid that. This way, the
entire list is sorted by version. Nothing changes semantically, as users
of the API can still filter out unstable releases using the "stable:
true" field.

This commit also adds a test case, to make sure this behavior doesn't
regress.

Fixes golang/go#50201.
Fixes golang/go#47367.

Change-Id: Ic136197dcdd47aa149601b75ba5152a2006c790a
Reviewed-on: https://go-review.googlesource.com/c/website/+/371934
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Jason Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Trust: Jamal Carvalho <jamalcarvalho@google.com>
@golang golang locked and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants