Skip to content

Commit

Permalink
cmd/go: fix disallow of p/vendor/x during vendor experiment
Browse files Browse the repository at this point in the history
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 golang#1.
Since golang#2 can't happen, tested golang#2 by hand after reverting fix for golang#1.

Fixes golang#11913.

Change-Id: I6d2fc65f93b8fb5f3b263ace8d5f68d803a2ae5c
Reviewed-on: https://go-review.googlesource.com/13022
Reviewed-by: Ian Lance Taylor <[email protected]>
  • Loading branch information
rsc committed Jul 31, 2015
1 parent c5dff72 commit 45971c6
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 5 deletions.
26 changes: 21 additions & 5 deletions src/cmd/go/pkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}

Expand Down
6 changes: 6 additions & 0 deletions src/cmd/go/testdata/testvendor/src/p/p.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package p

import (
_ "q/y"
_ "q/z"
)
1 change: 1 addition & 0 deletions src/cmd/go/testdata/testvendor/src/q/vendor/x/x.go
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package x
3 changes: 3 additions & 0 deletions src/cmd/go/testdata/testvendor/src/q/y/y.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package y

import _ "x"
3 changes: 3 additions & 0 deletions src/cmd/go/testdata/testvendor/src/q/z/z.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package z

import _ "q/vendor/x"
9 changes: 9 additions & 0 deletions src/cmd/go/vendor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

0 comments on commit 45971c6

Please sign in to comment.