]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: in 'go get', only load retractions for resolved versions
authorJay Conrod <jayconrod@google.com>
Tue, 10 Nov 2020 20:48:37 +0000 (15:48 -0500)
committerJay Conrod <jayconrod@google.com>
Wed, 18 Nov 2020 15:31:11 +0000 (15:31 +0000)
Previously, 'go get' loaded retractions for every module in the build
list, which took a long time and usually wasn't helpful.

This rolls forward CL 269019, which was reverted in CL 270521. The new
revision adds a call to modload.ListModules at the end of 'go get' to
ensure .info files are cached for everything in the build list.

Fixes #42185

Change-Id: I684f66c5e674384d5a0176fbc8317e5530b8a915
Reviewed-on: https://go-review.googlesource.com/c/go/+/270858
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
src/cmd/go/internal/modget/get.go
src/cmd/go/internal/modload/build.go
src/cmd/go/internal/modload/modfile.go
src/cmd/go/testdata/mod/example.com_retract_ambiguous_nested_v1.9.0-bad.txt [new file with mode: 0644]
src/cmd/go/testdata/mod/example.com_retract_ambiguous_other_v1.0.0.txt [new file with mode: 0644]
src/cmd/go/testdata/mod/example.com_retract_ambiguous_v1.0.0.txt [new file with mode: 0644]
src/cmd/go/testdata/script/mod_get_retract.txt
src/cmd/go/testdata/script/mod_get_retract_ambiguous.txt [new file with mode: 0644]
src/cmd/go/testdata/script/mod_retract_rationale.txt
src/cmd/go/testdata/script/mod_retract_rename.txt

index 0b7f6bf1d587dcc071b513389a02d8f107011f7a..13106de2f247266eca5ac65465d00081552695c9 100644 (file)
@@ -419,20 +419,7 @@ func runGet(ctx context.Context, cmd *base.Command, args []string) {
                        pkgPatterns = append(pkgPatterns, q.pattern)
                }
        }
-       if len(pkgPatterns) > 0 {
-               // We skipped over missing-package errors earlier: we want to resolve
-               // pathSets ourselves, but at that point we don't have enough context
-               // to log the package-import chains leading to the error. Reload the package
-               // import graph one last time to report any remaining unresolved
-               // dependencies.
-               pkgOpts := modload.PackageOpts{
-                       LoadTests:             *getT,
-                       ResolveMissingImports: false,
-                       AllowErrors:           false,
-               }
-               modload.LoadPackages(ctx, pkgOpts, pkgPatterns...)
-               base.ExitIfErrors()
-       }
+       r.checkPackagesAndRetractions(ctx, pkgPatterns)
 
        // We've already downloaded modules (and identified direct and indirect
        // dependencies) by loading packages in findAndUpgradeImports.
@@ -480,14 +467,20 @@ func runGet(ctx context.Context, cmd *base.Command, args []string) {
        modload.WriteGoMod()
        modload.DisallowWriteGoMod()
 
-       // Report warnings if any retracted versions are in the build list.
-       // This must be done after writing go.mod to avoid spurious '// indirect'
-       // comments. These functions read and write global state.
+       // Ensure .info files are cached for each module in the build list.
+       // This ensures 'go list -m all' can succeed later if offline.
+       // 'go get' only loads .info files for queried versions. 'go list -m' needs
+       // them to add timestamps to the output.
        //
-       // TODO(golang.org/issue/40775): ListModules (called from reportRetractions)
-       // resets modload.loader, which contains information about direct dependencies
-       // that WriteGoMod uses. Refactor to avoid these kinds of global side effects.
-       reportRetractions(ctx)
+       // This is best effort since build commands don't need .info files to load
+       // the build list.
+       //
+       // TODO(golang.org/issue/40775): ListModules resets modload.loader, which
+       // contains information about direct dependencies that WriteGoMod uses.
+       // Refactor to avoid these kinds of global side effects.
+       if modload.HasModRoot() {
+               modload.ListModules(ctx, []string{"all"}, false, false, false)
+       }
 }
 
 // parseArgs parses command-line arguments and reports errors.
@@ -531,43 +524,6 @@ func parseArgs(ctx context.Context, rawArgs []string) []*query {
        return queries
 }
 
