diff --git a/tools/bump/gomod.go b/tools/bump/gomod.go index 3a04533b..20fd2438 100644 --- a/tools/bump/gomod.go +++ b/tools/bump/gomod.go @@ -142,7 +142,7 @@ func sqliteAtom(ctx context.Context, r *repo) (string, error) { return "", err } - latest, err := latestVersion(ctx, modSqlite) + latest, err := latestVersion(ctx, modSqlite, before) if err != nil { return "", fmt.Errorf("%w: %w", errNoLockstepSource, err) } @@ -205,7 +205,19 @@ func moduleAtom(path string) func(context.Context, *repo) (string, error) { return "", err } - if _, err := r.nixRun(ctx, "go", "get", "-u", path); err != nil { //nolint:noinlineerr + // Resolve the target here rather than letting `go get -u` choose it. + // The go command takes the highest semver it is offered, which is how + // a stray tag on a fork ends up committed. + want, err := latestVersion(ctx, path, before) + if err != nil { + return "", err + } + + if want == before { + return "", nil + } + + if _, err := r.nixRun(ctx, "go", "get", "-u", path+"@"+want); err != nil { //nolint:noinlineerr return "", err } @@ -218,7 +230,7 @@ func moduleAtom(path string) func(context.Context, *repo) (string, error) { return "", nil } - return fmt.Sprintf("%s %s -> %s", path, before, after), nil + return describeChange(path, before, after), nil } } diff --git a/tools/bump/modproxy.go b/tools/bump/modproxy.go index eaa803f5..a996da31 100644 --- a/tools/bump/modproxy.go +++ b/tools/bump/modproxy.go @@ -7,17 +7,21 @@ import ( "fmt" "io" "net/http" + "strings" "time" "golang.org/x/mod/modfile" "golang.org/x/mod/module" + "golang.org/x/mod/semver" ) // proxyBase is the module mirror. It is deliberately the same source the go // command already trusts: immutable per version, so the go.mod read back here // is the one that will actually be resolved, not whatever a branch tip happens // to hold at fetch time. -const proxyBase = "https://proxy.golang.org" +// It is a var only so the tests can point it at a fake proxy; nothing changes +// it at runtime. +var proxyBase = "https://proxy.golang.org" const proxyTimeout = 30 * time.Second @@ -26,6 +30,14 @@ const proxyTimeout = 30 * time.Second // exact breakage the lockstep rule exists to prevent. var errNoLockstepSource = errors.New("lockstep source unavailable") +// errStrayTag means @latest sorts above the pinned version but was committed +// no later than it. +var errStrayTag = errors.New("newest tag is older than the pinned version") + +// errModuleMoved means the newest version under this path declares a different +// module path. +var errModuleMoved = errors.New("module has moved to a new path") + // fetch performs a GET and returns the body, or an error naming the status. func fetch(ctx context.Context, url string) ([]byte, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) @@ -55,30 +67,106 @@ func fetch(ctx context.Context, url string) ([]byte, error) { var errHTTPStatus = errors.New("unexpected status") -// latestVersion resolves a module's newest version through the proxy. -func latestVersion(ctx context.Context, path string) (string, error) { +// versionInfo is the proxy's @latest and @v/.info response. +type versionInfo struct { + Version string + Time time.Time +} + +// fetchInfo reads one of the proxy's info endpoints. ref is "@latest" or +// "@v/.info". +func fetchInfo(ctx context.Context, path, ref string) (versionInfo, error) { esc, err := module.EscapePath(path) if err != nil { - return "", fmt.Errorf("escaping %s: %w", path, err) + return versionInfo{}, fmt.Errorf("escaping %s: %w", path, err) } - body, err := fetch(ctx, proxyBase+"/"+esc+"/@latest") + body, err := fetch(ctx, proxyBase+"/"+esc+"/"+ref) + if err != nil { + return versionInfo{}, err + } + + var info versionInfo + if err := json.Unmarshal(body, &info); err != nil { //nolint:noinlineerr + return versionInfo{}, fmt.Errorf("decoding %s for %s: %w", ref, path, err) + } + + return info, nil +} + +// versionTime is when version of path was committed. Pseudo-versions carry it +// in their own name, so only tagged versions cost a round trip. +func versionTime(ctx context.Context, path, version string) (time.Time, error) { + if module.IsPseudoVersion(version) { + return module.PseudoVersionTime(version) + } + + esc, err := module.EscapeVersion(version) + if err != nil { + return time.Time{}, fmt.Errorf("escaping version %s: %w", version, err) + } + + info, err := fetchInfo(ctx, path, "@v/"+esc+".info") + + return info.Time, err +} + +// latestVersion resolves a module's newest usable version through the proxy. +// current is the version already in use, or "" when the module is not required +// yet. When nothing newer exists it returns current unchanged. +// +// Two things the proxy will hand back that the go command does not protect +// against, both of which have to be refused rather than committed: +// +// - A stray tag. Forks carry tags that sort above the branch the real work +// happens on, and @latest reports them. Semver says newer; the commit date +// says otherwise, and the commit date is the one telling the truth. +// - A module that has moved. Releases keep being tagged under the new path, +// the proxy still serves them under the old one, and go get then refuses +// the result with an error that does not say why. +func latestVersion(ctx context.Context, path, current string) (string, error) { + info, err := fetchInfo(ctx, path, "@latest") if err != nil { return "", err } - var info struct { - Version string `json:"Version"` - } - - if err := json.Unmarshal(body, &info); err != nil { //nolint:noinlineerr - return "", fmt.Errorf("decoding @latest for %s: %w", path, err) - } - if info.Version == "" { return "", fmt.Errorf("%w: empty @latest for %s", errNoLockstepSource, path) } + if !semver.IsValid(info.Version) { + return "", fmt.Errorf("%w: proxy returned %q for %s", errStrayTag, info.Version, path) + } + + if current != "" && semver.Compare(info.Version, current) <= 0 { + return current, nil + } + + f, err := modFileOf(ctx, path, info.Version) + if err != nil { + return "", err + } + + if f.Module != nil && f.Module.Mod.Path != path { + return "", fmt.Errorf("%w: %s@%s declares %s; change the import path by hand", + errModuleMoved, path, info.Version, f.Module.Mod.Path) + } + + if current == "" { + return info.Version, nil + } + + curTime, err := versionTime(ctx, path, current) + if err != nil { + return "", err + } + + if !info.Time.After(curTime) { + return "", fmt.Errorf("%w: %s@%s is dated %s, the pinned %s is dated %s", + errStrayTag, path, info.Version, info.Time.Format(time.DateOnly), + current, curTime.Format(time.DateOnly)) + } + return info.Version, nil } @@ -134,3 +222,56 @@ func partnerVersion(ctx context.Context, owner, ownerVersion, dep string) (strin return v, nil } + +// describeChange renders one module's move, as a compare link when the path +// names a repository a link can be built for. Reviewing fifty version numbers +// means opening fifty tabs otherwise. +func describeChange(path, from, to string) string { + link := compareLink(path, from, to) + if link == "" { + return fmt.Sprintf("%s %s -> %s", path, from, to) + } + + return fmt.Sprintf("%s %s -> [%s](%s)", path, from, to, link) +} + +// compareLink is the GitHub compare URL between two versions of a module, or +// "" when one cannot be built. The repository is the first three path +// segments; anything past that is a subdirectory with its own tag prefix. +func compareLink(path, from, to string) string { + if !strings.HasPrefix(path, "github.com/") { + return "" + } + + trimmed, _, ok := module.SplitPathVersion(path) + if !ok { + return "" + } + + parts := strings.Split(trimmed, "/") + if len(parts) < 3 { + return "" + } + + sub := strings.Join(parts[3:], "/") + + return fmt.Sprintf("https://%s/compare/%s...%s", + strings.Join(parts[:3], "/"), gitRef(sub, from), gitRef(sub, to)) +} + +// gitRef is the tag or commit one module version points at. A pseudo-version +// names a commit; a tagged version inside a subdirectory carries that +// subdirectory as a prefix. +func gitRef(sub, version string) string { + if module.IsPseudoVersion(version) { + if rev, err := module.PseudoVersionRev(version); err == nil { //nolint:noinlineerr + return rev + } + } + + if sub != "" { + return sub + "/" + version + } + + return version +} diff --git a/tools/bump/modproxy_test.go b/tools/bump/modproxy_test.go new file mode 100644 index 00000000..e8ec2f87 --- /dev/null +++ b/tools/bump/modproxy_test.go @@ -0,0 +1,276 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// fakeProxy serves the three module proxy endpoints the resolver reads. The +// interesting cases here are ones the real proxy only produces occasionally +// and never on demand, so they cannot be reached from a live lookup. +type fakeProxy struct { + latest map[string]versionInfo // module path -> @latest + info map[string]versionInfo // "path@version" -> .info + gomod map[string]string // "path@version" -> go.mod contents +} + +// serve points proxyBase at the fake for the rest of the test. +func (p fakeProxy) serve(t *testing.T) { + t.Helper() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := strings.TrimPrefix(r.URL.Path, "/") + + if mod, ok := strings.CutSuffix(path, "/@latest"); ok { + writeInfo(w, p.latest, mod) + + return + } + + mod, rest, ok := strings.Cut(path, "/@v/") + if !ok { + http.NotFound(w, r) + + return + } + + if version, ok := strings.CutSuffix(rest, ".info"); ok { + writeInfo(w, p.info, mod+"@"+version) + + return + } + + version, found := strings.CutSuffix(rest, ".mod") + if !found { + http.NotFound(w, r) + + return + } + + body, ok := p.gomod[mod+"@"+version] + if !ok { + http.NotFound(w, r) + + return + } + + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + + previous := proxyBase + proxyBase = srv.URL + + t.Cleanup(func() { proxyBase = previous }) +} + +func writeInfo(w http.ResponseWriter, from map[string]versionInfo, key string) { + info, ok := from[key] + if !ok { + http.Error(w, "not found", http.StatusNotFound) + + return + } + + body, err := json.Marshal(info) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + + return + } + + _, _ = w.Write(body) +} + +func day(s string) time.Time { + t, err := time.Parse(time.DateOnly, s) + if err != nil { + panic(err) + } + + return t +} + +func modOf(path string) string { + return "module " + path + "\n\ngo 1.24\n" +} + +func TestLatestVersion(t *testing.T) { + const path = "github.com/example/thing" + + tests := []struct { + name string + proxy fakeProxy + current string + want string + wantErr error + }{ + { + name: "upgrade", + proxy: fakeProxy{ + latest: map[string]versionInfo{path: {Version: "v1.3.0", Time: day("2026-09-01")}}, + info: map[string]versionInfo{path + "@v1.2.0": {Version: "v1.2.0", Time: day("2026-06-01")}}, + gomod: map[string]string{path + "@v1.3.0": modOf(path)}, + }, + current: "v1.2.0", + want: "v1.3.0", + }, + { + name: "nothing newer", + proxy: fakeProxy{ + latest: map[string]versionInfo{path: {Version: "v1.2.0", Time: day("2026-06-01")}}, + }, + current: "v1.2.0", + want: "v1.2.0", + }, + { + // A fork carrying a tag that sorts above the branch real work + // happens on. Semver says newer, the commit date says otherwise. + name: "stray tag dated before the pinned version", + proxy: fakeProxy{ + latest: map[string]versionInfo{path: {Version: "v0.91.0", Time: day("2024-02-01")}}, + info: map[string]versionInfo{ + path + "@v0.90.0": {Version: "v0.90.0", Time: day("2026-06-01")}, + }, + gomod: map[string]string{path + "@v0.91.0": modOf(path)}, + }, + current: "v0.90.0", + wantErr: errStrayTag, + }, + { + // The pinned version is a pseudo-version, so its date comes out of + // the version string itself with no round trip. + name: "stray tag against a pseudo-version", + proxy: fakeProxy{ + latest: map[string]versionInfo{path: {Version: "v1.9.0", Time: day("2024-01-01")}}, + gomod: map[string]string{path + "@v1.9.0": modOf(path)}, + }, + current: "v1.8.1-0.20260601120000-abcdef123456", + wantErr: errStrayTag, + }, + { + name: "module moved to a new path", + proxy: fakeProxy{ + latest: map[string]versionInfo{path: {Version: "v2.0.0", Time: day("2026-09-01")}}, + gomod: map[string]string{path + "@v2.0.0": modOf("example.org/thing")}, + }, + current: "v1.2.0", + wantErr: errModuleMoved, + }, + { + name: "not previously required", + proxy: fakeProxy{ + latest: map[string]versionInfo{path: {Version: "v1.0.0", Time: day("2026-09-01")}}, + gomod: map[string]string{path + "@v1.0.0": modOf(path)}, + }, + current: "", + want: "v1.0.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.proxy.serve(t) + + got, err := latestVersion(context.Background(), path, tt.current) + + if tt.wantErr != nil { + if !errors.Is(err, tt.wantErr) { + t.Fatalf("latestVersion() error = %v, want %v", err, tt.wantErr) + } + + return + } + + if err != nil { + t.Fatalf("latestVersion() error = %v", err) + } + + if got != tt.want { + t.Errorf("latestVersion() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestCompareLink(t *testing.T) { + tests := []struct { + name string + path string + from string + to string + want string + }{ + { + name: "plain repository", + path: "github.com/spf13/cobra", + from: "v1.8.0", + to: "v1.9.0", + want: "https://github.com/spf13/cobra/compare/v1.8.0...v1.9.0", + }, + { + name: "major version suffix is not part of the repository", + path: "github.com/oapi-codegen/oapi-codegen/v2", + from: "v2.7.1", + to: "v2.8.0", + want: "https://github.com/oapi-codegen/oapi-codegen/compare/v2.7.1...v2.8.0", + }, + { + name: "subdirectory tags carry the subdirectory", + path: "github.com/example/repo/sub/mod", + from: "v1.0.0", + to: "v1.1.0", + want: "https://github.com/example/repo/compare/sub/mod/v1.0.0...sub/mod/v1.1.0", + }, + { + name: "pseudo-versions name commits", + path: "github.com/example/repo", + from: "v0.0.0-20260101000000-aaaaaaaaaaaa", + to: "v0.0.0-20260201000000-bbbbbbbbbbbb", + want: "https://github.com/example/repo/compare/aaaaaaaaaaaa...bbbbbbbbbbbb", + }, + { + name: "non-github paths get no link", + path: "modernc.org/sqlite", + from: "v1.52.0", + to: "v1.58.0", + want: "", + }, + { + name: "a bare host is not a repository", + path: "github.com/example", + from: "v1.0.0", + to: "v1.1.0", + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := compareLink(tt.path, tt.from, tt.to); got != tt.want { + t.Errorf("compareLink() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestDescribeChange(t *testing.T) { + got := describeChange("github.com/spf13/cobra", "v1.8.0", "v1.9.0") + want := "github.com/spf13/cobra v1.8.0 -> " + + "[v1.9.0](https://github.com/spf13/cobra/compare/v1.8.0...v1.9.0)" + + if got != want { + t.Errorf("describeChange() = %q, want %q", got, want) + } + + plain := describeChange("modernc.org/sqlite", "v1.52.0", "v1.58.0") + if plain != "modernc.org/sqlite v1.52.0 -> v1.58.0" { + t.Errorf("describeChange() without a link = %q", plain) + } +} diff --git a/tools/bump/plan.go b/tools/bump/plan.go index 12072c7f..c69e9502 100644 --- a/tools/bump/plan.go +++ b/tools/bump/plan.go @@ -30,7 +30,7 @@ func planLines(ctx context.Context, r *repo) []string { if version, err := oapiVersion(r); err != nil { //nolint:noinlineerr lines = append(lines, "oapi-codegen: "+err.Error()) - } else if latest, err := latestVersion(ctx, "github.com/oapi-codegen/oapi-codegen/v2"); err != nil { //nolint:noinlineerr + } else if latest, err := latestVersion(ctx, "github.com/oapi-codegen/oapi-codegen/v2", version); err != nil { //nolint:noinlineerr lines = append(lines, "oapi-codegen: "+err.Error()) } else { lines = append(lines, gap("Makefile oapi-codegen", version, latest)) @@ -135,7 +135,7 @@ func planLockstep(ctx context.Context, r *repo) []string { want := have if pair.target == "" { - if latest, err := latestVersion(ctx, pair.owner); err == nil { //nolint:noinlineerr + if latest, err := latestVersion(ctx, pair.owner, have); err == nil { //nolint:noinlineerr want = latest } } diff --git a/tools/bump/report_test.go b/tools/bump/report_test.go index c731929c..550c7dd3 100644 --- a/tools/bump/report_test.go +++ b/tools/bump/report_test.go @@ -107,3 +107,22 @@ func TestSelector(t *testing.T) { }) } } + +// Detail lines are rendered as-is so a compare link stays a link. Wrapping +// them in backticks, the way the table cells are, would turn every entry in +// "What moved" back into text nobody can click. +func TestDetailKeepsCompareLinks(t *testing.T) { + link := "github.com/spf13/cobra v1.8.0 -> " + + "[v1.9.0](https://github.com/spf13/cobra/compare/v1.8.0...v1.9.0)" + + var sb strings.Builder + + writeDetails(&sb, []result{{ + Area: "gomod", + Change: change{Detail: []string{link}}, + }}) + + if got := sb.String(); !strings.Contains(got, "- "+link+"\n") { + t.Errorf("writeDetails() did not render the link unchanged:\n%s", got) + } +} diff --git a/tools/bump/tools.go b/tools/bump/tools.go index c343d81c..540b41a8 100644 --- a/tools/bump/tools.go +++ b/tools/bump/tools.go @@ -29,7 +29,7 @@ func applyOapiCodegen(ctx context.Context, r *repo) (change, error) { return change{}, err } - want, err := latestVersion(ctx, "github.com/oapi-codegen/oapi-codegen/v2") + want, err := latestVersion(ctx, "github.com/oapi-codegen/oapi-codegen/v2", have) if err != nil { return change{}, err }