From 71be83e8ca660c375592683bf71de8864a8464c5 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 9 May 2019 15:42:55 -0400 Subject: [PATCH] cmd/go: make 'go get' match patterns against packages, not modules This is a follow-up to CL 174099, fixing an important TODO. The 'go help modget' documentation will be clarified in anotehr CL, pending further discussion. When invoked without -m, 'go get' will no longer match arguments containing "..." against module paths. If a module's path matches a pattern but no packages within the module match the pattern, the module should not be upgraded. For example, if golang.org/x/tools/playground and golang.org/x/tools are separate modules, and only golang.org/x/tools is in the build list, 'go get golang.org/x/tools/playground/...' should add golang.org/x/tools/playground to the build list and leave golang.org/x/tools alone. Updates #26902 Change-Id: I2bd18c7950db1aa7bd8527210c1baf2c7d174375 Reviewed-on: https://go-review.googlesource.com/c/go/+/176578 Run-TryBot: Jay Conrod Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/modget/get.go | 82 ++++++++++--------- .../mod/example.com_nest_sub_v1.0.0.txt | 12 +++ .../testdata/mod/example.com_nest_v1.0.0.txt | 12 +++ .../testdata/mod/example.com_nest_v1.1.0.txt | 12 +++ src/cmd/go/testdata/script/mod_get_main.txt | 5 +- .../go/testdata/script/mod_get_patterns.txt | 8 ++ 6 files changed, 90 insertions(+), 41 deletions(-) create mode 100644 src/cmd/go/testdata/mod/example.com_nest_sub_v1.0.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_nest_v1.0.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_nest_v1.1.0.txt diff --git a/src/cmd/go/internal/modget/get.go b/src/cmd/go/internal/modget/get.go index 62519fd5faf..bf87c4a0d10 100644 --- a/src/cmd/go/internal/modget/get.go +++ b/src/cmd/go/internal/modget/get.go @@ -336,37 +336,32 @@ func runGet(cmd *base.Command, args []string) { } case strings.Contains(path, "..."): - // Find modules in the build list matching the pattern, if any. - match := search.MatchPattern(path) - matched := false - for _, m := range modload.BuildList() { - // TODO: If we have matching packages already in the build list and we - // know which module(s) they are in, then we should not upgrade the - // modules that do *not* contain those packages, even if the module path - // is a prefix of the pattern. - // - // For example, if we have modules golang.org/x/tools and - // golang.org/x/tools/playground, and all of the packages matching - // golang.org/x/tools/playground... are in the - // golang.org/x/tools/playground module, then we should not *also* try - // to upgrade golang.org/x/tools if the user says 'go get - // golang.org/x/tools/playground...@latest'. - if match(m.Path) || str.HasPathPrefix(path, m.Path) { - queries = append(queries, &query{querySpec: querySpec{path: m.Path, vers: vers, prevM: m, forceModulePath: true}, arg: arg}) - matched = true + // If we're using -m, look up modules in the build list that match + // the pattern. Report an error if no modules match. + if *getM { + match := search.MatchPattern(path) + matched := false + for _, m := range modload.BuildList() { + if match(m.Path) || str.HasPathPrefix(path, m.Path) { + queries = append(queries, &query{querySpec: querySpec{path: m.Path, vers: vers, prevM: m, forceModulePath: true}, arg: arg}) + matched = true + } + } + if !matched { + base.Errorf("go get %s: pattern matches no modules in build list", arg) + continue } - } - // If matched, we're done. - // If we're using -m, report an error. - // Otherwise, look up a module containing packages that match the pattern. - if matched { break } - if *getM { - base.Errorf("go get %s: pattern matches no modules in build list", arg) - continue - } - queries = append(queries, &query{querySpec: querySpec{path: path, vers: vers}, arg: arg}) + + // If we're not using -m, wait until we load packages to look up modules. + // We don't know yet whether any modules in the build list provide + // packages matching the pattern. For example, suppose + // golang.org/x/tools and golang.org/x/tools/playground are separate + // modules, and only golang.org/x/tools is in the build list. If the + // user runs 'go get golang.org/x/tools/playground/...', we should + // add a requirement for golang.org/x/tools/playground. We should not + // upgrade golang.org/x/tools. case path == "all": // This is the package pattern "all" not the module pattern "all", @@ -463,8 +458,16 @@ func runGet(cmd *base.Command, args []string) { var matches []*search.Match var install []string for { - var queries []*query var seenPkgs map[string]bool + seenQuery := make(map[querySpec]bool) + var queries []*query + addQuery := func(q *query) { + if !seenQuery[q.querySpec] { + seenQuery[q.querySpec] = true + queries = append(queries, q) + } + } + if len(pkgPatterns) > 0 { // Don't load packages if pkgPatterns is empty. Both // modload.ImportPathsQuiet and ModulePackages convert an empty list @@ -474,16 +477,21 @@ func runGet(cmd *base.Command, args []string) { } else { matches = modload.ImportPathsQuiet(pkgPatterns) } - seenQuery := make(map[querySpec]bool) seenPkgs = make(map[string]bool) install = make([]string, 0, len(pkgPatterns)) for i, match := range matches { - if len(match.Pkgs) == 0 { - // We'll print a warning at the end of the outer loop to avoid - // repeating warnings on multiple iterations. + arg := pkgGets[i] + + if !*getM && len(match.Pkgs) == 0 { + // If the pattern did not match any packages, look up a new module. + // If the pattern doesn't match anything on the last iteration, + // we'll print a warning after the outer loop. + if !search.IsRelativePath(arg.path) && !match.Literal && arg.path != "all" { + addQuery(&query{querySpec: querySpec{path: arg.path, vers: arg.vers}, arg: arg.raw}) + } continue } - arg := pkgGets[i] + install = append(install, arg.path) allStd := true for _, pkg := range match.Pkgs { @@ -501,11 +509,7 @@ func runGet(cmd *base.Command, args []string) { continue } allStd = false - spec := querySpec{path: m.Path, vers: arg.vers} - if !seenQuery[spec] { - seenQuery[spec] = true - queries = append(queries, &query{querySpec: querySpec{path: m.Path, vers: arg.vers, forceModulePath: true, prevM: m}, arg: arg.raw}) - } + addQuery(&query{querySpec: querySpec{path: m.Path, vers: arg.vers, forceModulePath: true, prevM: m}, arg: arg.raw}) } if allStd { if *getM { diff --git a/src/cmd/go/testdata/mod/example.com_nest_sub_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_nest_sub_v1.0.0.txt new file mode 100644 index 00000000000..90f14598039 --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_nest_sub_v1.0.0.txt @@ -0,0 +1,12 @@ +Written by hand. +Test case for nested modules without an explicit relationship. +This is nested below the top-level module. + +-- .mod -- +module example.com/nest/sub +-- .info -- +{"Version": "v1.0.0"} +-- go.mod -- +module example.com/nest/sub +-- y/y.go -- +package y diff --git a/src/cmd/go/testdata/mod/example.com_nest_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_nest_v1.0.0.txt new file mode 100644 index 00000000000..593caf1d90a --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_nest_v1.0.0.txt @@ -0,0 +1,12 @@ +Written by hand. +Test case for nested modules without an explicit relationship. +This is the top-level module. + +-- .mod -- +module example.com/nest +-- .info -- +{"Version": "v1.0.0"} +-- go.mod -- +module example.com/nest +-- sub/x/x.go -- +package x diff --git a/src/cmd/go/testdata/mod/example.com_nest_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_nest_v1.1.0.txt new file mode 100644 index 00000000000..5a01550fd5d --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_nest_v1.1.0.txt @@ -0,0 +1,12 @@ +Written by hand. +Test case for nested modules without an explicit relationship. +This is the top-level module. + +-- .mod -- +module example.com/nest +-- .info -- +{"Version": "v1.1.0"} +-- go.mod -- +module example.com/nest +-- sub/x/x.go -- +package x diff --git a/src/cmd/go/testdata/script/mod_get_main.txt b/src/cmd/go/testdata/script/mod_get_main.txt index 0acb7179646..06f9f238777 100644 --- a/src/cmd/go/testdata/script/mod_get_main.txt +++ b/src/cmd/go/testdata/script/mod_get_main.txt @@ -16,9 +16,10 @@ stderr '^go get rsc.io@v0.1.0: can.t get a specific version of the main module$' ! go get -d rsc.io/x@v0.1.0 stderr '^go get rsc.io/x@v0.1.0: can.t query specific version for package rsc.io/x in the main module \(rsc.io\)$' -# TODO: upgrading a package pattern not contained in the main module should not +# Upgrading a package pattern not contained in the main module should not # attempt to upgrade the main module. -! go get rsc.io/quote/...@v1.5.1 +go get rsc.io/quote/...@v1.5.1 +grep 'rsc.io/quote v1.5.1' go.mod -- go.mod.orig -- module rsc.io diff --git a/src/cmd/go/testdata/script/mod_get_patterns.txt b/src/cmd/go/testdata/script/mod_get_patterns.txt index 1642ff2d107..733d4452d7a 100644 --- a/src/cmd/go/testdata/script/mod_get_patterns.txt +++ b/src/cmd/go/testdata/script/mod_get_patterns.txt @@ -22,6 +22,14 @@ stderr 'go get rsc.io/quote/x...: module rsc.io/quote@latest \(v1.5.2\) found, b stderr 'go get rsc.io/quote/x/...: module rsc.io/quote@latest \(v1.5.2\) found, but does not contain packages matching rsc.io/quote/x/...' ! grep 'require rsc.io/quote' go.mod +# If a pattern matches no packages within a module, the module should not +# be upgraded, even if the module path matches the pattern. +cp go.mod.orig go.mod +go mod edit -require example.com/nest@v1.0.0 +go get example.com/nest/sub/y... +grep 'example.com/nest/sub v1.0.0' go.mod +grep 'example.com/nest v1.0.0' go.mod + -- go.mod.orig -- module m