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>

diff --git a/src/cmd/go/build.go b/src/cmd/go/build.go
index a1f925ed0ba2d7b8c26410150a78bd299c56edb8..f2a2a6014f2b134ce2375846663818e7f956e56f 100644 (file)
--- 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 6d12f7507396540fb0f4ad77cf4c382f04645c69..c60971efed3f5ba2b74aa9747a9ac9164a72a562 100644 (file)
--- 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()