From 97ecc4321ec3069d405c04cb2dc3132b39ef732e Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 16 May 2019 12:38:41 -0400 Subject: [PATCH] cmd/go: don't panic when explaining lost upgrades due to downgrades If a user runs 'go get mod@vers' where the module transitively requires itself at a newer version, 'go get' attempts to perform a downgrade, which necessarily excludes the requested version of the module. Previously, we called mvs.BuildList with the requested module version as the target. This panicked because BuildList doesn't allow the target module (typically the main module) to require a newer version of itself. With this change, when we lose an upgrade due to a downgrade, we call mvs.BuildList through a wrapper that treats the lost module version as requirement of a synthetic root module, rather than the target module. This avoids the panic. This change also starts reporting errors when an upgraded module is lost entirely (downgrades caused the module to be completely removed from the build list). Fixes #31491 Change-Id: I70ca261c20af7553cad2d3b840a1eaf3d18a4191 Reviewed-on: https://go-review.googlesource.com/c/go/+/177602 Run-TryBot: Jay Conrod Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/modget/get.go | 63 +++++++++++++++---- src/cmd/go/internal/mvs/mvs.go | 9 ++- .../mod/example.com_newcycle_a_v1.0.0.txt | 10 +++ .../mod/example.com_newcycle_a_v1.0.1.txt | 10 +++ .../mod/example.com_newcycle_b_v1.0.0.txt | 8 +++ .../go/testdata/script/mod_get_newcycle.txt | 14 +++++ 6 files changed, 99 insertions(+), 15 deletions(-) create mode 100644 src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.1.txt create mode 100644 src/cmd/go/testdata/mod/example.com_newcycle_b_v1.0.0.txt create mode 100644 src/cmd/go/testdata/script/mod_get_newcycle.txt diff --git a/src/cmd/go/internal/modget/get.go b/src/cmd/go/internal/modget/get.go index bf87c4a0d1..7a5d550997 100644 --- a/src/cmd/go/internal/modget/get.go +++ b/src/cmd/go/internal/modget/get.go @@ -23,6 +23,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "sync" ) @@ -570,14 +571,23 @@ func runGet(cmd *base.Command, args []string) { } // Scan for any upgrades lost by the downgrades. - lost := make(map[string]string) - for _, m := range modload.BuildList() { - t := byPath[m.Path] - if t != nil && semver.Compare(m.Version, t.m.Version) != 0 { - lost[m.Path] = m.Version + var lostUpgrades []*query + var versionByPath map[string]string + if len(down) > 0 { + versionByPath = make(map[string]string) + for _, m := range modload.BuildList() { + versionByPath[m.Path] = m.Version } + for _, q := range byPath { + if v, ok := versionByPath[q.m.Path]; q.m.Version != "none" && (!ok || semver.Compare(v, q.m.Version) != 0) { + lostUpgrades = append(lostUpgrades, q) + } + } + sort.Slice(lostUpgrades, func(i, j int) bool { + return lostUpgrades[i].m.Path < lostUpgrades[j].m.Path + }) } - if len(lost) > 0 { + if len(lostUpgrades) > 0 { desc := func(m module.Version) string { s := m.Path + "@" + m.Version t := byPath[m.Path] @@ -590,19 +600,17 @@ func runGet(cmd *base.Command, args []string) { for _, d := range down { downByPath[d.Path] = d } + var buf strings.Builder fmt.Fprintf(&buf, "go get: inconsistent versions:") reqs := modload.Reqs() - for _, q := range queries { - if lost[q.m.Path] == "" { - continue - } + for _, q := range lostUpgrades { // We lost q because its build list requires a newer version of something in down. // Figure out exactly what. // Repeatedly constructing the build list is inefficient // if there are MANY command-line arguments, // but at least all the necessary requirement lists are cached at this point. - list, err := mvs.BuildList(q.m, reqs) + list, err := buildListForLostUpgrade(q.m, reqs) if err != nil { base.Fatalf("go: %v", err) } @@ -618,7 +626,12 @@ func runGet(cmd *base.Command, args []string) { if sep != "," { // We have no idea why this happened. // At least report the problem. - fmt.Fprintf(&buf, " ended up at %v unexpectedly (please report at golang.org/issue/new)", lost[q.m.Path]) + if v := versionByPath[q.m.Path]; v == "" { + fmt.Fprintf(&buf, " removed unexpectedly") + } else { + fmt.Fprintf(&buf, " ended up at %s unexpectedly", v) + } + fmt.Fprintf(&buf, " (please report at golang.org/issue/new)") } } base.Fatalf("%v", buf.String()) @@ -894,3 +907,29 @@ func (u *upgrader) Upgrade(m module.Version) (module.Version, error) { return module.Version{Path: m.Path, Version: info.Version}, nil } + +// buildListForLostUpgrade returns the build list for the module graph +// rooted at lost. Unlike mvs.BuildList, the target module (lost) is not +// treated specially. The returned build list may contain a newer version +// of lost. +// +// buildListForLostUpgrade is used after a downgrade has removed a module +// requested at a specific version. This helps us understand the requirements +// implied by each downgrade. +func buildListForLostUpgrade(lost module.Version, reqs mvs.Reqs) ([]module.Version, error) { + return mvs.BuildList(lostUpgradeRoot, &lostUpgradeReqs{Reqs: reqs, lost: lost}) +} + +var lostUpgradeRoot = module.Version{Path: "lost-upgrade-root", Version: ""} + +type lostUpgradeReqs struct { + mvs.Reqs + lost module.Version +} + +func (r *lostUpgradeReqs) Required(mod module.Version) ([]module.Version, error) { + if mod == lostUpgradeRoot { + return []module.Version{r.lost}, nil + } + return r.Reqs.Required(mod) +} diff --git a/src/cmd/go/internal/mvs/mvs.go b/src/cmd/go/internal/mvs/mvs.go index d1c3d8c08a..90f8f269b5 100644 --- a/src/cmd/go/internal/mvs/mvs.go +++ b/src/cmd/go/internal/mvs/mvs.go @@ -121,9 +121,6 @@ func BuildList(target module.Version, reqs Reqs) ([]module.Version, error) { func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) module.Version) ([]module.Version, error) { // Explore work graph in parallel in case reqs.Required // does high-latency network operations. - var work par.Work - work.Add(target) - type modGraphNode struct { m module.Version required []module.Version @@ -137,6 +134,7 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo haveErr int32 ) + var work par.Work work.Add(target) work.Do(10, func(item interface{}) { m := item.(module.Version) @@ -217,6 +215,11 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo // Construct the list by traversing the graph again, replacing older // modules with required minimum versions. if v := min[target.Path]; v != target.Version { + // TODO(jayconrod): there is a special case in modload.mvsReqs.Max + // that prevents us from selecting a newer version of a module + // when the module has no version. This may only be the case for target. + // Should we always panic when target has a version? + // See golang.org/issue/31491, golang.org/issue/29773. panic(fmt.Sprintf("mistake: chose version %q instead of target %+v", v, target)) // TODO: Don't panic. } diff --git a/src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.0.txt new file mode 100644 index 0000000000..829065df9f --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.0.txt @@ -0,0 +1,10 @@ +example.com/newcycle/a v1.0.0 + +Transitively requires v1.0.1 of itself via example.com/newcycle/b + +-- .mod -- +module example.com/newcycle/a + +require example.com/newcycle/b v1.0.0 +-- .info -- +{"Version":"v1.0.0"} diff --git a/src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.1.txt b/src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.1.txt new file mode 100644 index 0000000000..a03f4b49fd --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_newcycle_a_v1.0.1.txt @@ -0,0 +1,10 @@ +example.com/newcycle/a v1.0.1 + +Transitively requires itself via example.com/newcycle/b + +-- .mod -- +module example.com/newcycle/a + +require example.com/newcycle/b v1.0.0 +-- .info -- +{"Version":"v1.0.1"} diff --git a/src/cmd/go/testdata/mod/example.com_newcycle_b_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_newcycle_b_v1.0.0.txt new file mode 100644 index 0000000000..ff9e1f5ea5 --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_newcycle_b_v1.0.0.txt @@ -0,0 +1,8 @@ +example.com/newcycle/b v1.0.0 + +-- .mod -- +module example.com/newcycle/b + +require example.com/newcycle/a v1.0.1 +-- .info -- +{"Version":"v1.0.0"} diff --git a/src/cmd/go/testdata/script/mod_get_newcycle.txt b/src/cmd/go/testdata/script/mod_get_newcycle.txt new file mode 100644 index 0000000000..9616863383 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_get_newcycle.txt @@ -0,0 +1,14 @@ +env GO111MODULE=on + +# Download modules to avoid stderr chatter +go mod download example.com/newcycle/a@v1.0.0 +go mod download example.com/newcycle/a@v1.0.1 +go mod download example.com/newcycle/b@v1.0.0 + +go mod init m +! go get example.com/newcycle/a@v1.0.0 +cmp stderr stderr-expected + +-- stderr-expected -- +go get: inconsistent versions: + example.com/newcycle/a@v1.0.0 requires example.com/newcycle/a@v1.0.1 (not example.com/newcycle/a@v1.0.0) -- 2.50.0