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>

diff --git a/src/cmd/go/pkg.go b/src/cmd/go/pkg.go
index a2c5ba7e5e7c64695a2d051ba8b2f5e5688ef1b4..0b61b0eeb4cb51d775351c88766b3e31f138dad1 100644 (file)
--- 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 (file)
index 0000000..e740715
--- /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 (file)
index 0000000..823aafd
--- /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 (file)
index 0000000..4f84223
--- /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 (file)
index 0000000..a8d4924
--- /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 ac32545b3b0f9b8d4843c2f7d020162618b572ec..3b27bdec0e423f1bd2c46c70baf01e6c6cda7374 100644 (file)
--- 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")
+}