cmd/go: always rebuild GOPATH code that looks out of date

We used to put a rebuilding barrier between GOPATHs, so that if
you had GOPATH=dir1:dir2 and you had "p" in dir1/src/p
and "q" in dir2/src/q, with "p" importing "q", then when you
ran 'go install p', it would see that it was working in dir1
and (since nothing from dir2 was explicitly mentioned)
would assume that everything in dir2 is up-to-date, provided
it is built at all.

This has the confusing behavior that if "q" hasn't been built ever,
then if you update sources in q and run 'go install p', the right
thing happens (q is rebuilt and then p), but after that, if you update
sources in q and run 'go install p', nothing happens: the installed
q is assumed up-to-date.

People using code conventions with multiple GOPATH entries
(for example, with commands in one place and libraries in another,
or vendoring conventions that try to avoid rewriting import paths)
run into this without realizing it and end up with incorrect build
results.

The original motivation here was to avoid rebuild standard packages
since a system-installed GOROOT might be unwritable.
The change introduced to separate GOROOT also separated
individual GOPATH entries. Later changes added a different, more
aggressive earlier shortcut for GOROOT in release settings,
so the code here is now only applying to (and confusing)
multiple GOPATH entries. Remove it.

Fixes #10509.

Change-Id: I687a3baa81eff4073b0d67f9acbc5a3ab192eda5
Reviewed-on: https://go-review.googlesource.com/9155Reviewed-by: Ian Lance Taylor <iant@golang.org>

src/cmd/go/pkg.go

