From 07ef3135253321176704bce6e629a07ac02bf1c6 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 17 Feb 2021 12:03:13 -0800 Subject: [PATCH 1/8] runtime/cgo: add cast in C code to avoid C compiler warning Fixes #44340 Change-Id: Id80dd1f44a988b653933732afcc8e49a826affc4 Reviewed-on: https://go-review.googlesource.com/c/go/+/293209 Reviewed-by: Andrew G. Morgan Reviewed-by: Bryan C. Mills Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor --- src/runtime/cgo/linux_syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/cgo/linux_syscall.c b/src/runtime/cgo/linux_syscall.c index 56f3d67d8bf..59761c8b409 100644 --- a/src/runtime/cgo/linux_syscall.c +++ b/src/runtime/cgo/linux_syscall.c @@ -32,7 +32,7 @@ typedef struct { #define SET_RETVAL(fn) \ uintptr_t ret = (uintptr_t) fn ; \ - if (ret == -1) { \ + if (ret == (uintptr_t) -1) { \ x->retval = (uintptr_t) errno; \ } else \ x->retval = ret From f0be3cc5476bd726707985a6382ae6a2d6fa8968 Mon Sep 17 00:00:00 2001 From: Joel Sing Date: Wed, 17 Feb 2021 20:34:34 +1100 Subject: [PATCH 2/8] runtime: unbreak linux/riscv64 following regabi merge Unbreak the linux/riscv64 port by storing the zero value register to memory, rather than the current code that is moving a zero intermediate to the stack pointer register (ideally this should be caught by the assembler). This was broken in CL#272568. On riscv64 a zero immediate value cannot be moved directly to memory, rather a register needs to be loaded with zero and then stored. Alternatively, the the zero value register (aka X0) can be used directly. Change-Id: Id57121541d50c9993cec5c2270b638b184ab9bc1 Reviewed-on: https://go-review.googlesource.com/c/go/+/292894 Trust: Joel Sing Reviewed-by: Michael Knyszek Reviewed-by: Cherry Zhang Run-TryBot: Michael Knyszek TryBot-Result: Go Bot --- src/runtime/asm_riscv64.s | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/asm_riscv64.s b/src/runtime/asm_riscv64.s index 31e324d6775..3d0349471a2 100644 --- a/src/runtime/asm_riscv64.s +++ b/src/runtime/asm_riscv64.s @@ -449,7 +449,7 @@ TEXT callRet<>(SB), NOSPLIT, $40-0 MOV A1, 16(X2) MOV A3, 24(X2) MOV A2, 32(X2) - MOV $0, 40(X2) + MOV ZERO, 40(X2) CALL runtime·reflectcallmove(SB) RET From 609d82b28992e6a7fac48add680f170c4eee83fd Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Fri, 12 Feb 2021 21:34:11 +0100 Subject: [PATCH 3/8] cmd/dist: set GOARM=7 for windows/arm GOARM=6 executables fail to launch on windows/arm, so set this to ARMv7 like we do for Android. This CL is part of a stack adding windows/arm64 support (#36439), intended to land in the Go 1.17 cycle. Change-Id: Ifa13685e7ab6edd367f3dfec10296e376319dbd4 Reviewed-on: https://go-review.googlesource.com/c/go/+/291629 Reviewed-by: Russ Cox Trust: Jason A. Donenfeld Run-TryBot: Jason A. Donenfeld TryBot-Result: Go Bot --- src/cmd/dist/util.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cmd/dist/util.go b/src/cmd/dist/util.go index 0a419e465fe..e99375f5380 100644 --- a/src/cmd/dist/util.go +++ b/src/cmd/dist/util.go @@ -389,6 +389,10 @@ func xgetgoarm() string { // sense to auto-detect the setting. return "7" } + if goos == "windows" { + // windows/arm only works with ARMv7 executables. + return "7" + } if gohostarch != "arm" || goos != gohostos { // Conservative default for cross-compilation. return "5" From a76efea1fe18045ecb8d1cf2c1104208e636fbae Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 28 Jan 2021 10:18:24 -0500 Subject: [PATCH 4/8] cmd/go/internal/mvs: don't emit duplicates from Req MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Req is supposed to return “a minimal requirement list” that includes each of the module paths listed in base. Currently, if base contains duplicates Req emits duplicates, and a list containing duplicates is certainly not minimal. That, in turn, requires callers to be careful to deduplicate the base slice, and there are multiple callers that are already quite complicated to reason about even without the added complication of deduplicating slices. For #36460 Change-Id: I391a1dc0641fe1dd424c16b7a1082da0d00c7292 Reviewed-on: https://go-review.googlesource.com/c/go/+/287632 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob Reviewed-by: Jay Conrod --- src/cmd/go/internal/mvs/mvs.go | 5 +++++ src/cmd/go/internal/mvs/mvs_test.go | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/cmd/go/internal/mvs/mvs.go b/src/cmd/go/internal/mvs/mvs.go index b630b610f10..f016d8ff15d 100644 --- a/src/cmd/go/internal/mvs/mvs.go +++ b/src/cmd/go/internal/mvs/mvs.go @@ -293,10 +293,15 @@ func Req(target module.Version, base []string, reqs Reqs) ([]module.Version, err } // First walk the base modules that must be listed. var min []module.Version + haveBase := map[string]bool{} for _, path := range base { + if haveBase[path] { + continue + } m := module.Version{Path: path, Version: max[path]} min = append(min, m) walk(m) + haveBase[path] = true } // Now the reverse postorder to bring in anything else. for i := len(postorder) - 1; i >= 0; i-- { diff --git a/src/cmd/go/internal/mvs/mvs_test.go b/src/cmd/go/internal/mvs/mvs_test.go index 721cd9635c8..995a38fa92a 100644 --- a/src/cmd/go/internal/mvs/mvs_test.go +++ b/src/cmd/go/internal/mvs/mvs_test.go @@ -327,6 +327,19 @@ B1: Cnone D1 E1: Fnone build M: M B1 D1 E1 req M: B1 E1 + +name: reqdup +M: A1 B1 +A1: B1 +B1: +req M A A: A1 + +name: reqcross +M: A1 B1 C1 +A1: B1 C1 +B1: C1 +C1: +req M A B: A1 B1 ` func Test(t *testing.T) { From a5c8a15f649ffe29b3a80144e6d422046adb2cf0 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 28 Jan 2021 17:01:54 -0500 Subject: [PATCH 5/8] cmd/go/internal/mvs: clarify and annotate test cases For #36460 Change-Id: I5a8be8f36fb8825ffa08ed1427cb1e15b106b31a Reviewed-on: https://go-review.googlesource.com/c/go/+/287732 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Jay Conrod Reviewed-by: Michael Matloob --- src/cmd/go/internal/mvs/mvs_test.go | 94 ++++++++++++++++++++++------- 1 file changed, 72 insertions(+), 22 deletions(-) diff --git a/src/cmd/go/internal/mvs/mvs_test.go b/src/cmd/go/internal/mvs/mvs_test.go index 995a38fa92a..b8ff3bd8c29 100644 --- a/src/cmd/go/internal/mvs/mvs_test.go +++ b/src/cmd/go/internal/mvs/mvs_test.go @@ -28,9 +28,11 @@ D4: E2 F1 D5: E2 G1: C4 A2: B1 C4 D4 -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 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 name: trim @@ -68,7 +70,7 @@ B2: D1 C: D2 D1: E2 D2: E1 -build A: A B1 C D2 E1 +build A: A B1 C D2 E1 upgrade A B2: A B2 C D2 E2 name: cross1R @@ -136,17 +138,17 @@ name: cross5 A: D1 D1: E2 D2: E1 -build A: A D1 E2 -upgrade* A: A D2 E2 -upgrade A D2: A D2 E2 +build A: A D1 E2 +upgrade* A: A D2 E2 +upgrade A D2: A D2 E2 upgradereq A D2: D2 E2 name: cross6 A: D2 D1: E2 D2: E1 -build A: A D2 E1 -upgrade* A: A D2 E2 +build A: A D2 E1 +upgrade* A: A D2 E2 upgrade A E2: A D2 E2 name: cross7 @@ -175,7 +177,7 @@ B1: D1 B2: C2: D2: -build A: A B1 C1 D1 +build A: A B1 C1 D1 upgrade* A: A B2 C2 D2 name: simplify @@ -194,7 +196,7 @@ B4: B5.hidden: C2: C3: -build A: A B1 C1 +build A: A B1 C1 upgrade* A: A B4 C3 name: up2 @@ -206,14 +208,15 @@ B4: B5.hidden: C2: C3: -build A: A B5.hidden C1 +build A: A B5.hidden C1 upgrade* A: A B5.hidden C3 name: down1 A: B2 B1: C1 B2: C2 -build A: A B2 C2 +build A: A B2 C2 +# BUG: build list from downgrade omits selected version C1. downgrade A C1: A B1 name: down2 @@ -227,12 +230,56 @@ D2: B2 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 +# https://research.swtch.com/vgo-mvs#algorithm_4: +# “[D]owngrades are constrained to only downgrade packages, not also upgrade +# them; if an upgrade before downgrade is needed, the user must ask for it +# explicitly.” +# +# Here, downgrading B2 to B1 upgrades C1 to C2, and C2 does not depend on D2. +# However, C2 would be an upgrade — not a downgrade — so B1 must also be +# rejected. +name: downcross1 +A: B2 C1 +B1: C2 +B2: C1 +C1: D2 +C2: +D1: +D2: +build A: A B2 C1 D2 +# BUG: requested version D1 is not selected. +downgrade A D1: A + +# https://research.swtch.com/vgo-mvs#algorithm_4: +# “Unlike upgrades, downgrades must work by removing requirements, not adding +# them.” +# +# However, downgrading a requirement may introduce a new requirement on a +# previously-unrequired module. If each dependency's requirements are complete +# (“tidy”), that can't change the behavior of any other package whose version is +# not also being downgraded, so we should allow it. +name: downcross2 +A: B2 +B1: C1 +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 + name: downcycle A: A B2 B2: A B1: +build A: A B2 downgrade A B1: A B1 # golang.org/issue/25542. @@ -240,6 +287,7 @@ name: noprev1 A: B4 C2 B2.hidden: C2: +build A: A B4 C2 downgrade A B2.hidden: A B2.hidden C2 name: noprev2 @@ -247,6 +295,7 @@ A: B4 C2 B2.hidden: B1: C2: +build A: A B4 C2 downgrade A B2.hidden: A B2.hidden C2 name: noprev3 @@ -254,6 +303,7 @@ A: B4 C2 B3: B2.hidden: C2: +build A: A B4 C2 downgrade A B2.hidden: A B2.hidden C2 # Cycles involving the target. @@ -264,9 +314,9 @@ A: B1 B1: A1 B2: A2 B3: A3 -build A: A B1 +build A: A B1 upgrade A B2: A B2 -upgrade* A: A B3 +upgrade* A: A B3 # golang.org/issue/29773: # Requirements of older versions of the target @@ -280,7 +330,7 @@ B2: A2 C1: A2 C2: D2: -build A: A B1 C1 D1 +build A: A B1 C1 D1 upgrade* A: A B2 C2 D2 # Cycles with multiple possible solutions. @@ -293,23 +343,23 @@ B2: C2 C1: C2: B2 build M: M A1 B2 C2 -req M: A1 B2 -req M A: A1 B2 -req M C: A1 C2 +req M: A1 B2 +req M A: A1 B2 +req M C: A1 C2 # Requirement minimization. name: req1 A: B1 C1 D1 E1 F1 B1: C1 E1 F1 -req A: B1 D1 +req A: B1 D1 req A C: B1 C1 D1 name: req2 A: G1 H1 G1: H1 H1: G1 -req A: G1 +req A: G1 req A G: G1 req A H: H1 @@ -326,7 +376,7 @@ M: Anone B1 D1 E1 B1: Cnone D1 E1: Fnone build M: M B1 D1 E1 -req M: B1 E1 +req M: B1 E1 name: reqdup M: A1 B1 From f3c2208e2cca69911fa72c22211361a6a57f4e26 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 29 Jan 2021 21:02:46 -0500 Subject: [PATCH 6/8] cmd/go: add script tests for potential upgrades due to downgrades For #36460 Change-Id: I1620c23819263ef82e571fc4d4c778277842c02d Reviewed-on: https://go-review.googlesource.com/c/go/+/288535 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob Reviewed-by: Jay Conrod --- .../script/mod_get_downadd_indirect.txt | 81 ++++++++++++++ .../script/mod_get_downup_indirect.txt | 101 ++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 src/cmd/go/testdata/script/mod_get_downadd_indirect.txt create mode 100644 src/cmd/go/testdata/script/mod_get_downup_indirect.txt diff --git a/src/cmd/go/testdata/script/mod_get_downadd_indirect.txt b/src/cmd/go/testdata/script/mod_get_downadd_indirect.txt new file mode 100644 index 00000000000..efc38f77c85 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_get_downadd_indirect.txt @@ -0,0 +1,81 @@ +# This test illustrates a case where downgrading one module may upgrade another. +# Compare to the downcross2 test case in cmd/go/internal/mvs/mvs_test.go. + +# The initial package import graph used in this test looks like: +# +# a ---- b ---- d +# +# The module dependency graph originally looks like: +# +# a ---- b.2 ---- d.2 +# +# b.1 ---- c.1 +# +# If we downgrade module d to version 1, we must downgrade b as well. +# If that downgrade selects b version 1, we will add a new dependency on module c. + +cp go.mod go.mod.orig +go mod tidy +cmp go.mod.orig go.mod + +go get -d example.com/d@v0.1.0 +go list -m all +stdout '^example.com/b v0.1.0 ' +stdout '^example.com/c v0.1.0 ' +stdout '^example.com/d v0.1.0 ' + +-- go.mod -- +module example.com/a + +go 1.15 + +require example.com/b v0.2.0 + +replace ( + example.com/b v0.1.0 => ./b1 + example.com/b v0.2.0 => ./b2 + example.com/c v0.1.0 => ./c + example.com/d v0.1.0 => ./d + example.com/d v0.2.0 => ./d +) +-- a.go -- +package a + +import _ "example.com/b" + +-- b1/go.mod -- +module example.com/b + +go 1.15 + +require example.com/c v0.1.0 +-- b1/b.go -- +package b + +import _ "example.com/c" + +-- b2/go.mod -- +module example.com/b + +go 1.15 + +require example.com/d v0.2.0 +-- b2/b.go -- +package b + +import _ "example.com/d" + +-- c/go.mod -- +module example.com/c + +go 1.15 + +-- c/c.go -- +package c + +-- d/go.mod -- +module example.com/d + +go 1.15 +-- d/d.go -- +package d diff --git a/src/cmd/go/testdata/script/mod_get_downup_indirect.txt b/src/cmd/go/testdata/script/mod_get_downup_indirect.txt new file mode 100644 index 00000000000..ced1dcd6b14 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_get_downup_indirect.txt @@ -0,0 +1,101 @@ +# This test illustrates a case where downgrading one module may upgrade another. +# Compare to the downcross1 test case in cmd/go/internal/mvs/mvs_test.go. + +# The package import graph used in this test looks like: +# +# a ---- b +# \ \ +# \ \ +# ----- c ---- d +# +# The module dependency graph originally looks like: +# +# a ---- b.2 +# \ \ +# \ \ +# ----- c.1 ---- d.2 +# +# b.1 ---- c.2 +# +# If we downgrade module d to version 1, we must downgrade b as well. +# If that downgrade selects b version 1, we will upgrade module c to version 2. +# So 'go get d@1' should instead downgrade both b and c to "none". + +cp go.mod go.mod.orig +go mod tidy +cmp go.mod.orig go.mod + +go get -d example.com/d@v0.1.0 +go list -m all +! stdout '^example.com/b ' +! stdout '^example.com/c ' +stdout '^example.com/d v0.1.0 ' + +-- go.mod -- +module example.com/a + +go 1.15 + +require ( + example.com/b v0.2.0 + example.com/c v0.1.0 +) + +replace ( + example.com/b v0.1.0 => ./b1 + example.com/b v0.2.0 => ./b2 + example.com/c v0.1.0 => ./c1 + example.com/c v0.2.0 => ./c2 + example.com/d v0.1.0 => ./d + example.com/d v0.2.0 => ./d +) +-- a.go -- +package a + +import ( + _ "example.com/b" + _ "example.com/c" +) + +-- b1/go.mod -- +module example.com/b + +go 1.15 + +require example.com/c v0.2.0 +-- b1/b.go -- +package b + +import _ "example.com/c" + +-- b2/go.mod -- +module example.com/b + +go 1.15 + +require example.com/c v0.1.0 +-- b2/b.go -- +package b + +import _ "example.com/c" + +-- c1/go.mod -- +module example.com/c + +go 1.15 + +require example.com/d v0.2.0 +-- c1/c.go -- +package c + +-- c2/go.mod -- +module example.com/c + +go 1.15 +-- c2/c.go -- +package c + +-- d/go.mod -- +module example.com/d + +go 1.15 From 3b7277d3651b5c5856c5b0879ba3fb7a5f279508 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 4 Feb 2021 10:59:17 -0500 Subject: [PATCH 7/8] cmd/go: add a script test for artifacts resulting from 'go get -u' For #36460 Change-Id: I4f8bf0fb8dfa508b346acb3868302452409ee9da Reviewed-on: https://go-review.googlesource.com/c/go/+/289696 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob Reviewed-by: Jay Conrod --- .../script/mod_get_downup_artifact.txt | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 src/cmd/go/testdata/script/mod_get_downup_artifact.txt diff --git a/src/cmd/go/testdata/script/mod_get_downup_artifact.txt b/src/cmd/go/testdata/script/mod_get_downup_artifact.txt new file mode 100644 index 00000000000..b35d4c4fd03 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_get_downup_artifact.txt @@ -0,0 +1,165 @@ +# This test illustrates a case where an upgrade–downgrade–upgrade cycle can +# result in upgrades of otherwise-irrelevant dependencies. +# +# This case has no corresponding test in the mvs package, because it is an +# artifact that results from the composition of *multiple* MVS operations. + +# The initial package import graph used in the test looks like: +# +# m ---- a +# | | +# +----- b +# | | +# +----- c +# | +# +----- d +# +# b version 2 adds its own import of package d. +# +# The module dependency graph initially looks like: +# +# m ---- a.1 +# | | +# +----- b.1 +# | | +# +----- c.1 +# | +# +----- d.1 +# +# b.2 ---- c.2 +# | +# +------ d.2 +# | +# +------ e.1 +# +# If we upgrade module b to version 2, we will upgrade c and d and add a new +# dependency on e. If b version 2 is disallowed because of any of those +# dependencies, the other dependencies should not be upgraded as a side-effect. + +cp go.mod go.mod.orig +go mod tidy +cmp go.mod go.mod.orig + +go list -m all +stdout '^example.com/a v0.1.0 ' +stdout '^example.com/b v0.1.0 ' +stdout '^example.com/c v0.1.0 ' +stdout '^example.com/d v0.1.0 ' +! stdout '^example.com/e ' + +# b is imported by a, so the -u flag would normally upgrade it to v0.2.0. +# However, that would conflict with the explicit c@v0.1.0 constraint, +# so b must remain at v0.1.0. +# +# If we're not careful, we might temporarily add b@v0.2.0 and pull in its +# upgrades of module d and addition of module e, which are not relevant to +# b@v0.1.0 and should not be added to the main module's dependencies. + +go get -u -d example.com/a@latest example.com/c@v0.1.0 + +go list -m all +stdout '^example.com/a v0.1.0 ' +stdout '^example.com/b v0.1.0 ' +stdout '^example.com/c v0.1.0 ' + + # BUG: d should remain at v0.1.0, because it is not transitively imported by a + # with b@v0.1.0. Today, it is spuriously upgraded to v0.2.0. +stdout '^example.com/d v0.2.0 ' + + # BUG: e should not be added, because it is not transitively imported by a + # with b@v0.1.0. Today, it is spuriously added. +stdout '^example.com/e v0.1.0 ' + +-- go.mod -- +module example.com/m + +go 1.16 + +require ( + example.com/a v0.1.0 + example.com/b v0.1.0 + example.com/c v0.1.0 + example.com/d v0.1.0 +) + +replace ( + example.com/a v0.1.0 => ./a1 + example.com/b v0.1.0 => ./b1 + example.com/b v0.2.0 => ./b2 + example.com/c v0.1.0 => ./c + example.com/c v0.2.0 => ./c + example.com/d v0.1.0 => ./d + example.com/d v0.2.0 => ./d + example.com/e v0.1.0 => ./e +) +-- m.go -- +package m + +import ( + _ "example.com/a" + _ "example.com/b" + _ "example.com/c" + _ "example.com/d" +) + +-- a1/go.mod -- +module example.com/a + +go 1.16 + +require example.com/b v0.1.0 +-- a1/a.go -- +package a + +import _ "example.com/b" + +-- b1/go.mod -- +module example.com/b + +go 1.16 + +require example.com/c v0.1.0 +-- b1/b.go -- +package b + +import _ "example.com/c" + +-- b2/go.mod -- +module example.com/b + +go 1.16 + +require ( + example.com/c v0.2.0 + example.com/d v0.2.0 + example.com/e v0.1.0 +) +-- b2/b.go -- +package b + +import ( + "example.com/c" + "example.com/d" + "example.com/e" +) + +-- c/go.mod -- +module example.com/c + +go 1.16 +-- c/c.go -- +package c + +-- d/go.mod -- +module example.com/d + +go 1.16 +-- d/d.go -- +package d + +-- e/go.mod -- +module example.com/e + +go 1.16 +-- e/e.go -- +package e From eb982727e33263c0bb67de607beb44c5e0bd2bea Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 28 Jan 2021 09:10:57 -0500 Subject: [PATCH 8/8] 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