#907 Warn when an importer cannot preserve a constraint

.github/CODEOWNERS

@@ -8,6 +8,8 @@
 /cmd/dep/*_importer*                    @carolynvs
 /cmd/dep/testdata/glide                 @carolynvs
 /cmd/dep/testdata/godep                 @carolynvs
+/cmd/dep/testdata/vndr                  @carolynvs
+/cmd/dep/testdata/govend                @carolynvs
 /cmd/dep/testdata/harness_tests/init    @carolynvs
 /cmd/dep/testdata/init**                @carolynvs
 /analyzer*                              @carolynvs

CONTRIBUTING.md

@@ -22,6 +22,10 @@ The gophers there will answer or ask you to file an issue if you've tripped over
 
 ## Contributing code
 
+Let us know if you are interested in working on an issue by leaving a comment
+on the issue in GitHub. This helps avoid multiple people unknowingly 
+working on the same issue.
+
 Please read the [Contribution Guidelines](https://golang.org/doc/contribute.html)
 before sending patches.
 

README.md

@@ -152,6 +152,26 @@ This happens when a new import is added. Run `dep ensure` to install the missing
 
 As `dep status` suggests, run `dep ensure` to update your lockfile. Then run `dep status` again, and the lock mismatch should go away.
 
+### Visualizing dependencies
+
+Generate a visual representation of the dependency tree by piping the output of `dep status -dot` to [graphviz](http://www.graphviz.org/).
+#### Linux
+```
+$ sudo apt-get install graphviz
+$ dep status -dot | dot -T png | display
+```
+#### MacOS
+```
+$ brew install graphviz
+$ dep status -dot | dot -T png | open -f -a /Applications/Preview.app
+```
+#### Windows
+```
+> choco install graphviz.portable
+> dep status -dot | dot -T png -o status.png; start status.png
+```
+<p align="center"><img src="docs/img/StatusGraph.png"></p>
+
 ### Updating dependencies
 
 Updating brings the version of a dependency in `Gopkg.lock` and `vendor/` to the latest version allowed by the constraints in `Gopkg.toml`.

cmd/dep/ensure.go

@@ -174,8 +174,12 @@ func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error {
 		return errors.Wrap(err, "ensure ListPackage for project")
 	}
 
-	if err := checkErrors(params.RootPackageTree.Packages); err != nil {
-		return err
+	if fatal, err := checkErrors(params.RootPackageTree.Packages); err != nil {
+		if fatal {
+			return err
+		} else if ctx.Verbose {
+			ctx.Out.Println(err)
+		}
 	}
 
 	if cmd.add {
@@ -223,15 +227,15 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project
 
 	if p.Lock != nil && bytes.Equal(p.Lock.InputHash(), solver.HashInputs()) {
 		// Memo matches, so there's probably nothing to do.
+		if ctx.Verbose {
+			ctx.Out.Printf("%s was already in sync with imports and %s\n", dep.LockName, dep.ManifestName)
+		}
+
 		if cmd.noVendor {
 			// The user said not to touch vendor/, so definitely nothing to do.
 			return nil
 		}
 
-		if ctx.Verbose {
-			ctx.Out.Printf("%s was already in sync with imports and %s, recreating vendor/ directory", dep.LockName, dep.ManifestName)
-		}
-
 		// TODO(sdboyer) The desired behavior at this point is to determine
 		// whether it's necessary to write out vendor, or if it's already
 		// consistent with the lock. However, we haven't yet determined what
@@ -244,8 +248,7 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project
 		}
 
 		if cmd.dryRun {
-			ctx.Out.Printf("Would have populated vendor/ directory from %s", dep.LockName)
-			return nil
+			return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
 		}
 
 		logger := ctx.Err
@@ -261,12 +264,16 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project
 		return errors.Wrap(err, "ensure Solve()")
 	}
 
-	sw, err := dep.NewSafeWriter(nil, p.Lock, dep.LockFromSolution(solution), dep.VendorOnChanged)
+	vendorBehavior := dep.VendorOnChanged
+	if cmd.noVendor {
+		vendorBehavior = dep.VendorNever
+	}
+	sw, err := dep.NewSafeWriter(nil, p.Lock, dep.LockFromSolution(solution), vendorBehavior)
 	if err != nil {
 		return err
 	}
 	if cmd.dryRun {
-		return sw.PrintPreparedActions(ctx.Out)
+		return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
 	}
 
 	logger := ctx.Err
@@ -292,14 +299,7 @@ func (cmd *ensureCommand) runVendorOnly(ctx *dep.Ctx, args []string, p *dep.Proj
 	}
 
 	if cmd.dryRun {
-		ctx.Out.Printf("Would have populated vendor/ directory from %s", dep.LockName)
-		if ctx.Verbose {
-			err := sw.PrintPreparedActions(ctx.Err)
-			if err != nil {
-				return errors.WithMessage(err, "prepared actions")
-			}
-		}
-		return nil
+		return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
 	}
 
 	logger := ctx.Err
@@ -394,7 +394,7 @@ func (cmd *ensureCommand) runUpdate(ctx *dep.Ctx, args []string, p *dep.Project,
 		return err
 	}
 	if cmd.dryRun {
-		return sw.PrintPreparedActions(ctx.Out)
+		return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
 	}
 
 	logger := ctx.Err
@@ -603,9 +603,8 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 
 	// Prep post-actions and feedback from adds.
 	var reqlist []string
-	appender := &dep.Manifest{
-		Constraints: make(gps.ProjectConstraints),
-	}
+	appender := dep.NewManifest()
+
 	for pr, instr := range addInstructions {
 		for path := range instr.ephReq {
 			reqlist = append(reqlist, path)
@@ -648,7 +647,7 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 	}
 
 	if cmd.dryRun {
-		return sw.PrintPreparedActions(ctx.Out)
+		return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
 	}
 
 	logger := ctx.Err
@@ -748,10 +747,10 @@ func getProjectConstraint(arg string, sm gps.SourceManager) (gps.ProjectConstrai
 	return gps.ProjectConstraint{Ident: pi, Constraint: c}, arg, nil
 }
 
-func checkErrors(m map[string]pkgtree.PackageOrErr) error {
+func checkErrors(m map[string]pkgtree.PackageOrErr) (fatal bool, err error) {
 	var (
-		buildErrors []string
-		noGoErrors  int
+		noGoErrors    int
+		pkgtreeErrors = make(pkgtreeErrs, 0, len(m))
 	)
 
 	for _, poe := range m {
@@ -760,18 +759,42 @@ func checkErrors(m map[string]pkgtree.PackageOrErr) error {
 			case *build.NoGoError:
 				noGoErrors++
 			default:
-				buildErrors = append(buildErrors, poe.Err.Error())
+				pkgtreeErrors = append(pkgtreeErrors, poe.Err)
 			}
 		}
 	}
 
+	// If pkgtree was empty or all dirs lacked any Go code, return an error.
 	if len(m) == 0 || len(m) == noGoErrors {
-		return errors.New("all dirs lacked any go code")
+		return true, errors.New("no dirs contained any Go code")
 	}
 
-	if len(buildErrors) > 0 {
-		return errors.Errorf("Found %d errors:\n\n%s", len(buildErrors), strings.Join(buildErrors, "\n"))
+	// If all dirs contained build errors, return an error.
+	if len(m) == len(pkgtreeErrors) {
+		return true, errors.New("all dirs contained build errors")
 	}
 
-	return nil
+	// If all directories either had no Go files or caused a build error, return an error.
+	if len(m) == len(pkgtreeErrors)+noGoErrors {
+		return true, pkgtreeErrors
+	}
+
+	// If m contained some errors, return a warning with those errors.
+	if len(pkgtreeErrors) > 0 {
+		return false, pkgtreeErrors
+	}
+
+	return false, nil
+}
+
+type pkgtreeErrs []error
+
+func (e pkgtreeErrs) Error() string {
+	errs := make([]string, 0, len(e))
+
+	for _, err := range e {
+		errs = append(errs, err.Error())
+	}
+
+	return fmt.Sprintf("found %d errors in the package tree:\n%s", len(e), strings.Join(errs, "\n"))
 }

cmd/dep/ensure_test.go

@@ -56,21 +56,21 @@ func TestInvalidEnsureFlagCombinations(t *testing.T) {
 func TestCheckErrors(t *testing.T) {
 	tt := []struct {
 		name        string
-		hasErrs     bool
+		fatal       bool
 		pkgOrErrMap map[string]pkgtree.PackageOrErr
 	}{
 		{
-			name:    "noErrors",
-			hasErrs: false,
+			name:  "noErrors",
+			fatal: false,
 			pkgOrErrMap: map[string]pkgtree.PackageOrErr{
 				"mypkg": {
 					P: pkgtree.Package{},
 				},
 			},
 		},
 		{
-			name:    "hasErrors",
-			hasErrs: true,
+			name:  "hasErrors",
+			fatal: true,
 			pkgOrErrMap: map[string]pkgtree.PackageOrErr{
 				"github.com/me/pkg": {
 					Err: &build.NoGoError{},
@@ -81,8 +81,8 @@ func TestCheckErrors(t *testing.T) {
 			},
 		},
 		{
-			name:    "onlyGoErrors",
-			hasErrs: false,
+			name:  "onlyGoErrors",
+			fatal: false,
 			pkgOrErrMap: map[string]pkgtree.PackageOrErr{
 				"github.com/me/pkg": {
 					Err: &build.NoGoError{},
@@ -93,20 +93,48 @@ func TestCheckErrors(t *testing.T) {
 			},
 		},
 		{
-			name:    "allGoErrors",
-			hasErrs: true,
+			name:  "onlyBuildErrors",
+			fatal: false,
 			pkgOrErrMap: map[string]pkgtree.PackageOrErr{
 				"github.com/me/pkg": {
 					Err: &build.NoGoError{},
 				},
+				"github.com/someone/pkg": {
+					P: pkgtree.Package{},
+				},
+			},
+		},
+		{
+			name:  "allGoErrors",
+			fatal: true,
+			pkgOrErrMap: map[string]pkgtree.PackageOrErr{
+				"github.com/me/pkg": {
+					Err: &build.NoGoError{},
+				},
+			},
+		},
+		{
+			name:  "allMixedErrors",
+			fatal: true,
+			pkgOrErrMap: map[string]pkgtree.PackageOrErr{
+				"github.com/me/pkg": {
+					Err: &build.NoGoError{},
+				},
+				"github.com/someone/pkg": {
+					Err: errors.New("code is busted"),
+				},
 			},
 		},
 	}
 
 	for _, tc := range tt {
 		t.Run(tc.name, func(t *testing.T) {
-			if hasErrs := checkErrors(tc.pkgOrErrMap) != nil; hasErrs != tc.hasErrs {
-				t.Fail()
+			fatal, err := checkErrors(tc.pkgOrErrMap)
+			if tc.fatal != fatal {
+				t.Fatalf("expected fatal flag to be %T, got %T", tc.fatal, fatal)
+			}
+			if err == nil && fatal {
+				t.Fatal("unexpected fatal flag value while err is nil")
 			}
 		})
 	}

cmd/dep/glide_importer.go

@@ -141,9 +141,7 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e
 	task.WriteString("...")
 	g.logger.Println(task)
 
-	manifest := &dep.Manifest{
-		Constraints: make(gps.ProjectConstraints),
-	}
+	manifest := dep.NewManifest()
 
 	for _, pkg := range append(g.yaml.Imports, g.yaml.TestImports...) {
 		pc, err := g.buildProjectConstraint(pkg)
@@ -204,7 +202,8 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project
 	}
 	pc.Constraint, err = g.sm.InferConstraint(pkg.Reference, pc.Ident)
 	if err != nil {
-		return
+		g.logger.Printf("Unable to interpret revision specifier '%s' for package %s: %s", pkg.Name, pc.Ident, err.Error())
+		pc.Constraint = gps.Any()
 	}
 
 	if g.verbose {

cmd/dep/godep_importer.go

@@ -88,9 +88,7 @@ func (g *godepImporter) load(projectDir string) error {
 func (g *godepImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) {
 	g.logger.Println("Converting from Godeps.json ...")
 
-	manifest := &dep.Manifest{
-		Constraints: make(gps.ProjectConstraints),
-	}
+	manifest := dep.NewManifest()
 	lock := &dep.Lock{}
 
 	for _, pkg := range g.json.Imports {
@@ -160,7 +158,8 @@ func (g *godepImporter) buildProjectConstraint(pkg godepPackage) (pc gps.Project
 	pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.ImportPath)}
 	pc.Constraint, err = g.sm.InferConstraint(pkg.Comment, pc.Ident)
 	if err != nil {
-		return
+		g.logger.Printf("Unable to interpret revision specifier '%s' for package %s: %s", pkg.ImportPath, pc.Ident, err.Error())
+		pc.Constraint = gps.Any()
 	}
 
 	f := fb.NewConstraintFeedback(pc, fb.DepTypeImported)

cmd/dep/godep_importer_test.go

@@ -52,7 +52,6 @@ func TestGodepConfig_Convert(t *testing.T) {
 				},
 			},
 			convertTestCase: &convertTestCase{
-
 				projectRoot:    gps.ProjectRoot("github.com/sdboyer/deptest"),
 				wantConstraint: "^1.12.0-12-g2fd980e",
 				wantLockCount:  1,
@@ -70,7 +69,6 @@ func TestGodepConfig_Convert(t *testing.T) {
 				},
 			},
 			convertTestCase: &convertTestCase{
-
 				projectRoot:    gps.ProjectRoot("github.com/sdboyer/deptest"),
 				wantConstraint: "^1.0.0",
 				wantRevision:   gps.Revision("ff2948a2ac8f538c4ecd55962e919d1e13e74baf"),
@@ -83,7 +81,6 @@ func TestGodepConfig_Convert(t *testing.T) {
 				Imports: []godepPackage{{ImportPath: ""}},
 			},
 			convertTestCase: &convertTestCase{
-
 				wantConvertErr: true,
 			},
 		},
@@ -96,7 +93,6 @@ func TestGodepConfig_Convert(t *testing.T) {
 				},
 			},
 			convertTestCase: &convertTestCase{
-
 				wantConvertErr: true,
 			},
 		},
@@ -116,7 +112,6 @@ func TestGodepConfig_Convert(t *testing.T) {
 				},
 			},
 			convertTestCase: &convertTestCase{
-
 				projectRoot:    gps.ProjectRoot("github.com/sdboyer/deptest"),
 				wantLockCount:  1,
 				wantConstraint: "^1.0.0",