Skip to content

Commit

Permalink
cmd/go: always rebuild GOPATH code that looks out of date
Browse files Browse the repository at this point in the history 
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/9155
Reviewed-by: Ian Lance Taylor <iant@golang.org>
  • Loading branch information
@rsc
rsc committed Jun 4, 2015
1 parent 7b87631 commit 119daba
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 34 deletions.
96 changes: 63 additions & 33 deletions src/cmd/go/pkg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand All @@ -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
Expand All @@ -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
}

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
51 changes: 50 additions & 1 deletion src/cmd/go/test.bash
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand Down

0 comments on commit 119daba

Please sign in to comment.