-// reportRetractions prints warnings if any modules in the build list are
-// retracted.
-func reportRetractions(ctx context.Context) {
-       // Query for retractions of modules in the build list.
-       // Use modload.ListModules, since that provides information in the same format
-       // as 'go list -m'. Don't query for "all", since that's not allowed outside a
-       // module.
-       buildList := modload.LoadedModules()
-       args := make([]string, 0, len(buildList))
-       for _, m := range buildList {
-               if m.Version == "" {
-                       // main module or dummy target module
-                       continue
-               }
-               args = append(args, m.Path+"@"+m.Version)
-       }
-       listU := false
-       listVersions := false
-       listRetractions := true
-       mods := modload.ListModules(ctx, args, listU, listVersions, listRetractions)
-       retractPath := ""
-       for _, mod := range mods {
-               if len(mod.Retracted) > 0 {
-                       if retractPath == "" {
-                               retractPath = mod.Path
-                       } else {
-                               retractPath = "<module>"
-                       }
-                       rationale := modload.ShortRetractionRationale(mod.Retracted[0])
-                       fmt.Fprintf(os.Stderr, "go: warning: %s@%s is retracted: %s\n", mod.Path, mod.Version, rationale)
-               }
-       }
-       if modload.HasModRoot() && retractPath != "" {
-               fmt.Fprintf(os.Stderr, "go: run 'go get %s@latest' to switch to the latest unretracted version\n", retractPath)
-       }
-}
-
 type resolver struct {
        localQueries      []*query // queries for absolute or relative paths
        pathQueries       []*query // package path literal queries in original order
@@ -594,9 +550,6 @@ type resolver struct {
 
        work *par.Queue
 
-       queryModuleCache   par.Cache
-       queryPackagesCache par.Cache
-       queryPatternCache  par.Cache
        matchInModuleCache par.Cache
 }
 
@@ -681,7 +634,7 @@ func (r *resolver) noneForPath(mPath string) (nq *query, found bool) {
        return nil, false
 }
 
-// queryModule wraps modload.Query, substituting r.checkAllowedor to decide
+// queryModule wraps modload.Query, substituting r.checkAllowedOr to decide
 // allowed versions.
 func (r *resolver) queryModule(ctx context.Context, mPath, query string, selected func(string) string) (module.Version, error) {
        current := r.initialSelected(mPath)
@@ -1217,7 +1170,7 @@ func (r *resolver) findAndUpgradeImports(ctx context.Context, queries []*query)
                }
 
                mu.Lock()
-               upgrades = append(upgrades, pathSet{pkgMods: pkgMods, err: err})
+               upgrades = append(upgrades, pathSet{path: path, pkgMods: pkgMods, err: err})
                mu.Unlock()
                return false
        }
@@ -1544,6 +1497,87 @@ func (r *resolver) chooseArbitrarily(cs pathSet) (isPackage bool, m module.Versi
        return false, cs.mod
 }
 
