cmd/go: use GOPATH order for compile -I and link -L options

Given GOPATH=p1:p2 and source code of just the right form,
the go command could previously end up invoking the compiler
with -I p2 -I p1 or the linker with -L p2 -L p1, so that
compiled packages in p2 incorrectly shadowed packages in p1.
If foo were in both p1 and p2 and the compilation of bar
were such that the -I and -L options were inverted in this way,
then

	GOPATH=p2 go install foo
	GOPATH=p1:p2 go install bar

would get the p2 copy of foo instead of the (expected) p1 copy of foo.

This manifested in real usage in a few different ways, but in all
the root cause was that the -I or -L option sequence did not
match GOPATH.

Make it match GOPATH.

Fixes #14176 (second report).
Fixes #14192.
Related but less common issue #14271 not fixed.

Change-Id: I9c0f69042bb2bf92c9fc370535da2c60a1187d30
Reviewed-on: https://go-review.googlesource.com/19385
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>

2 files changed, 71 insertions, 0 deletions

diff --git a/src/cmd/go/build.go b/src/cmd/go/build.go
index a1f925ed0b..f2a2a6014f 100644
--- a/src/cmd/go/build.go
+++ b/src/cmd/go/build.go
@@ -667,6 +667,7 @@ var (
 	goarch    string
 	goos      string
 	exeSuffix string
+	gopath    []string
 )
 
 func init() {
@@ -675,6 +676,7 @@ func init() {
 	if goos == "windows" {
 		exeSuffix = ".exe"
 	}
+	gopath = filepath.SplitList(buildContext.GOPATH)
 }
 
 // A builder holds global state about a build.
@@ -1684,6 +1686,22 @@ func (b *builder) includeArgs(flag string, all []*action) []string {
 	inc = append(inc, flag, b.work)
 
 	// Finally, look in the installed package directories for each action.
+	// First add the package dirs corresponding to GOPATH entries
+	// in the original GOPATH order.
+	need := map[string]*build.Package{}
+	for _, a1 := range all {
+		if a1.p != nil && a1.pkgdir == a1.p.build.PkgRoot {
+			need[a1.p.build.Root] = a1.p.build
+		}
+	}
+	for _, root := range gopath {
+		if p := need[root]; p != nil && !incMap[p.PkgRoot] {
+			incMap[p.PkgRoot] = true
+			inc = append(inc, flag, p.PkgTargetRoot)
+		}
+	}
+
+	// Then add anything that's left.
 	for _, a1 := range all {
 		if a1.p == nil {
 			continue
diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 6d12f75073..c60971efed 100644
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -2565,6 +2565,59 @@ func TestGoInstallShadowedGOPATH(t *testing.T) {
 	tg.grepStderr("no install location for.*gopath2.src.test: hidden by .*gopath1.src.test", "missing error")
 }
 
+func TestGoBuildGOPATHOrder(t *testing.T) {
+	// golang.org/issue/14176#issuecomment-179895769
+	// golang.org/issue/14192
+	// -I arguments to compiler could end up not in GOPATH order,
+	// leading to unexpected import resolution in the compiler.
+	// This is still not a complete fix (see golang.org/issue/14271 and next test)
+	// but it is clearly OK and enough to fix both of the two reported
+	// instances of the underlying problem. It will have to do for now.
+
+	tg := testgo(t)
+	defer tg.cleanup()
+	tg.makeTempdir()
+	tg.setenv("GOPATH", tg.path("p1")+string(filepath.ListSeparator)+tg.path("p2"))
+
+	tg.tempFile("p1/src/foo/foo.go", "package foo\n")
+	tg.tempFile("p2/src/baz/baz.go", "package baz\n")
+	tg.tempFile("p2/pkg/"+runtime.GOOS+"_"+runtime.GOARCH+"/foo.a", "bad\n")
+	tg.tempFile("p1/src/bar/bar.go", `
+		package bar
+		import _ "baz"
+		import _ "foo"
+	`)
+
+	tg.run("install", "-x", "bar")
+}
+
+func TestGoBuildGOPATHOrderBroken(t *testing.T) {
+	// This test is known not to work.
+	// See golang.org/issue/14271.
+	t.Skip("golang.org/issue/14271")
+
+	tg := testgo(t)
+	defer tg.cleanup()
+	tg.makeTempdir()
+
+	tg.tempFile("p1/src/foo/foo.go", "package foo\n")
+	tg.tempFile("p2/src/baz/baz.go", "package baz\n")
+	tg.tempFile("p1/pkg/"+runtime.GOOS+"_"+runtime.GOARCH+"/baz.a", "bad\n")
+	tg.tempFile("p2/pkg/"+runtime.GOOS+"_"+runtime.GOARCH+"/foo.a", "bad\n")
+	tg.tempFile("p1/src/bar/bar.go", `
+		package bar
+		import _ "baz"
+		import _ "foo"
+	`)
+
+	colon := string(filepath.ListSeparator)
+	tg.setenv("GOPATH", tg.path("p1")+colon+tg.path("p2"))
+	tg.run("install", "-x", "bar")
+
+	tg.setenv("GOPATH", tg.path("p2")+colon+tg.path("p1"))
+	tg.run("install", "-x", "bar")
+}
+
 func TestIssue11709(t *testing.T) {
 	tg := testgo(t)
 	defer tg.cleanup()