From 0b23c88d9f908aa09c7e35ec339b3678f9dcf5fc Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 18 Jul 2018 20:08:04 -0400 Subject: [PATCH] cmd/go: fix module get -u to avoid spurious new deps If we have go get -u x1@v1 x2@v2 and x1 depends on x2, use v2 as the "upgraded" x2 chosen by -u instead of letting -u pick something (say, v2.1) and then immediately overriding it. This avoids chasing down the deps from v2.1 and also avoids them polluting the overall module graph. This fix also lets us delete some code in the preparation step, reducing the overall latency of get -u. Suggested by Bryan Mills in https://go-review.googlesource.com/c/vgo/+/122396/6#371. Fixes #26342. Change-Id: I50fa842304820d3f16f66a8e65dea695e2b0f88b Reviewed-on: https://go-review.googlesource.com/124856 Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/modget/get.go | 69 ++++++++----------- .../go/testdata/script/mod_get_downgrade.txt | 10 +++ 2 files changed, 39 insertions(+), 40 deletions(-) diff --git a/src/cmd/go/internal/modget/get.go b/src/cmd/go/internal/modget/get.go index 361d4808f3..7cbd1f9406 100644 --- a/src/cmd/go/internal/modget/get.go +++ b/src/cmd/go/internal/modget/get.go @@ -177,10 +177,15 @@ func init() { CmdGet.Flag.Var(&getU, "u", "") } -type Pkg struct { - Arg string - Path string - Vers string +// A task holds the state for processing a single get argument (path@vers). +type task struct { + arg string // original argument + index int + path string // package path part of arg + forceModulePath bool // path must be interpreted as a module path + vers string // version part of arg + m module.Version // module version indicated by argument + req []module.Version // m's requirement list (not upgraded) } func runGet(cmd *base.Command, args []string) { @@ -211,17 +216,6 @@ func runGet(cmd *base.Command, args []string) { // what was requested. modload.DisallowWriteGoMod() - // A task holds the state for processing a single get argument (path@vers). - type task struct { - arg string // original argument - index int - path string // package path part of arg - forceModulePath bool // path must be interpreted as a module path - vers string // version part of arg - m module.Version // module version indicated by argument - req []module.Version // m's requirement list (not upgraded) - } - // Build task and install lists. // The command-line arguments are of the form path@version // or simply path, with implicit @latest. path@none is "downgrade away". @@ -291,10 +285,6 @@ func runGet(cmd *base.Command, args []string) { continue } if path == "all" { - if path != arg { - base.Errorf("go get %s: cannot use pattern %q with explicit version", arg, arg) - } - // TODO: If *getM, should this be the module pattern "all"? // This is the package pattern "all" not the module pattern "all": @@ -354,7 +344,7 @@ func runGet(cmd *base.Command, args []string) { base.ExitIfErrors() // Now we've reduced the upgrade/downgrade work to a list of path@vers pairs (tasks). - // Resolve each one and load direct requirements in parallel. + // Resolve each one in parallell. reqs := modload.Reqs() var lookup par.Work for _, t := range tasks { @@ -373,36 +363,21 @@ func runGet(cmd *base.Command, args []string) { return } t.m = m - // If there is no -u, then we don't need to upgrade the - // collected requirements separately from the overall - // recalculation of the build list (modload.ReloadBuildList below), - // so don't bother doing it now. Doing it now wouldn't be - // any slower (because it would prime the cache for later) - // but the larger operation below can report more errors in a single run. - if getU != "" { - list, err := reqs.Required(m) - if err != nil { - base.Errorf("go get %v: %v", t.arg, err) - return - } - t.req = list - } }) base.ExitIfErrors() - // Now we know the specific version of each path@vers along with its requirements. + // Now we know the specific version of each path@vers. // The final build list will be the union of three build lists: // 1. the original build list // 2. the modules named on the command line // 3. the upgraded requirements of those modules (if upgrading) // Start building those lists. - // This loop collects (2) and the not-yet-upgraded (3). + // This loop collects (2). // Also, because the list of paths might have named multiple packages in a single module // (or even the same package multiple times), now that we know the module for each // package, this loop deduplicates multiple references to a given module. // (If a module is mentioned multiple times, the listed target version must be the same each time.) var named []module.Version - var required []module.Version byPath := make(map[string]*task) for _, t := range tasks { prev, ok := byPath[t.m.Path] @@ -416,7 +391,6 @@ func runGet(cmd *base.Command, args []string) { } byPath[t.m.Path] = t named = append(named, t.m) - required = append(required, t.req...) } base.ExitIfErrors() @@ -425,11 +399,13 @@ func runGet(cmd *base.Command, args []string) { // chase down the full list of upgraded dependencies. // This turns required from a not-yet-upgraded (3) to the final (3). // (See list above.) - if len(required) > 0 { + var required []module.Version + if getU != "" { upgraded, err := mvs.UpgradeAll(upgradeTarget, &upgrader{ Reqs: modload.Reqs(), - targets: required, + targets: named, patch: getU == "patch", + tasks: byPath, }) if err != nil { base.Fatalf("go get: %v", err) @@ -609,6 +585,7 @@ type upgrader struct { mvs.Reqs targets []module.Version patch bool + tasks map[string]*task } // upgradeTarget is a fake "target" requiring all the modules to be upgraded. @@ -631,6 +608,17 @@ func (u *upgrader) Required(m module.Version) ([]module.Version, error) { // This special case prevents accidental downgrades // when already using a pseudo-version newer than the latest tagged version. func (u *upgrader) Upgrade(m module.Version) (module.Version, error) { + // Allow pkg@vers on the command line to override the upgrade choice v. + // If t's version is < v, then we're going to downgrade anyway, + // and it's cleaner to avoid moving back and forth and picking up + // extraneous other newer dependencies. + // If t's version is > v, then we're going to upgrade past v anyway, + // and again it's cleaner to avoid moving back and forth picking up + // extraneous other newer dependencies. + if t := u.tasks[m.Path]; t != nil { + return t.m, nil + } + // Note that query "latest" is not the same as // using repo.Latest. // The query only falls back to untagged versions @@ -667,5 +655,6 @@ func (u *upgrader) Upgrade(m module.Version) (module.Version, error) { if mTime, err := modfetch.PseudoVersionTime(m.Version); err == nil && info.Time.Before(mTime) { return m, nil } + return module.Version{Path: m.Path, Version: info.Version}, nil } diff --git a/src/cmd/go/testdata/script/mod_get_downgrade.txt b/src/cmd/go/testdata/script/mod_get_downgrade.txt index 5a00d6d51d..ac814dae08 100644 --- a/src/cmd/go/testdata/script/mod_get_downgrade.txt +++ b/src/cmd/go/testdata/script/mod_get_downgrade.txt @@ -22,8 +22,18 @@ go list -m all stdout 'rsc.io/quote v1.5.1' stdout 'rsc.io/sampler v1.3.0' +# go get -u args should limit upgrades +cp go.mod.empty go.mod +go get -u rsc.io/quote@v1.4.0 rsc.io/sampler@v1.0.0 +go list -m all +stdout 'rsc.io/quote v1.4.0' +stdout 'rsc.io/sampler v1.0.0' +! stdout golang.org/x/text + -- go.mod -- module x require rsc.io/quote v1.5.1 +-- go.mod.empty -- +module x -- x.go -- package x -- 2.50.0