From 45971c60c31303ed9100d3ac8c3030c4d48d7084 Mon Sep 17 00:00:00 2001
From: Russ Cox <rsc@golang.org>
Date: Fri, 31 Jul 2015 11:54:42 -0400
Subject: [PATCH] cmd/go: fix disallow of p/vendor/x during vendor experiment

The percolation of errors upward in the load process could
drop errors, meaning that a build tree could, depending on the
processing order, import the same directory as both "p/vendor/x"
and as "x". That's not supposed to be allowed. But then, worse,
the build would generate two jobs for building that directory,
which would use the same work space and overwrite each other's files,
leading to very strange failures.

Two fixes:

1. Fix the propagation of errors upward (prefer errors over success).
2. Check explicitly for duplicated packages before starting a build.

New test for #1.
Since #2 can't happen, tested #2 by hand after reverting fix for #1.

Fixes #11913.

Change-Id: I6d2fc65f93b8fb5f3b263ace8d5f68d803a2ae5c
Reviewed-on: https://go-review.googlesource.com/13022
Reviewed-by: Ian Lance Taylor <iant@golang.org>
---
 src/cmd/go/pkg.go                             | 26 +++++++++++++++----
 src/cmd/go/testdata/testvendor/src/p/p.go     |  6 +++++
 .../testdata/testvendor/src/q/vendor/x/x.go   |  1 +
 src/cmd/go/testdata/testvendor/src/q/y/y.go   |  3 +++
 src/cmd/go/testdata/testvendor/src/q/z/z.go   |  3 +++
 src/cmd/go/vendor_test.go                     |  9 +++++++
 6 files changed, 43 insertions(+), 5 deletions(-)
 create mode 100644 src/cmd/go/testdata/testvendor/src/p/p.go
 create mode 100644 src/cmd/go/testdata/testvendor/src/q/vendor/x/x.go
 create mode 100644 src/cmd/go/testdata/testvendor/src/q/y/y.go
 create mode 100644 src/cmd/go/testdata/testvendor/src/q/z/z.go

diff --git a/src/cmd/go/pkg.go b/src/cmd/go/pkg.go
index a2c5ba7e5e7c6..0b61b0eeb4cb5 100644
--- a/src/cmd/go/pkg.go
+++ b/src/cmd/go/pkg.go
@@ -900,11 +900,10 @@ func (p *Package) load(stk *importStack, bp *build.Package, err error) *Package
 		deps[path] = p1
 		imports = append(imports, p1)
 		for _, dep := range p1.deps {
-			// Do not overwrite entries installed by direct import
-			// just above this loop. Those have stricter constraints
-			// about internal and vendor visibility and may contain
-			// errors that we need to preserve.
-			if deps[dep.ImportPath] == nil {
+			// The same import path could produce an error or not,
+			// depending on what tries to import it.
+			// Prefer to record entries with errors, so we can report them.
+			if deps[dep.ImportPath] == nil || dep.Error != nil {
 				deps[dep.ImportPath] = dep
 			}
 		}
@@ -1612,6 +1611,23 @@ func packagesForBuild(args []string) []*Package {
 		}
 	}
 	exitIfErrors()
+
+	// Check for duplicate loads of the same package.
+	// That should be impossible, but if it does happen then
+	// we end up trying to build the same package twice,
+	// usually in parallel overwriting the same files,
+	// which doesn't work very well.
+	seen := map[string]bool{}
+	reported := map[string]bool{}
+	for _, pkg := range packageList(pkgs) {
+		if seen[pkg.ImportPath] && !reported[pkg.ImportPath] {
+			reported[pkg.ImportPath] = true
+			errorf("internal error: duplicate loads of %s", pkg.ImportPath)
+		}
+		seen[pkg.ImportPath] = true
+	}
+	exitIfErrors()
+
 	return pkgs
 }
 
diff --git a/src/cmd/go/testdata/testvendor/src/p/p.go b/src/cmd/go/testdata/testvendor/src/p/p.go
new file mode 100644
index 0000000000000..e740715186ebf
--- /dev/null
+++ b/src/cmd/go/testdata/testvendor/src/p/p.go
@@ -0,0 +1,6 @@
+package p
+
+import (
+	_ "q/y"
+	_ "q/z"
+)
diff --git a/src/cmd/go/testdata/testvendor/src/q/vendor/x/x.go b/src/cmd/go/testdata/testvendor/src/q/vendor/x/x.go
new file mode 100644
index 0000000000000..823aafd0712b6
--- /dev/null
+++ b/src/cmd/go/testdata/testvendor/src/q/vendor/x/x.go
@@ -0,0 +1 @@
+package x
diff --git a/src/cmd/go/testdata/testvendor/src/q/y/y.go b/src/cmd/go/testdata/testvendor/src/q/y/y.go
new file mode 100644
index 0000000000000..4f842237675a4
--- /dev/null
+++ b/src/cmd/go/testdata/testvendor/src/q/y/y.go
@@ -0,0 +1,3 @@
+package y
+
+import _ "x"
diff --git a/src/cmd/go/testdata/testvendor/src/q/z/z.go b/src/cmd/go/testdata/testvendor/src/q/z/z.go
new file mode 100644
index 0000000000000..a8d4924936a7a
--- /dev/null
+++ b/src/cmd/go/testdata/testvendor/src/q/z/z.go
@@ -0,0 +1,3 @@
+package z
+
+import _ "q/vendor/x"
diff --git a/src/cmd/go/vendor_test.go b/src/cmd/go/vendor_test.go
index ac32545b3b0f9..3b27bdec0e423 100644
--- a/src/cmd/go/vendor_test.go
+++ b/src/cmd/go/vendor_test.go
@@ -186,3 +186,12 @@ func TestVendorGetUpdate(t *testing.T) {
 	tg.run("get", "github.com/rsc/go-get-issue-11864")
 	tg.run("get", "-u", "github.com/rsc/go-get-issue-11864")
 }
+
+func TestVendorCache(t *testing.T) {
+	tg := testgo(t)
+	defer tg.cleanup()
+	tg.setenv("GOPATH", filepath.Join(tg.pwd(), "testdata/testvendor"))
+	tg.setenv("GO15VENDOREXPERIMENT", "1")
+	tg.runFail("build", "p")
+	tg.grepStderr("must be imported as x", "did not fail to build p")
+}