From 1e3cdd1edce67350f8007e4a9b9b555f1e27c5b4 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Mon, 11 Mar 2019 18:53:08 -0400 Subject: [PATCH] cmd/go: print require chains in build list errors mvs.BuildList and functions that invoke it directly (UpgradeAll) now return an *mvs.BuildListError when there is an error retrieving the requirements for a module. This new error prints the chain of requirements from the main module to the module where the error occurred. These errors come up most commonly when a go.mod file has an unexpected module path or can't be parsed for some other reason. It's currently difficult to debug these errors because it's not clear where the "bad" module is required from. Tools like "go list -m" and "go mod why" don't work without the build graph. Fixes #30661 Change-Id: I3c9d4683dcd9a5d7c259e5e4cc7e1ee209700b10 Reviewed-on: https://go-review.googlesource.com/c/go/+/166984 Run-TryBot: Jay Conrod TryBot-Result: Gobot Gobot Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/modload/load.go | 23 +--- src/cmd/go/internal/mvs/mvs.go | 122 ++++++++++++++---- src/cmd/go/internal/mvs/mvs_test.go | 5 +- .../mod/example.com_badchain_a_v1.0.0.txt | 12 ++ .../mod/example.com_badchain_a_v1.1.0.txt | 12 ++ .../mod/example.com_badchain_b_v1.0.0.txt | 12 ++ .../mod/example.com_badchain_b_v1.1.0.txt | 12 ++ .../mod/example.com_badchain_c_v1.0.0.txt | 8 ++ .../mod/example.com_badchain_c_v1.1.0.txt | 8 ++ .../go/testdata/script/mod_load_badchain.txt | 41 ++++++ 10 files changed, 209 insertions(+), 46 deletions(-) create mode 100644 src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt create mode 100644 src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt create mode 100644 src/cmd/go/testdata/script/mod_load_badchain.txt diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go index ea0ac6771f..78681b165a 100644 --- a/src/cmd/go/internal/modload/load.go +++ b/src/cmd/go/internal/modload/load.go @@ -1023,13 +1023,11 @@ func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) { gomod := filepath.Join(dir, "go.mod") data, err := ioutil.ReadFile(gomod) if err != nil { - base.Errorf("go: parsing %s: %v", base.ShortPath(gomod), err) - return nil, ErrRequire + return nil, fmt.Errorf("parsing %s: %v", base.ShortPath(gomod), err) } f, err := modfile.ParseLax(gomod, data, nil) if err != nil { - base.Errorf("go: parsing %s: %v", base.ShortPath(gomod), err) - return nil, ErrRequire + return nil, fmt.Errorf("parsing %s: %v", base.ShortPath(gomod), err) } if f.Go != nil { r.versions.LoadOrStore(mod, f.Go.Version) @@ -1050,22 +1048,18 @@ func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) { data, err := modfetch.GoMod(mod.Path, mod.Version) if err != nil { - base.Errorf("go: %s@%s: %v\n", mod.Path, mod.Version, err) - return nil, ErrRequire + return nil, fmt.Errorf("%s@%s: %v", mod.Path, mod.Version, err) } f, err := modfile.ParseLax("go.mod", data, nil) if err != nil { - base.Errorf("go: %s@%s: parsing go.mod: %v", mod.Path, mod.Version, err) - return nil, ErrRequire + return nil, fmt.Errorf("%s@%s: parsing go.mod: %v", mod.Path, mod.Version, err) } if f.Module == nil { - base.Errorf("go: %s@%s: parsing go.mod: missing module line", mod.Path, mod.Version) - return nil, ErrRequire + return nil, fmt.Errorf("%s@%s: parsing go.mod: missing module line", mod.Path, mod.Version) } if mpath := f.Module.Mod.Path; mpath != origPath && mpath != mod.Path { - base.Errorf("go: %s@%s: parsing go.mod: unexpected module path %q", mod.Path, mod.Version, mpath) - return nil, ErrRequire + return nil, fmt.Errorf("%s@%s: parsing go.mod: unexpected module path %q", mod.Path, mod.Version, mpath) } if f.Go != nil { r.versions.LoadOrStore(mod, f.Go.Version) @@ -1074,11 +1068,6 @@ func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) { return r.modFileToList(f), nil } -// ErrRequire is the sentinel error returned when Require encounters problems. -// It prints the problems directly to standard error, so that multiple errors -// can be displayed easily. -var ErrRequire = errors.New("error loading module requirements") - func (*mvsReqs) Max(v1, v2 string) string { if v1 != "" && semver.Compare(v1, v2) == -1 { return v2 diff --git a/src/cmd/go/internal/mvs/mvs.go b/src/cmd/go/internal/mvs/mvs.go index aa109693f3..160e6089db 100644 --- a/src/cmd/go/internal/mvs/mvs.go +++ b/src/cmd/go/internal/mvs/mvs.go @@ -9,7 +9,9 @@ package mvs import ( "fmt" "sort" + "strings" "sync" + "sync/atomic" "cmd/go/internal/base" "cmd/go/internal/module" @@ -59,12 +61,38 @@ type Reqs interface { Previous(m module.Version) (module.Version, error) } -type MissingModuleError struct { - Module module.Version +// BuildListError decorates an error that occurred gathering requirements +// while constructing a build list. BuildListError prints the chain +// of requirements to the module where the error occurred. +type BuildListError struct { + Err error + Stack []module.Version } -func (e *MissingModuleError) Error() string { - return fmt.Sprintf("missing module: %v", e.Module) +func (e *BuildListError) Error() string { + b := &strings.Builder{} + errMsg := e.Err.Error() + stack := e.Stack + + // Don't print modules at the beginning of the chain without a + // version. These always seem to be the main module or a + // synthetic module ("target@"). + for len(stack) > 0 && stack[len(stack)-1].Version == "" { + stack = stack[:len(stack)-1] + } + + // Don't print the last module if the error message already + // starts with module path and version. + if len(stack) > 0 && strings.HasPrefix(errMsg, fmt.Sprintf("%s@%s: ", stack[0].Path, stack[0].Version)) { + // error already mentions module + stack = stack[1:] + } + + for i := len(stack) - 1; i >= 0; i-- { + fmt.Fprintf(b, "%s@%s ->\n\t", stack[i].Path, stack[i].Version) + } + b.WriteString(errMsg) + return b.String() } // BuildList returns the build list for the target module. @@ -78,33 +106,40 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo // does high-latency network operations. var work par.Work work.Add(target) + + type modGraphNode struct { + m module.Version + required []module.Version + upgrade module.Version + err error + } var ( mu sync.Mutex - min = map[string]string{target.Path: target.Version} - firstErr error + modGraph = map[module.Version]*modGraphNode{} + min = map[string]string{} // maps module path to minimum required version + haveErr int32 ) + + work.Add(target) work.Do(10, func(item interface{}) { m := item.(module.Version) - required, err := reqs.Required(m) + node := &modGraphNode{m: m} mu.Lock() - if err != nil && firstErr == nil { - firstErr = err - } - if firstErr != nil { - mu.Unlock() - return - } + modGraph[m] = node if v, ok := min[m.Path]; !ok || reqs.Max(v, m.Version) != v { min[m.Path] = m.Version } mu.Unlock() - for _, r := range required { - if r.Path == "" { - base.Errorf("Required(%v) returned zero module in list", m) - continue - } + required, err := reqs.Required(m) + if err != nil { + node.err = err + atomic.StoreInt32(&haveErr, 1) + return + } + node.required = required + for _, r := range node.required { work.Add(r) } @@ -114,13 +149,49 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo base.Errorf("Upgrade(%v) returned zero module", m) return } - work.Add(u) + if u != m { + node.upgrade = u + work.Add(u) + } } }) - if firstErr != nil { - return nil, firstErr + // If there was an error, find the shortest path from the target to the + // node where the error occurred so we can report a useful error message. + if haveErr != 0 { + // neededBy[a] = b means a was added to the module graph by b. + neededBy := make(map[*modGraphNode]*modGraphNode) + q := make([]*modGraphNode, 0, len(modGraph)) + q = append(q, modGraph[target]) + for len(q) > 0 { + node := q[0] + q = q[1:] + + if node.err != nil { + err := &BuildListError{Err: node.err} + for n := node; n != nil; n = neededBy[n] { + err.Stack = append(err.Stack, n.m) + } + return nil, err + } + + neighbors := node.required + if node.upgrade.Path != "" { + neighbors = append(neighbors, node.upgrade) + } + for _, neighbor := range neighbors { + nn := modGraph[neighbor] + if neededBy[nn] != nil { + continue + } + neededBy[nn] = node + q = append(q, nn) + } + } } + + // Construct the list by traversing the graph again, replacing older + // modules with required minimum versions. if v := min[target.Path]; v != target.Version { panic(fmt.Sprintf("mistake: chose version %q instead of target %+v", v, target)) // TODO: Don't panic. } @@ -128,11 +199,8 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo list := []module.Version{target} listed := map[string]bool{target.Path: true} for i := 0; i < len(list); i++ { - m := list[i] - required, err := reqs.Required(m) - if err != nil { - return nil, err - } + n := modGraph[list[i]] + required := n.required for _, r := range required { v := min[r.Path] if r.Path != target.Path && reqs.Max(v, r.Version) != v { diff --git a/src/cmd/go/internal/mvs/mvs_test.go b/src/cmd/go/internal/mvs/mvs_test.go index 2a27dfb288..cab4bb241b 100644 --- a/src/cmd/go/internal/mvs/mvs_test.go +++ b/src/cmd/go/internal/mvs/mvs_test.go @@ -5,6 +5,7 @@ package mvs import ( + "fmt" "reflect" "strings" "testing" @@ -446,7 +447,7 @@ func (r reqsMap) Upgrade(m module.Version) (module.Version, error) { } } if u.Path == "" { - return module.Version{}, &MissingModuleError{module.Version{Path: m.Path, Version: ""}} + return module.Version{}, fmt.Errorf("missing module: %v", module.Version{Path: m.Path}) } return u, nil } @@ -467,7 +468,7 @@ func (r reqsMap) Previous(m module.Version) (module.Version, error) { func (r reqsMap) Required(m module.Version) ([]module.Version, error) { rr, ok := r[m] if !ok { - return nil, &MissingModuleError{m} + return nil, fmt.Errorf("missing module: %v", m) } return rr, nil } diff --git a/src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt new file mode 100644 index 0000000000..d7bf6471b7 --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt @@ -0,0 +1,12 @@ +example.com/badchain/a v1.0.0 + +-- .mod -- +module example.com/badchain/a + +require example.com/badchain/b v1.0.0 +-- .info -- +{"Version":"v1.0.0"} +-- a.go -- +package a + +import _ "example.com/badchain/b" diff --git a/src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt new file mode 100644 index 0000000000..92190d8ac1 --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt @@ -0,0 +1,12 @@ +example.com/badchain/a v1.1.0 + +-- .mod -- +module example.com/badchain/a + +require example.com/badchain/b v1.1.0 +-- .info -- +{"Version":"v1.1.0"} +-- a.go -- +package a + +import _ "example.com/badchain/b" diff --git a/src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt new file mode 100644 index 0000000000..d42b8aab16 --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt @@ -0,0 +1,12 @@ +example.com/badchain/b v1.0.0 + +-- .mod -- +module example.com/badchain/b + +require example.com/badchain/c v1.0.0 +-- .info -- +{"Version":"v1.0.0"} +-- b.go -- +package b + +import _ "example.com/badchain/c" diff --git a/src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt new file mode 100644 index 0000000000..664818474c --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt @@ -0,0 +1,12 @@ +example.com/badchain/b v1.1.0 + +-- .mod -- +module example.com/badchain/b + +require example.com/badchain/c v1.1.0 +-- .info -- +{"Version":"v1.1.0"} +-- b.go -- +package b + +import _ "example.com/badchain/c" diff --git a/src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt new file mode 100644 index 0000000000..9c717cb0e6 --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt @@ -0,0 +1,8 @@ +example.com/badchain/c v1.0.0 + +-- .mod -- +module example.com/badchain/c +-- .info -- +{"Version":"v1.0.0"} +-- c.go -- +package c diff --git a/src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt new file mode 100644 index 0000000000..da19ebd9ec --- /dev/null +++ b/src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt @@ -0,0 +1,8 @@ +example.com/badchain/c v1.1.0 + +-- .mod -- +module example.com/badchain/wrong +-- .info -- +{"Version":"v1.1.0"} +-- c.go -- +package c diff --git a/src/cmd/go/testdata/script/mod_load_badchain.txt b/src/cmd/go/testdata/script/mod_load_badchain.txt new file mode 100644 index 0000000000..ded6e1669d --- /dev/null +++ b/src/cmd/go/testdata/script/mod_load_badchain.txt @@ -0,0 +1,41 @@ +[short] skip +env GO111MODULE=on + +# Download everything to avoid "finding" messages in stderr later. +cp go.mod.orig go.mod +go mod download +go mod download example.com/badchain/a@v1.1.0 +go mod download example.com/badchain/b@v1.1.0 +go mod download example.com/badchain/c@v1.1.0 + +# Try to upgrade example.com/badchain/a (and its dependencies). +! go get -u example.com/badchain/a +cmp stderr upgrade-a-expected +cmp go.mod go.mod.orig + +# Try to upgrade the main module. This upgrades everything, including +# modules that aren't direct requirements, so the error stack is shorter. +! go get -u +cmp stderr upgrade-main-expected +cmp go.mod go.mod.orig + +# Upgrade manually. Listing modules should produce an error. +go mod edit -require=example.com/badchain/a@v1.1.0 +! go list -m +cmp stderr list-expected + +-- go.mod.orig -- +module m + +require example.com/badchain/a v1.0.0 +-- upgrade-main-expected -- +go get: example.com/badchain/c@v1.0.0 -> + example.com/badchain/c@v1.1.0: parsing go.mod: unexpected module path "example.com/badchain/wrong" +-- upgrade-a-expected -- +go get: example.com/badchain/a@v1.1.0 -> + example.com/badchain/b@v1.1.0 -> + example.com/badchain/c@v1.1.0: parsing go.mod: unexpected module path "example.com/badchain/wrong" +-- list-expected -- +go: example.com/badchain/a@v1.1.0 -> + example.com/badchain/b@v1.1.0 -> + example.com/badchain/c@v1.1.0: parsing go.mod: unexpected module path "example.com/badchain/wrong" -- 2.50.0