From eb982727e33263c0bb67de607beb44c5e0bd2bea Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 28 Jan 2021 09:10:57 -0500 Subject: [PATCH] cmd/go/internal/mvs: fix Downgrade to match Algorithm 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mvs.Downgrade is pretty clearly intended to match Algorithm 4 from the MVS blog post (https://research.swtch.com/vgo-mvs#algorithm_4). Per the blog post: “Downgrading one module may require downgrading other modules, but we want to downgrade as few other modules as possible. … To avoid an unnecessary downgrade to E 1.1, we must also add a new requirement on E 1.2. We can apply Algorithm R to find the minimal set of new requirements to write to go.mod.” mvs.Downgrade does not match that behavior today: it fails to retain the selected versions of transitive dependencies that are not implied by downgraded direct dependencies of the target (module E in the post). This bug is currently masked by the fact that we only call Downgrade today with a *modload.mvsReqs, for which the Required method happens to return the complete build list — rather than only the direct dependencies as documented for the mvs.Reqs interface. For #36460 Change-Id: If9c8f413b156b5f67c02787d9359394e169951b0 Reviewed-on: https://go-review.googlesource.com/c/go/+/287633 Trust: Bryan C. Mills Reviewed-by: Michael Matloob Reviewed-by: Jay Conrod --- src/cmd/go/internal/mvs/mvs.go | 27 +++++++++++++++---- src/cmd/go/internal/mvs/mvs_test.go | 19 +++++-------- .../go/testdata/script/mod_load_badchain.txt | 3 +-- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/cmd/go/internal/mvs/mvs.go b/src/cmd/go/internal/mvs/mvs.go index f016d8ff15d..bed4d5c1ba5 100644 --- a/src/cmd/go/internal/mvs/mvs.go +++ b/src/cmd/go/internal/mvs/mvs.go @@ -375,10 +375,19 @@ func Upgrade(target module.Version, reqs Reqs, upgrade ...module.Version) ([]mod // reqs.Previous, but the methods of reqs must otherwise handle such versions // correctly. func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([]module.Version, error) { - list, err := reqs.Required(target) + // Per https://research.swtch.com/vgo-mvs#algorithm_4: + // “To avoid an unnecessary downgrade to E 1.1, we must also add a new + // requirement on E 1.2. We can apply Algorithm R to find the minimal set of + // new requirements to write to go.mod.” + // + // In order to generate those new requirements, we need to identify versions + // for every module in the build list — not just reqs.Required(target). + list, err := BuildList(target, reqs) if err != nil { return nil, err } + list = list[1:] // remove target + max := make(map[string]string) for _, r := range list { max[r.Path] = r.Version @@ -411,6 +420,9 @@ func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([ } added[m] = true if v, ok := max[m.Path]; ok && reqs.Max(m.Version, v) != v { + // m would upgrade an existing dependency — it is not a strict downgrade, + // and because it was already present as a dependency, it could affect the + // behavior of other relevant packages. exclude(m) return } @@ -427,6 +439,7 @@ func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([ // is transient (we couldn't download go.mod), return the error from // Downgrade. Currently, we can't tell what kind of error it is. exclude(m) + return } for _, r := range list { add(r) @@ -438,8 +451,8 @@ func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([ } } - var out []module.Version - out = append(out, target) + downgraded := make([]module.Version, 0, len(list)+1) + downgraded = append(downgraded, target) List: for _, r := range list { add(r) @@ -466,10 +479,14 @@ List: add(p) r = p } - out = append(out, r) + downgraded = append(downgraded, r) } - return out, nil + return BuildList(target, &override{ + target: target, + list: downgraded, + Reqs: reqs, + }) } type override struct { diff --git a/src/cmd/go/internal/mvs/mvs_test.go b/src/cmd/go/internal/mvs/mvs_test.go index b8ff3bd8c29..742e396e0dc 100644 --- a/src/cmd/go/internal/mvs/mvs_test.go +++ b/src/cmd/go/internal/mvs/mvs_test.go @@ -32,8 +32,7 @@ build A: A B1 C2 D4 E2 F1 upgrade* A: A B1 C4 D5 E2 F1 G1 upgrade A C4: A B1 C4 D4 E2 F1 G1 build A2: A2 B1 C4 D4 E2 F1 G1 -# BUG: selected versions E2 and F1 are not preserved. -downgrade A2 D2: A2 C4 D2 +downgrade A2 D2: A2 C4 D2 E2 F1 G1 name: trim A: B1 C2 @@ -216,8 +215,7 @@ A: B2 B1: C1 B2: C2 build A: A B2 C2 -# BUG: build list from downgrade omits selected version C1. -downgrade A C1: A B1 +downgrade A C1: A B1 C1 name: down2 A: B2 E2 @@ -231,9 +229,7 @@ E2: D2 E1: F1: build A: A B2 C2 D2 E2 F2 -# BUG: selected versions C1 and D1 are not preserved, and -# requested version F1 is not selected. -downgrade A F1: A B1 E1 +downgrade A F1: A B1 C1 D1 E1 F1 # https://research.swtch.com/vgo-mvs#algorithm_4: # “[D]owngrades are constrained to only downgrade packages, not also upgrade @@ -252,8 +248,7 @@ C2: D1: D2: build A: A B2 C1 D2 -# BUG: requested version D1 is not selected. -downgrade A D1: A +downgrade A D1: A D1 # https://research.swtch.com/vgo-mvs#algorithm_4: # “Unlike upgrades, downgrades must work by removing requirements, not adding @@ -270,10 +265,8 @@ B2: D2 C1: D1: D2: -build A: A B2 D2 -# BUG: requested version D1 is not selected, -# and selected version C1 is omitted from the returned build list. -downgrade A D1: A B1 +build A: A B2 D2 +downgrade A D1: A B1 C1 D1 name: downcycle A: A B2 diff --git a/src/cmd/go/testdata/script/mod_load_badchain.txt b/src/cmd/go/testdata/script/mod_load_badchain.txt index c0c382bfa63..32d9fb24d11 100644 --- a/src/cmd/go/testdata/script/mod_load_badchain.txt +++ b/src/cmd/go/testdata/script/mod_load_badchain.txt @@ -74,8 +74,7 @@ go get: example.com/badchain/c@v1.0.0 updating to module declares its path as: badchain.example.com/c but was required as: example.com/badchain/c -- update-a-expected -- -go get: example.com/badchain/a@v1.0.0 updating to - example.com/badchain/a@v1.1.0 requires +go get: example.com/badchain/a@v1.1.0 requires example.com/badchain/b@v1.1.0 requires example.com/badchain/c@v1.1.0: parsing go.mod: module declares its path as: badchain.example.com/c