Merge pull request #2002 from sdboyer/noverify-preserve-files

sdboyer · web-flow · commit 3c04147ed4f9 · 2018-09-06T10:08:02.000-04:00
dep: Make noverify preserve excess vendor paths
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # v0.5.1
 
+IMPROVEMENTS:
+
+* The `noverify` field in `Gopkg.toml` allows for the preservation of excess files under `vendor`. ([#2002](https://github.com/golang/dep/issue/2002))
+
 BUG FIXES:
 
 * Correctly handle certain cases where `dep ensure` removed projects from Gopkg.lock. ([#1945](https://github.com/golang/dep/issue/1945)).
diff --git a/cmd/dep/check.go b/cmd/dep/check.go
@@ -141,62 +141,83 @@ func (cmd *checkCommand) Run(ctx *dep.Ctx, args []string) error {
 			noverify[skip] = true
 		}
 
-		var vendorfail bool
+		var vendorfail, hasnoverify bool
 		// One full pass through, to see if we need to print the header, and to
 		// create an array of names to sort for deterministic output.
 		var ordered []string
 		for path, status := range statuses {
 			ordered = append(ordered, path)
 
 			switch status {
-			case verify.DigestMismatchInLock, verify.HashVersionMismatch, verify.EmptyDigestInLock:
-				// NoVerify applies only to these three cases.
+			case verify.DigestMismatchInLock, verify.HashVersionMismatch, verify.EmptyDigestInLock, verify.NotInLock:
 				if noverify[path] {
+					hasnoverify = true
 					continue
 				}
 				fallthrough
-			case verify.NotInTree, verify.NotInLock:
+			case verify.NotInTree:
+				// NoVerify cannot be used to make dep check ignore the absence
+				// of a project entirely.
+				if noverify[path] {
+					delete(noverify, path)
+				}
+
 				fail = true
 				if !vendorfail {
 					vendorfail = true
-					logger.Println("# vendor is out of sync:")
 				}
-
 			}
 		}
 		sort.Strings(ordered)
 
+		var vfbuf, novbuf bytes.Buffer
+		var bufptr *bytes.Buffer
+
+		fmt.Fprintf(&vfbuf, "# vendor is out of sync:\n")
+		fmt.Fprintf(&novbuf, "# out of sync, but ignored, due to noverify in Gopkg.toml:\n")
+
 		for _, pr := range ordered {
-			var nvSuffix string
 			if noverify[pr] {
-				nvSuffix = "  (CHECK IGNORED: marked noverify in Gopkg.toml)"
+				bufptr = &novbuf
+			} else {
+				bufptr = &vfbuf
 			}
 
 			status := statuses[pr]
 			switch status {
 			case verify.NotInTree:
-				logger.Printf("%s: missing from vendor\n", pr)
+				fmt.Fprintf(bufptr, "%s: missing from vendor\n", pr)
 			case verify.NotInLock:
 				fi, err := os.Stat(filepath.Join(p.AbsRoot, "vendor", pr))
 				if err != nil {
 					return errors.Wrap(err, "could not stat file that VerifyVendor claimed existed")
 				}
 				if fi.IsDir() {
-					logger.Printf("%s: unused project\n", pr)
+					fmt.Fprintf(bufptr, "%s: unused project\n", pr)
 				} else {
-					logger.Printf("%s: orphaned file\n", pr)
+					fmt.Fprintf(bufptr, "%s: orphaned file\n", pr)
 				}
 			case verify.DigestMismatchInLock:
-				logger.Printf("%s: hash of vendored tree not equal to digest in Gopkg.lock%s\n", pr, nvSuffix)
+				fmt.Fprintf(bufptr, "%s: hash of vendored tree not equal to digest in Gopkg.lock\n", pr)
 			case verify.EmptyDigestInLock:
-				logger.Printf("%s: no digest in Gopkg.lock to compare against hash of vendored tree%s\n", pr, nvSuffix)
+				fmt.Fprintf(bufptr, "%s: no digest in Gopkg.lock to compare against hash of vendored tree\n", pr)
 			case verify.HashVersionMismatch:
 				// This will double-print if the hash version is zero, but
 				// that's a rare case that really only occurs before the first
 				// run with a version of dep >=0.5.0, so it's fine.
-				logger.Printf("%s: hash algorithm mismatch, want version %v%s\n", pr, verify.HashVersion, nvSuffix)
+				fmt.Fprintf(bufptr, "%s: hash algorithm mismatch, want version %v\n", pr, verify.HashVersion)
 			}
 		}
+
+		if vendorfail {
+			logger.Print(vfbuf.String())
+			if hasnoverify {
+				logger.Println()
+			}
+		}
+		if hasnoverify {
+			logger.Print(novbuf.String())
+		}
 	}
 
 	if fail {
diff --git a/cmd/dep/testdata/harness_tests/check/noverify/hash_mismatch/stdout.txt b/cmd/dep/testdata/harness_tests/check/noverify/hash_mismatch/stdout.txt
@@ -1 +1,2 @@
-github.com/sdboyer/deptest: hash of vendored tree not equal to digest in Gopkg.lock  (CHECK IGNORED: marked noverify in Gopkg.toml)
+# out of sync, but ignored, due to noverify in Gopkg.toml:
+github.com/sdboyer/deptest: hash of vendored tree not equal to digest in Gopkg.lock
diff --git a/cmd/dep/testdata/harness_tests/check/noverify/hash_version_mismatch/stdout.txt b/cmd/dep/testdata/harness_tests/check/noverify/hash_version_mismatch/stdout.txt
@@ -1 +1,2 @@
-github.com/sdboyer/deptest: hash algorithm mismatch, want version 1  (CHECK IGNORED: marked noverify in Gopkg.toml)
+# out of sync, but ignored, due to noverify in Gopkg.toml:
+github.com/sdboyer/deptest: hash algorithm mismatch, want version 1
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/hash_mismatch/README b/cmd/dep/testdata/harness_tests/ensure/noverify/hash_mismatch/README
@@ -2,4 +2,4 @@ This is a hack - it's effectively just verifying that the Gopkg.lock doesn't
 change for projects with noverify set, which (under the current logic) is an
 indicator that vendor wasn't updated.
 
-Of course, that vendor -> lock relatinoship isn't guaranteed to hold...
+Of course, that vendor -> lock relationship isn't guaranteed to hold...
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/final/Gopkg.lock
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/final/Gopkg.toml
@@ -0,0 +1 @@
+noverify = ["foo", "orphdir"]
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/Gopkg.lock
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/Gopkg.toml
@@ -0,0 +1 @@
+noverify = ["foo", "orphdir"]
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/main.go b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/main.go
@@ -0,0 +1,12 @@
+// 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 main
+
+import (
+	_ "github.com/sdboyer/deptest"
+)
+
+func main() {
+}
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/vendor/foo b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/vendor/foo
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/vendor/github.com/sdboyer/deptest/deptest.go b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/vendor/github.com/sdboyer/deptest/deptest.go
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/vendor/orphdir/.gitkeep b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/initial/vendor/orphdir/.gitkeep
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/stdout.txt b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/stdout.txt
@@ -0,0 +1,3 @@
+# out of sync, but ignored, due to noverify in Gopkg.toml:
+foo: orphaned file
+orphdir: unused project
diff --git a/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/testcase.json b/cmd/dep/testdata/harness_tests/ensure/noverify/vendororphans/testcase.json
@@ -0,0 +1,9 @@
+{
+  "commands": [
+    ["ensure"],
+    ["check"]
+  ],
+  "vendor-final": [
+    "github.com/sdboyer/deptest"
+  ]
+}
diff --git a/docs/Gopkg.toml.md b/docs/Gopkg.toml.md
@@ -231,18 +231,20 @@ It is usually safe to set `non-go = true`, as well. However, as dep only has a c
 
 ## `noverify`
 
-The `noverify` field is a list of [project roots](glossary.md#project-root) to exclude from [vendor verification](glossary.md#vendor-verification).
+The `noverify` field is a list of paths, typically [project roots](glossary.md#project-root), to exclude from [vendor verification](glossary.md#vendor-verification).
 
 Dep uses per-project hash digests, computed after pruning and recorded in [Gopkg.lock](Gopkg.lock.md#digest), to determine if the contents of `vendor/` are as expected. If the recorded digest and the hash of the corresponding tree in `vendor/` differ, that project is considered to be out of sync:
 
 * `dep ensure` will regenerate it
 * `dep check` will complain of a hash mismatch and exit 1
 
-It is strongly preferable for almost all workflows that you leave `vendor/` unmodified, in whatever state dep puts it in. However, this isn't always an option. If you have no choice but to modify `vendor/` for a particular project, then add the project root for that project to `noverify`. This will have the following effects:
+It is strongly recommended that you leave `vendor/` unmodified, in whatever state dep puts it in. However, this isn't always feasible. If you have no choice but to modify `vendor/` for a particular project, then add the project root for that project to `noverify`. This will have the following effects:
 
 * `dep ensure` will ignore hash mismatches for the project, and only regenerate it in `vendor/` if absolutely necessary (prune options change, package list changes, version changes)
 * `dep check` will continue to report hash mismatches (albeit with an annotation about `noverify`) for the project, but will no longer exit 1. 
 
+`noverify` can also be used to preserve certain excess paths that would otherwise be removed; for example, adding `WORKSPACE` to the `noverify` list would allow you to preserve `vendor/WORKSPACE`, which can help with some Bazel-based workflows.
+
 ## Scope
 
 `dep` evaluates
diff --git a/txn_writer.go b/txn_writer.go
@@ -432,6 +432,7 @@ const (
 	missingFromTree
 	projectAdded
 	projectRemoved
+	pathPreserved
 )
 
 // NewDeltaWriter prepares a vendor writer that will construct a vendor
@@ -510,17 +511,26 @@ func NewDeltaWriter(p *Project, newLock *Lock, behavior VendorBehavior) (TreeWri
 		// We don't validate this field elsewhere as it can be difficult to know
 		// at the beginning of a dep ensure command whether or not the noverify
 		// project actually will exist as part of the Lock by the end of the
-		// run. So, only apply if it's in the lockdiff, and isn't a removal.
+		// run. So, only apply if it's in the lockdiff.
 		if _, has := dw.lockDiff.ProjectDeltas[pr]; has {
-			if typ, has := dw.changed[pr]; has && typ < noVerify {
-				// Avoid writing noverify projects at all for the lower change
-				// types.
-				delete(dw.changed, pr)
-
-				// Uncomment this if we want to switch to the safer behavior,
-				// where we ALWAYS write noverify projects.
-				//dw.changed[pr] = noVerify
+			if typ, has := dw.changed[pr]; has {
+				if typ < noVerify {
+					// Avoid writing noverify projects at all for the lower change
+					// types.
+					delete(dw.changed, pr)
+
+					// Uncomment this if we want to switch to the safer behavior,
+					// where we ALWAYS write noverify projects.
+					//dw.changed[pr] = noVerify
+				} else if typ == projectRemoved {
+					// noverify can also be used to preserve files that would
+					// otherwise be removed.
+					dw.changed[pr] = pathPreserved
+				}
 			}
+			// It's also allowed to preserve entirely unknown paths using noverify.
+		} else if _, has := status[spr]; has {
+			dw.changed[pr] = pathPreserved
 		}
 	}
 
@@ -564,29 +574,28 @@ func (dw *DeltaWriter) Write(path string, sm gps.SourceManager, examples bool, l
 		projs[lp.Ident().ProjectRoot] = lp
 	}
 
-	dropped := []gps.ProjectRoot{}
+	var dropped, preserved []gps.ProjectRoot
 	i := 0
 	tot := len(dw.changed)
-	if len(dw.changed) > 0 {
-		logger.Println("# Bringing vendor into sync")
+	for _, reason := range dw.changed {
+		if reason != pathPreserved {
+			logger.Println("# Bringing vendor into sync")
+			break
+		}
 	}
+
 	for pr, reason := range dw.changed {
-		if reason == projectRemoved {
+		switch reason {
+		case projectRemoved:
 			dropped = append(dropped, pr)
 			continue
+		case pathPreserved:
+			preserved = append(preserved, pr)
+			continue
 		}
 
 		to := filepath.FromSlash(filepath.Join(vnewpath, string(pr)))
-		proj, has := projs[pr]
-		if !has {
-			// This shouldn't be reachable, but it's preferable to print an
-			// error and continue rather than panic. https://github.com/golang/dep/issues/1945
-			// TODO(sdboyer) remove this once we've increased confidence around
-			// this case.
-			fmt.Fprintf(os.Stderr, "Internal error - %s had change code %v but was not in new Gopkg.lock. Re-running dep ensure should fix this. Please file a bug at https://github.com/golang/dep/issues/new!\n", pr, reason)
-			continue
-		}
-		po := proj.(verify.VerifiableProject).PruneOpts
+		po := projs[pr].(verify.VerifiableProject).PruneOpts
 		if err := sm.ExportPrunedProject(context.TODO(), projs[pr], po, to); err != nil {
 			return errors.Wrapf(err, "failed to export %s", pr)
 		}
@@ -666,13 +675,18 @@ func (dw *DeltaWriter) Write(path string, sm gps.SourceManager, examples bool, l
 		}
 	}
 
-	// Ensure vendor/.git is preserved if present
+	// Special case: ensure vendor/.git is preserved if present
 	if hasDotGit(vpath) {
-		err = fs.RenameWithFallback(filepath.Join(vpath, ".git"), filepath.Join(vnewpath, ".git"))
+		preserved = append(preserved, ".git")
+	}
+
+	for _, path := range preserved {
+		err = fs.RenameWithFallback(filepath.Join(vpath, string(path)), filepath.Join(vnewpath, string(path)))
 		if err != nil {
-			return errors.Wrap(err, "failed to preserve vendor/.git")
+			return errors.Wrapf(err, "failed to preserve vendor/%s", path)
 		}
 	}
+
 	err = os.RemoveAll(vpath)
 	if err != nil {
 		return errors.Wrap(err, "failed to remove original vendor directory")

Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
`1`		`-github.com/sdboyer/deptest: hash of vendored tree not equal to digest in Gopkg.lock (CHECK IGNORED: marked noverify in Gopkg.toml)`
	`1`	`+# out of sync, but ignored, due to noverify in Gopkg.toml:`
	`2`	`+github.com/sdboyer/deptest: hash of vendored tree not equal to digest in Gopkg.lock`