+// checkPackagesAndRetractions reloads packages for the given patterns and
+// reports missing and ambiguous package errors. It also reports loads and
+// reports retractions for resolved modules and modules needed to build
+// named packages.
+//
+// We skip missing-package errors earlier in the process, since we want to
+// resolve pathSets ourselves, but at that point, we don't have enough context
+// to log the package-import chains leading to each error.
+func (r *resolver) checkPackagesAndRetractions(ctx context.Context, pkgPatterns []string) {
+       defer base.ExitIfErrors()
+
+       // Build a list of modules to load retractions for. Start with versions
+       // selected based on command line queries.
+       //
+       // This is a subset of the build list. If the main module has a lot of
+       // dependencies, loading retractions for the entire build list would be slow.
+       relevantMods := make(map[module.Version]struct{})
+       for path, reason := range r.resolvedVersion {
+               relevantMods[module.Version{Path: path, Version: reason.version}] = struct{}{}
+       }
+
+       // Reload packages, reporting errors for missing and ambiguous imports.
+       if len(pkgPatterns) > 0 {
+               // LoadPackages will print errors (since it has more context) but will not
+               // exit, since we need to load retractions later.
+               pkgOpts := modload.PackageOpts{
+                       LoadTests:             *getT,
+                       ResolveMissingImports: false,
+                       AllowErrors:           true,
+               }
+               matches, pkgs := modload.LoadPackages(ctx, pkgOpts, pkgPatterns...)
+               for _, m := range matches {
+                       if len(m.Errs) > 0 {
+                               base.SetExitStatus(1)
+                               break
+                       }
+               }
+               for _, pkg := range pkgs {
+                       if _, _, err := modload.Lookup("", false, pkg); err != nil {
+                               base.SetExitStatus(1)
+                               if ambiguousErr := (*modload.AmbiguousImportError)(nil); errors.As(err, &ambiguousErr) {
+                                       for _, m := range ambiguousErr.Modules {
+                                               relevantMods[m] = struct{}{}
+                                       }
+                               }
+                       }
+                       if m := modload.PackageModule(pkg); m.Path != "" {
+                               relevantMods[m] = struct{}{}
+                       }
+               }
+       }
+
+       // Load and report retractions.
+       type retraction struct {
+               m   module.Version
+               err error
+       }
+       retractions := make([]retraction, 0, len(relevantMods))
+       for m := range relevantMods {
+               retractions = append(retractions, retraction{m: m})
+       }
+       sort.Slice(retractions, func(i, j int) bool {
+               return retractions[i].m.Path < retractions[j].m.Path
+       })
+       for i := 0; i < len(retractions); i++ {
+               i := i
+               r.work.Add(func() {
+                       err := modload.CheckRetractions(ctx, retractions[i].m)
+                       if retractErr := (*modload.ModuleRetractedError)(nil); errors.As(err, &retractErr) {
+                               retractions[i].err = err
+                       }
+               })
+       }
+       <-r.work.Idle()
+       for _, r := range retractions {
+               if r.err != nil {
+                       fmt.Fprintf(os.Stderr, "go: warning: %v\n", r.err)
+               }
+       }
+}
+
 // reportChanges logs resolved version changes to os.Stderr.
 func (r *resolver) reportChanges(queries []*query) {
        for _, q := range queries {
index b9abb0b93c10829fb9c9a42045b189d1b5b055ca..b9e344045d2ba104476c3bb9a24b93a4a81ff0c7 100644 (file)
@@ -123,13 +123,13 @@ func addRetraction(ctx context.Context, m *modinfo.ModulePublic) {
                return
        }
 
-       err := checkRetractions(ctx, module.Version{Path: m.Path, Version: m.Version})
-       var rerr *retractedError
+       err := CheckRetractions(ctx, module.Version{Path: m.Path, Version: m.Version})
+       var rerr *ModuleRetractedError
        if errors.As(err, &rerr) {
-               if len(rerr.rationale) == 0 {
+               if len(rerr.Rationale) == 0 {
                        m.Retracted = []string{"retracted by module author"}
                } else {
-                       m.Retracted = rerr.rationale
+                       m.Retracted = rerr.Rationale
                }
        } else if err != nil && m.Error == nil {
                m.Error = &modinfo.ModuleError{Err: err.Error()}
index 7a8963246bed2df728c78d72f5eb3d2f0246b602..e9601c3e7c24477e5a1ac892a3e95695602daadf 100644 (file)
@@ -59,7 +59,7 @@ func CheckAllowed(ctx context.Context, m module.Version) error {
        if err := CheckExclusions(ctx, m); err != nil {
                return err
        }
-       if err := checkRetractions(ctx, m); err != nil {
+       if err := CheckRetractions(ctx, m); err != nil {
                return err
        }
        return nil
@@ -85,9 +85,9 @@ type excludedError struct{}
 func (e *excludedError) Error() string     { return "excluded by go.mod" }
 func (e *excludedError) Is(err error) bool { return err == ErrDisallowed }
 
-// checkRetractions returns an error if module m has been retracted by
+// CheckRetractions returns an error if module m has been retracted by
 // its author.
-func checkRetractions(ctx context.Context, m module.Version) error {
+func CheckRetractions(ctx context.Context, m module.Version) error {
        if m.Version == "" {
                // Main module, standard library, or file replacement module.
                // Cannot be retracted.
@@ -165,28 +165,28 @@ func checkRetractions(ctx context.Context, m module.Version) error {
                }
        }
        if isRetracted {
-               return module.VersionError(m, &retractedError{rationale: rationale})
+               return module.VersionError(m, &ModuleRetractedError{Rationale: rationale})
        }
        return nil
 }
 
 var retractCache par.Cache
 
-type retractedError struct {
-       rationale []string
+type ModuleRetractedError struct {
+       Rationale []string
 }
 
-func (e *retractedError) Error() string {
+func (e *ModuleRetractedError) Error() string {
        msg := "retracted by module author"
-       if len(e.rationale) > 0 {
+       if len(e.Rationale) > 0 {
                // This is meant to be a short error printed on a terminal, so just
                // print the first rationale.
-               msg += ": " + ShortRetractionRationale(e.rationale[0])
+               msg += ": " + ShortRetractionRationale(e.Rationale[0])
        }
        return msg
 }
 
-func (e *retractedError) Is(err error) bool {
+func (e *ModuleRetractedError) Is(err error) bool {
        return err == ErrDisallowed
 }
 
diff --git a/src/cmd/go/testdata/mod/example.com_retract_ambiguous_nested_v1.9.0-bad.txt b/src/cmd/go/testdata/mod/example.com_retract_ambiguous_nested_v1.9.0-bad.txt
new file mode 100644 (file)
index 0000000..f8e623d
--- /dev/null
@@ -0,0 +1,10 @@
+-- .mod --
+module example.com/retract/ambiguous/nested
+
+go 1.16
+
+retract v1.9.0-bad // nested modules are bad
+-- .info --
+{"Version":"v1.9.0-bad"}
+-- nested.go --
+package nested
diff --git a/src/cmd/go/testdata/mod/example.com_retract_ambiguous_other_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_retract_ambiguous_other_v1.0.0.txt
new file mode 100644 (file)
index 0000000..5ee0139
--- /dev/null
@@ -0,0 +1,12 @@
+-- .mod --
+module example.com/retract/ambiguous/other
+
+go 1.16
+
+require example.com/retract/ambiguous v1.0.0
+-- .info --
+{"Version":"v1.0.0"}
+-- other.go --
+package other
+
+import _ "example.com/retract/ambiguous/nested"
diff --git a/src/cmd/go/testdata/mod/example.com_retract_ambiguous_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_retract_ambiguous_v1.0.0.txt
new file mode 100644 (file)
index 0000000..c8eeb16
--- /dev/null
@@ -0,0 +1,9 @@
+-- .mod --
+module example.com/retract/ambiguous
+
+go 1.16
+-- .info --
+{"Version":"v1.0.0"}
+-- nested/nested.go --
+package nested
+
index da6c25523fa7da003459939d56d5027db53304b9..13a47bc359d9f0b1a2d599f20046b9065166dc42 100644 (file)
@@ -10,7 +10,7 @@ stdout '^example.com/retract/self/prev v1.1.0$'
 cp go.mod.orig go.mod
 go mod edit -require example.com/retract/self/prev@v1.9.0
 go get -d example.com/retract/self/prev
-stderr '^go: warning: example.com/retract/self/prev@v1.9.0 is retracted: self$'
+stderr '^go: warning: example.com/retract/self/prev@v1.9.0: retracted by module author: self$'
 go list -m example.com/retract/self/prev
 stdout '^example.com/retract/self/prev v1.9.0$'
 
@@ -25,7 +25,7 @@ stdout '^example.com/retract/self/prev v1.1.0$'
 # version is retracted.
 cp go.mod.orig go.mod
 go get -d example.com/retract@v1.0.0-bad
-stderr '^go: warning: example.com/retract@v1.0.0-bad is retracted: bad$'
+stderr '^go: warning: example.com/retract@v1.0.0-bad: retracted by module author: bad$'
 go list -m example.com/retract
 stdout '^example.com/retract v1.0.0-bad$'
 
@@ -33,17 +33,26 @@ stdout '^example.com/retract v1.0.0-bad$'
 # version is available.
 cp go.mod.orig go.mod
 go mod edit -require example.com/retract/self/prev@v1.9.0
-go get -d -u .
-stderr '^go: warning: example.com/retract/self/prev@v1.9.0 is retracted: self$'
+go get -d -u ./use
+stderr '^go: warning: example.com/retract/self/prev@v1.9.0: retracted by module author: self$'
 go list -m example.com/retract/self/prev
 stdout '^example.com/retract/self/prev v1.9.0$'
 
+# 'go get' should warn if a module needed to build named packages is retracted.
+# 'go get' should not warn about unrelated modules.
+go get -d ./empty
+! stderr retracted
+go get -d ./use
+stderr '^go: warning: example.com/retract/self/prev@v1.9.0: retracted by module author: self$'
+
 -- go.mod.orig --
 module example.com/use
 
 go 1.15
 
--- use.go --
+-- use/use.go --
 package use
 
 import _ "example.com/retract/self/prev"
+-- empty/empty.go --
+package empty
diff --git a/src/cmd/go/testdata/script/mod_get_retract_ambiguous.txt b/src/cmd/go/testdata/script/mod_get_retract_ambiguous.txt
new file mode 100644 (file)
index 0000000..b49ba54
--- /dev/null
@@ -0,0 +1,10 @@
+! go get -d example.com/retract/ambiguous/other
+stderr 'ambiguous import: found package example.com/retract/ambiguous/nested in multiple modules:'
+stderr '^go: warning: example.com/retract/ambiguous/nested@v1.9.0-bad: retracted by module author: nested modules are bad$'
+
+-- go.mod --
+module example.com/use
+
+go 1.16
+
+require example.com/retract/ambiguous/nested v1.9.0-bad
index 584c3a3849628757a062846c6fc0b84bd10a1ef3..4d3a3d67c65b5026a3f7724bef209414e71d55a0 100644 (file)
@@ -1,6 +1,6 @@
 # When there is no rationale, 'go get' should print a hard-coded message.
 go get -d example.com/retract/rationale@v1.0.0-empty
-stderr '^go: warning: example.com/retract/rationale@v1.0.0-empty is retracted: retracted by module author$'
+stderr '^go: warning: example.com/retract/rationale@v1.0.0-empty: retracted by module author$'
 
 # 'go list' should print the same hard-coded message.
 go list -m -retracted -f '{{.Retracted}}' example.com/retract/rationale
@@ -9,7 +9,7 @@ stdout '^\[retracted by module author\]$'
 
 # When there is a multi-line message, 'go get' should print the first line.
 go get -d example.com/retract/rationale@v1.0.0-multiline1
-stderr '^go: warning: example.com/retract/rationale@v1.0.0-multiline1 is retracted: short description$'
+stderr '^go: warning: example.com/retract/rationale@v1.0.0-multiline1: retracted by module author: short description$'
 ! stderr 'detail'
 
 # 'go list' should show the full message.
@@ -19,7 +19,7 @@ cmp stdout multiline
 # 'go get' output should be the same whether the retraction appears at top-level
 # or in a block.
 go get -d example.com/retract/rationale@v1.0.0-multiline2
-stderr '^go: warning: example.com/retract/rationale@v1.0.0-multiline2 is retracted: short description$'
+stderr '^go: warning: example.com/retract/rationale@v1.0.0-multiline2: retracted by module author: short description$'
 ! stderr 'detail'
 
 # Same for 'go list'.
@@ -29,7 +29,7 @@ cmp stdout multiline
 
 # 'go get' should omit long messages.
 go get -d example.com/retract/rationale@v1.0.0-long
-stderr '^go: warning: example.com/retract/rationale@v1.0.0-long is retracted: \(rationale omitted: too long\)'
+stderr '^go: warning: example.com/retract/rationale@v1.0.0-long: retracted by module author: \(rationale omitted: too long\)'
 
 # 'go list' should show the full message.
 go list -m -retracted -f '{{.Retracted}}' example.com/retract/rationale
@@ -38,7 +38,7 @@ stdout '^\[lo{500}ng\]$'
 
 # 'go get' should omit messages with unprintable characters.
 go get -d example.com/retract/rationale@v1.0.0-unprintable
-stderr '^go: warning: example.com/retract/rationale@v1.0.0-unprintable is retracted: \(rationale omitted: contains non-printable characters\)'
+stderr '^go: warning: example.com/retract/rationale@v1.0.0-unprintable: retracted by module author: \(rationale omitted: contains non-printable characters\)'
 
 # 'go list' should show the full message.
 go list -m -retracted -f '{{.Retracted}}' example.com/retract/rationale
@@ -62,9 +62,9 @@ stdout '^single version,degenerate range,$'
 
 # 'go get' will only report the first retraction to avoid being too verbose.
 go get -d example.com/retract/rationale@v1.0.0-order
-stderr '^go: warning: example.com/retract/rationale@v1.0.0-order is retracted: degenerate range$'
+stderr '^go: warning: example.com/retract/rationale@v1.0.0-order: retracted by module author: degenerate range$'
 go get -d example.com/retract/rationale@v1.0.1-order
-stderr '^go: warning: example.com/retract/rationale@v1.0.1-order is retracted: single version$'
+stderr '^go: warning: example.com/retract/rationale@v1.0.1-order: retracted by module author: single version$'
 
 -- go.mod --
 module m
index b75bfe996302d5be71beb47f21074a42845881b8..f54742c5232253b6d4f44927fab010f5770d24a7 100644 (file)
@@ -10,7 +10,7 @@ go list -m -u -f '{{with .Retracted}}retracted{{end}}' example.com/retract/renam
 
 # 'go get' should warn about the retracted version.
 go get -d
-stderr '^go: warning: example.com/retract/rename@v1.0.0-bad is retracted: bad$'
+stderr '^go: warning: example.com/retract/rename@v1.0.0-bad: retracted by module author: bad$'
 
 # We can't upgrade, since this latest version has a different module path.
 ! go get -d example.com/retract/rename