From a961d76aa0dd3dc17922882d382eff1b0916cc6c Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Thu, 15 Jun 2017 23:27:38 +0300 Subject: [PATCH 1/3] internal/gps: ensure packages are deducible before attempting to solve Add gps.ValidateParams to ensure all packages in SolverParams are deducible. Signed-off-by: Ibrahim AshShohail --- cmd/dep/ensure.go | 12 +++++++ internal/gps/hash_test.go | 3 +- internal/gps/solver.go | 48 ++++++++++++++++++++++++++-- internal/gps/solver_test.go | 62 +++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 internal/gps/solver_test.go diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index dd558b9047..5310cf275e 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -142,6 +142,18 @@ func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error { } } + err = gps.ValidateParams(params, sm) + if err != nil { + if deduceErrs, ok := err.(gps.DeductionErrs); ok { + ctx.Err.Println("The following errors occurred while deducing packages:") + for ip, dErr := range deduceErrs { + ctx.Err.Printf(" * \"%s\": %s", ip, dErr) + } + ctx.Err.Println() + } + return errors.Wrap(err, "validateParams") + } + solver, err := gps.Prepare(params, sm) if err != nil { return errors.Wrap(err, "ensure Prepare") diff --git a/internal/gps/hash_test.go b/internal/gps/hash_test.go index 220eebae5d..ac2300a6df 100644 --- a/internal/gps/hash_test.go +++ b/internal/gps/hash_test.go @@ -528,7 +528,8 @@ func TestHashInputsOverrides(t *testing.T) { s, err := Prepare(params, newdepspecSM(basefix.ds, nil)) if err != nil { - t.Fatalf("(fix: %q) Unexpected error while prepping solver: %s", fix.name, err) + t.Errorf("(fix: %q) Unexpected error while prepping solver: %s", fix.name, err) + continue } h := sha256.New() diff --git a/internal/gps/solver.go b/internal/gps/solver.go index fd8695fd13..c2354fbc38 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -10,6 +10,7 @@ import ( "log" "sort" "strings" + "sync" "github.com/armon/go-radix" "github.com/golang/dep/internal/gps/paths" @@ -381,6 +382,50 @@ func (s *solver) Version() int { return 1 } +// DeductionErrs maps package import path to errors occurring during deduction. +type DeductionErrs map[string]error + +func (e DeductionErrs) Error() string { + return "could not deduce external imports' project roots" +} + +// ValidateParams validates the solver parameters to ensure solving can be completed. +func ValidateParams(params SolveParameters, sm SourceManager) error { + // Ensure that all packages are deducible without issues. + var deducePkgsGroup sync.WaitGroup + deductionErrs := make(DeductionErrs) + var errsMut sync.Mutex + + rd, err := params.toRootdata() + if err != nil { + return err + } + + deducePkg := func(ip string, sm SourceManager) { + fmt.Println(ip) + _, err := sm.DeduceProjectRoot(ip) + if err != nil { + errsMut.Lock() + deductionErrs[ip] = err + errsMut.Unlock() + } + deducePkgsGroup.Done() + } + + for _, ip := range rd.externalImportList(paths.IsStandardImportPath) { + deducePkgsGroup.Add(1) + go deducePkg(ip, sm) + } + + deducePkgsGroup.Wait() + + if len(deductionErrs) > 0 { + return deductionErrs + } + + return nil +} + // Solve attempts to find a dependency solution for the given project, as // represented by the SolveParameters with which this Solver was created. // @@ -391,8 +436,7 @@ func (s *solver) Solve() (Solution, error) { s.vUnify.mtr = s.mtr // Prime the queues with the root project - err := s.selectRoot() - if err != nil { + if err := s.selectRoot(); err != nil { return nil, err } diff --git a/internal/gps/solver_test.go b/internal/gps/solver_test.go new file mode 100644 index 0000000000..f0032a154d --- /dev/null +++ b/internal/gps/solver_test.go @@ -0,0 +1,62 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gps + +import ( + "testing" + + "github.com/golang/dep/internal/gps/pkgtree" + "github.com/golang/dep/internal/test" +) + +func TestValidateParams(t *testing.T) { + h := test.NewHelper(t) + defer h.Cleanup() + + cacheDir := "gps-cache" + h.TempDir(cacheDir) + sm, err := NewSourceManager(h.Path(cacheDir)) + h.Must(err) + defer sm.Release() + + h.TempDir("src") + + testcases := []struct { + imports []string + err bool + }{ + {[]string{"google.com/non-existing/package"}, true}, + {[]string{"google.com/non-existing/package/subpkg"}, true}, + {[]string{"github.com/sdboyer/testrepo"}, false}, + {[]string{"github.com/sdboyer/testrepo/subpkg"}, false}, + } + + params := SolveParameters{ + ProjectAnalyzer: naiveAnalyzer{}, + RootDir: h.Path("src"), + RootPackageTree: pkgtree.PackageTree{ + ImportRoot: "github.com/sdboyer/dep", + }, + } + + for _, tc := range testcases { + params.RootPackageTree.Packages = map[string]pkgtree.PackageOrErr{ + "github.com/sdboyer/dep": { + P: pkgtree.Package{ + Name: "github.com/sdboyer/dep", + ImportPath: "github.com/sdboyer/dep", + Imports: tc.imports, + }, + }, + } + + err = ValidateParams(params, sm) + if tc.err && err == nil { + t.Fatalf("expected an error when deducing package fails, got none") + } else if !tc.err && err != nil { + t.Fatalf("deducing packges should have succeeded, got err: %#v", err) + } + } +} From 4a2215bcdac5c2e007b0eb394b78d62a02f2f076 Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Fri, 21 Jul 2017 03:41:45 +0300 Subject: [PATCH 2/3] rename internal/gps/solver_test.go -> internal/gps/solver_inputs_test.go Signed-off-by: Ibrahim AshShohail --- internal/gps/{solver_test.go => solver_inputs_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename internal/gps/{solver_test.go => solver_inputs_test.go} (100%) diff --git a/internal/gps/solver_test.go b/internal/gps/solver_inputs_test.go similarity index 100% rename from internal/gps/solver_test.go rename to internal/gps/solver_inputs_test.go From 92df3dc040fb5ad64d81f25d4ef6151b81324e6e Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Fri, 21 Jul 2017 03:42:26 +0300 Subject: [PATCH 3/3] gps: move TestBadSolveOpts to solver_inputs_test.go Signed-off-by: Ibrahim AshShohail --- internal/gps/solve_test.go | 152 ----------------------------- internal/gps/solver.go | 1 - internal/gps/solver_inputs_test.go | 152 +++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 153 deletions(-) diff --git a/internal/gps/solve_test.go b/internal/gps/solve_test.go index b2ccc087d3..6713dfecf3 100644 --- a/internal/gps/solve_test.go +++ b/internal/gps/solve_test.go @@ -8,17 +8,12 @@ import ( "bytes" "flag" "fmt" - "io/ioutil" "log" - "math/rand" "reflect" "sort" - "strconv" "strings" "testing" "unicode" - - "github.com/golang/dep/internal/gps/pkgtree" ) var fixtorun string @@ -292,150 +287,3 @@ func TestRootLockNoVersionPairMatching(t *testing.T) { fixtureSolveSimpleChecks(fix, res, err, t) } - -// TestBadSolveOpts exercises the different possible inputs to a solver that can -// be determined as invalid in Prepare(), without any further work -func TestBadSolveOpts(t *testing.T) { - pn := strconv.FormatInt(rand.Int63(), 36) - fix := basicFixtures["no dependencies"] - fix.ds[0].n = ProjectRoot(pn) - - sm := newdepspecSM(fix.ds, nil) - params := SolveParameters{ - mkBridgeFn: overrideMkBridge, - } - - _, err := Prepare(params, nil) - if err == nil { - t.Errorf("Prepare should have errored on nil SourceManager") - } else if !strings.Contains(err.Error(), "non-nil SourceManager") { - t.Error("Prepare should have given error on nil SourceManager, but gave:", err) - } - - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Prepare should have errored without ProjectAnalyzer") - } else if !strings.Contains(err.Error(), "must provide a ProjectAnalyzer") { - t.Error("Prepare should have given error without ProjectAnalyzer, but gave:", err) - } - - params.ProjectAnalyzer = naiveAnalyzer{} - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Prepare should have errored on empty root") - } else if !strings.Contains(err.Error(), "non-empty root directory") { - t.Error("Prepare should have given error on empty root, but gave:", err) - } - - params.RootDir = pn - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Prepare should have errored on empty name") - } else if !strings.Contains(err.Error(), "non-empty import root") { - t.Error("Prepare should have given error on empty import root, but gave:", err) - } - - params.RootPackageTree = pkgtree.PackageTree{ - ImportRoot: pn, - } - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Prepare should have errored on empty name") - } else if !strings.Contains(err.Error(), "at least one package") { - t.Error("Prepare should have given error on empty import root, but gave:", err) - } - - params.RootPackageTree = pkgtree.PackageTree{ - ImportRoot: pn, - Packages: map[string]pkgtree.PackageOrErr{ - pn: { - P: pkgtree.Package{ - ImportPath: pn, - Name: pn, - }, - }, - }, - } - params.TraceLogger = log.New(ioutil.Discard, "", 0) - - params.Manifest = simpleRootManifest{ - ovr: ProjectConstraints{ - ProjectRoot("foo"): ProjectProperties{}, - }, - } - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on override with empty ProjectProperties") - } else if !strings.Contains(err.Error(), "foo, but without any non-zero properties") { - t.Error("Prepare should have given error override with empty ProjectProperties, but gave:", err) - } - - params.Manifest = simpleRootManifest{ - ig: map[string]bool{"foo": true}, - req: map[string]bool{"foo": true}, - } - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on pkg both ignored and required") - } else if !strings.Contains(err.Error(), "was given as both a required and ignored package") { - t.Error("Prepare should have given error with single ignore/require conflict error, but gave:", err) - } - - params.Manifest = simpleRootManifest{ - ig: map[string]bool{"foo": true, "bar": true}, - req: map[string]bool{"foo": true, "bar": true}, - } - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on pkg both ignored and required") - } else if !strings.Contains(err.Error(), "multiple packages given as both required and ignored:") { - t.Error("Prepare should have given error with multiple ignore/require conflict error, but gave:", err) - } - params.Manifest = nil - - params.ToChange = []ProjectRoot{"foo"} - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on non-empty ToChange without a lock provided") - } else if !strings.Contains(err.Error(), "update specifically requested for") { - t.Error("Prepare should have given error on ToChange without Lock, but gave:", err) - } - - params.Lock = safeLock{ - p: []LockedProject{ - NewLockedProject(mkPI("bar"), Revision("makebelieve"), nil), - }, - } - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on ToChange containing project not in lock") - } else if !strings.Contains(err.Error(), "cannot update foo as it is not in the lock") { - t.Error("Prepare should have given error on ToChange with item not present in Lock, but gave:", err) - } - - params.Lock, params.ToChange = nil, nil - _, err = Prepare(params, sm) - if err != nil { - t.Error("Basic conditions satisfied, prepare should have completed successfully, err as:", err) - } - - // swap out the test mkBridge override temporarily, just to make sure we get - // the right error - params.mkBridgeFn = nil - - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on nonexistent root") - } else if !strings.Contains(err.Error(), "could not read project root") { - t.Error("Prepare should have given error nonexistent project root dir, but gave:", err) - } - - // Pointing it at a file should also be an err - params.RootDir = "solve_test.go" - _, err = Prepare(params, sm) - if err == nil { - t.Errorf("Should have errored on file for RootDir") - } else if !strings.Contains(err.Error(), "is a file, not a directory") { - t.Error("Prepare should have given error on file as RootDir, but gave:", err) - } -} diff --git a/internal/gps/solver.go b/internal/gps/solver.go index c2354fbc38..ea8573d501 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -402,7 +402,6 @@ func ValidateParams(params SolveParameters, sm SourceManager) error { } deducePkg := func(ip string, sm SourceManager) { - fmt.Println(ip) _, err := sm.DeduceProjectRoot(ip) if err != nil { errsMut.Lock() diff --git a/internal/gps/solver_inputs_test.go b/internal/gps/solver_inputs_test.go index f0032a154d..b6aad11f2a 100644 --- a/internal/gps/solver_inputs_test.go +++ b/internal/gps/solver_inputs_test.go @@ -5,12 +5,164 @@ package gps import ( + "io/ioutil" + "log" + "math/rand" + "strconv" + "strings" "testing" "github.com/golang/dep/internal/gps/pkgtree" "github.com/golang/dep/internal/test" ) +// TestBadSolveOpts exercises the different possible inputs to a solver that can +// be determined as invalid in Prepare(), without any further work +func TestBadSolveOpts(t *testing.T) { + pn := strconv.FormatInt(rand.Int63(), 36) + fix := basicFixtures["no dependencies"] + fix.ds[0].n = ProjectRoot(pn) + + sm := newdepspecSM(fix.ds, nil) + params := SolveParameters{ + mkBridgeFn: overrideMkBridge, + } + + _, err := Prepare(params, nil) + if err == nil { + t.Errorf("Prepare should have errored on nil SourceManager") + } else if !strings.Contains(err.Error(), "non-nil SourceManager") { + t.Error("Prepare should have given error on nil SourceManager, but gave:", err) + } + + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Prepare should have errored without ProjectAnalyzer") + } else if !strings.Contains(err.Error(), "must provide a ProjectAnalyzer") { + t.Error("Prepare should have given error without ProjectAnalyzer, but gave:", err) + } + + params.ProjectAnalyzer = naiveAnalyzer{} + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Prepare should have errored on empty root") + } else if !strings.Contains(err.Error(), "non-empty root directory") { + t.Error("Prepare should have given error on empty root, but gave:", err) + } + + params.RootDir = pn + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Prepare should have errored on empty name") + } else if !strings.Contains(err.Error(), "non-empty import root") { + t.Error("Prepare should have given error on empty import root, but gave:", err) + } + + params.RootPackageTree = pkgtree.PackageTree{ + ImportRoot: pn, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Prepare should have errored on empty name") + } else if !strings.Contains(err.Error(), "at least one package") { + t.Error("Prepare should have given error on empty import root, but gave:", err) + } + + params.RootPackageTree = pkgtree.PackageTree{ + ImportRoot: pn, + Packages: map[string]pkgtree.PackageOrErr{ + pn: { + P: pkgtree.Package{ + ImportPath: pn, + Name: pn, + }, + }, + }, + } + params.TraceLogger = log.New(ioutil.Discard, "", 0) + + params.Manifest = simpleRootManifest{ + ovr: ProjectConstraints{ + ProjectRoot("foo"): ProjectProperties{}, + }, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on override with empty ProjectProperties") + } else if !strings.Contains(err.Error(), "foo, but without any non-zero properties") { + t.Error("Prepare should have given error override with empty ProjectProperties, but gave:", err) + } + + params.Manifest = simpleRootManifest{ + ig: map[string]bool{"foo": true}, + req: map[string]bool{"foo": true}, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on pkg both ignored and required") + } else if !strings.Contains(err.Error(), "was given as both a required and ignored package") { + t.Error("Prepare should have given error with single ignore/require conflict error, but gave:", err) + } + + params.Manifest = simpleRootManifest{ + ig: map[string]bool{"foo": true, "bar": true}, + req: map[string]bool{"foo": true, "bar": true}, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on pkg both ignored and required") + } else if !strings.Contains(err.Error(), "multiple packages given as both required and ignored:") { + t.Error("Prepare should have given error with multiple ignore/require conflict error, but gave:", err) + } + params.Manifest = nil + + params.ToChange = []ProjectRoot{"foo"} + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on non-empty ToChange without a lock provided") + } else if !strings.Contains(err.Error(), "update specifically requested for") { + t.Error("Prepare should have given error on ToChange without Lock, but gave:", err) + } + + params.Lock = safeLock{ + p: []LockedProject{ + NewLockedProject(mkPI("bar"), Revision("makebelieve"), nil), + }, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on ToChange containing project not in lock") + } else if !strings.Contains(err.Error(), "cannot update foo as it is not in the lock") { + t.Error("Prepare should have given error on ToChange with item not present in Lock, but gave:", err) + } + + params.Lock, params.ToChange = nil, nil + _, err = Prepare(params, sm) + if err != nil { + t.Error("Basic conditions satisfied, prepare should have completed successfully, err as:", err) + } + + // swap out the test mkBridge override temporarily, just to make sure we get + // the right error + params.mkBridgeFn = nil + + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on nonexistent root") + } else if !strings.Contains(err.Error(), "could not read project root") { + t.Error("Prepare should have given error nonexistent project root dir, but gave:", err) + } + + // Pointing it at a file should also be an err + params.RootDir = "solve_test.go" + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on file for RootDir") + } else if !strings.Contains(err.Error(), "is a file, not a directory") { + t.Error("Prepare should have given error on file as RootDir, but gave:", err) + } +} + func TestValidateParams(t *testing.T) { h := test.NewHelper(t) defer h.Cleanup()