@@ -759,13 +759,8 @@ func packageList(roots []*Package) []*Package {
 // computeStale computes the Stale flag in the package dag that starts
 // at the named pkgs (command-line arguments).
 func computeStale(pkgs ...*Package) {
-	topRoot := map[string]bool{}
-	for _, p := range pkgs {
-		topRoot[p.Root] = true
-	}
-
 	for _, p := range packageList(pkgs) {
-		p.Stale = isStale(p, topRoot)
+		p.Stale = isStale(p)
 	}
 }
 
@@ -776,7 +771,7 @@ func computeStale(pkgs ...*Package) {
 var isGoRelease = strings.HasPrefix(runtime.Version(), "go1")
 
 // isStale reports whether package p needs to be rebuilt.
-func isStale(p *Package, topRoot map[string]bool) bool {
+func isStale(p *Package) bool {
 	if p.Standard && (p.ImportPath == "unsafe" || buildContext.Compiler == "gccgo") {
 		// fake, builtin package
 		return false
@@ -795,16 +790,8 @@ func isStale(p *Package, topRoot map[string]bool) bool {
 		return false
 	}
 
-	// If we are running a release copy of Go, do not rebuild the standard packages.
-	// They may not be writable anyway, but they are certainly not changing.
-	// This makes 'go build -a' skip the standard packages when using an official release.
-	// See issue 4106 and issue 8290.
-	pkgBuildA := buildA
-	if p.Standard && isGoRelease {
-		pkgBuildA = false
-	}
-
-	if pkgBuildA || p.target == "" || p.Stale {
+	// If there's no install target or it's already marked stale, we have to rebuild.
+	if p.target == "" || p.Stale {
 		return true
 	}
 
@@ -817,6 +804,22 @@ func isStale(p *Package, topRoot map[string]bool) bool {
 		return true
 	}
 
+	// If we are running a release copy of Go, do not rebuild the standard packages.
+	// They may not be writable anyway, but they are certainly not changing.
+	// This makes 'go build' and 'go build -a' skip the standard packages when
+	// using an official release. See issue 3036, issue 3149, issue 4106, issue 8290.
+	// (If a change to a release tree must be made by hand, the way to force the
+	// install is to run make.bash, which will remove the old package archives
+	// before rebuilding.)
+	if p.Standard && isGoRelease {
+		return false
+	}
+
+	// If the -a flag is given, rebuild everything (except standard packages; see above).
+	if buildA {
+		return true
+	}
+
 	olderThan := func(file string) bool {
 		fi, err := os.Stat(file)
 		return err != nil || fi.ModTime().After(built)
@@ -844,8 +847,12 @@ func isStale(p *Package, topRoot map[string]bool) bool {
 	// back-dated, as some binary distributions may do, but it does handle
 	// a very common case.
 	// See issue 3036.
-	// Assume code in $GOROOT is up to date, since it may not be writeable.
-	// See issue 4106.
+	// Exclude $GOROOT, under the assumption that people working on
+	// the compiler may want to control when everything gets rebuilt,
+	// and people updating the Go repository will run make.bash or all.bash
+	// and get a full rebuild anyway.
+	// Excluding $GOROOT used to also fix issue 4106, but that's now
+	// taken care of above (at least when the installed Go is a released version).
 	if p.Root != goroot {
 		if olderThan(buildToolchain.compiler()) {
 			return true
@@ -855,20 +862,43 @@ func isStale(p *Package, topRoot map[string]bool) bool {
 		}
 	}
 
-	// Have installed copy, probably built using current compilers,
-	// built with the right set of source files,
-	// and built after its imported packages. The only reason now
-	// that we'd have to rebuild it is if the sources were newer than
-	// the package. If a package p is not in the same tree as any
-	// package named on the command-line, assume it is up-to-date
-	// no matter what the modification times on the source files indicate.
-	// This avoids rebuilding $GOROOT packages when people are
-	// working outside the Go root, and it effectively makes each tree
-	// listed in $GOPATH a separate compilation world.
-	// See issue 3149.
-	if p.Root != "" && !topRoot[p.Root] {
-		return false
-	}
+	// Note: Until Go 1.5, we had an additional shortcut here.
+	// We built a list of the workspace roots ($GOROOT, each $GOPATH)
+	// containing targets directly named on the command line,
+	// and if p were not in any of those, it would be treated as up-to-date
+	// as long as it is built. The goal was to avoid rebuilding a system-installed
+	// $GOROOT, unless something from $GOROOT were explicitly named
+	// on the command line (like go install math).
+	// That's now handled by the isGoRelease clause above.
+	// The other effect of the shortcut was to isolate different entries in
+	// $GOPATH from each other. This had the unfortunate effect that
+	// if you had (say), GOPATH listing two entries, one for commands
+	// and one for libraries, and you did a 'git pull' in the library one
+	// and then tried 'go install commands/...', it would build the new libraries
+	// during the first build (because they wouldn't have been installed at all)
+	// but then subsequent builds would not rebuild the libraries, even if the
+	// mtimes indicate they are stale, because the different GOPATH entries
+	// were treated differently. This behavior was confusing when using
+	// non-trivial GOPATHs, which were particularly common with some
+	// code management conventions, like the original godep.
+	// Since the $GOROOT case (the original motivation) is handled separately,
+	// we no longer put a barrier between the different $GOPATH entries.
+	//
+	// One implication of this is that if there is a system directory for
+	// non-standard Go packages that is included in $GOPATH, the mtimes
+	// on those compiled packages must be no earlier than the mtimes
+	// on the source files. Since most distributions use the same mtime
+	// for all files in a tree, they will be unaffected. People using plain
+	// tar x to extract system-installed packages will need to adjust mtimes,
+	// but it's better to force them to get the mtimes right than to ignore
+	// the mtimes and thereby do the wrong thing in common use cases.
+	//
+	// So there is no GOPATH vs GOPATH shortcut here anymore.
+	//
+	// If something needs to come back here, we could try writing a dummy
+	// file with a random name to the $GOPATH/pkg directory (and removing it)
+	// to test for write access, and then skip GOPATH roots we don't have write
+	// access to. But hopefully we can just use the mtimes always.
 
 	srcs := stringList(p.GoFiles, p.CFiles, p.CXXFiles, p.MFiles, p.HFiles, p.SFiles, p.CgoFiles, p.SysoFiles, p.SwigFiles, p.SwigCXXFiles)
 	for _, src := range srcs {

src/cmd/go/test.bash

@@ -96,6 +96,55 @@ elif grep -q runtime $d/err.out; then
 fi
 rm -r $d
 
+TEST 'go install rebuilds stale packages in other GOPATH'
+d=$(TMPDIR=/var/tmp mktemp -d -t testgoXXX)
+export GOPATH=$d/d1:$d/d2
+mkdir -p $d/d1/src/p1 $d/d2/src/p2
+echo 'package p1
+
+import "p2"
+
+func F() { p2.F() }
+' > $d/d1/src/p1/p1.go
+echo 'package p2
+
+func F() {}
+' > $d/d2/src/p2/p2.go
+if ! ./testgo install p1; then
+	echo "./testgo install p1 failed"
+	ok=false
+elif [ "$(./testgo list -f '{{.Stale}}' p1)" != false ]; then
+	echo "./testgo list mypkg claims p1 is stale, incorrectly"
+	ok=false
+elif [ "$(./testgo list -f '{{.Stale}}' p2)" != false ]; then
+	echo "./testgo list mypkg claims p2 is stale, incorrectly"
+	ok=false
+else
+	sleep 1
+	echo 'func G() {}' >>$d/d2/src/p2/p2.go
+	if [ "$(./testgo list -f '{{.Stale}}' p2)" != true ]; then
+		echo "./testgo list mypkg claims p2 is NOT stale, incorrectly"
+		ok=false
+	elif [ "$(./testgo list -f '{{.Stale}}' p1)" != true ]; then
+		echo "./testgo list mypkg claims p1 is NOT stale, incorrectly"
+		ok=false
+	fi
+	
+	if ! ./testgo install p1; then
+		echo "./testgo install p1 failed second time"
+		ok=false
+	else
+		if [ "$(./testgo list -f '{{.Stale}}' p2)" != false ]; then
+			echo "./testgo list mypkg claims p2 is stale after reinstall, incorrectly"
+			ok=false
+		elif [ "$(./testgo list -f '{{.Stale}}' p1)" != false ]; then
+			echo "./testgo list mypkg claims p1 is stale after reinstall, incorrectly"
+			ok=false
+		fi
+	fi		
+fi
+rm -r $d
+
 TEST 'go install detects removed files'
 d=$(TMPDIR=/var/tmp mktemp -d -t testgoXXX)
 export GOPATH=$d
@@ -106,7 +155,7 @@ echo '// +build missingtag
 
 package mypkg' >$d/src/mypkg/z.go
 if ! ./testgo install mypkg; then
-	echo "testgo install mypkg failed"
+	echo "./testgo install mypkg failed"
 	ok=false
 elif [ "$(./testgo list -f '{{.Stale}}' mypkg)" != false ]; then
 	echo "./testgo list mypkg claims mypkg is stale, incorrectly"