Skip to content

Commit

Permalink
cmd/go: limit test input file change detection to local GOROOT/GOPATH…
Browse files Browse the repository at this point in the history
… tree

We've had a series of problems with tests unexpectedly (and innocently)
looking at system files that appear to (but don't) change in meaningful ways,
like /dev/null on OS X having a modification time set to the current time.

Cut all these off by only applying file change detection to the local package
root: the GOROOT or specific sub-GOPATH in which the package being tested
is found.

(This means that if you test reads /tmp/x and you change /tmp/x, the cached
result will still be used. Don't do that, or else use -count=1.)

Fixes #23390.

Change-Id: I30b6dd194835deb645a040aea5e6e4f68af09edb
Reviewed-on: https://go-review.googlesource.com/87015
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
  • Loading branch information
rsc committed Jan 10, 2018
1 parent c203696 commit 37d5627
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 28 deletions.
24 changes: 24 additions & 0 deletions src/cmd/go/go_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5307,6 +5307,30 @@ func TestTestCacheInputs(t *testing.T) {
tg.run("test", "testcache", "-run=DirList")
tg.grepStdout(`\(cached\)`, "did not cache")

tg.tempFile("file.txt", "")
tg.must(ioutil.WriteFile(filepath.Join(tg.pwd(), "testdata/src/testcache/testcachetmp_test.go"), []byte(`package testcache
import (
"os"
"testing"
)
func TestExternalFile(t *testing.T) {
os.Open(`+fmt.Sprintf("%q", tg.path("file.txt"))+`)
_, err := os.Stat(`+fmt.Sprintf("%q", tg.path("file.txt"))+`)
if err != nil {
t.Fatal(err)
}
}
`), 0666))
defer os.Remove(filepath.Join(tg.pwd(), "testdata/src/testcache/testcachetmp_test.go"))
tg.run("test", "testcache", "-run=ExternalFile")
tg.run("test", "testcache", "-run=ExternalFile")
tg.grepStdout(`\(cached\)`, "did not cache")
tg.must(os.Remove(filepath.Join(tg.tempdir, "file.txt")))
tg.run("test", "testcache", "-run=ExternalFile")
tg.grepStdout(`\(cached\)`, "did not cache")

switch runtime.GOOS {
case "nacl", "plan9", "windows":
// no shell scripts
Expand Down
22 changes: 0 additions & 22 deletions src/cmd/go/internal/load/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,25 +56,3 @@ func expandPath(p string) string {
}
return p
}

// hasFilePathPrefix reports whether the filesystem path s begins with the
// elements in prefix.
func hasFilePathPrefix(s, prefix string) bool {
sv := strings.ToUpper(filepath.VolumeName(s))
pv := strings.ToUpper(filepath.VolumeName(prefix))
s = s[len(sv):]
prefix = prefix[len(pv):]
switch {
default:
return false
case sv != pv:
return false
case len(s) == len(prefix):
return s == prefix
case len(s) > len(prefix):
if prefix != "" && prefix[len(prefix)-1] == filepath.Separator {
return strings.HasPrefix(s, prefix)
}
return s[len(prefix)] == filepath.Separator && s[:len(prefix)] == prefix
}
}
12 changes: 6 additions & 6 deletions src/cmd/go/internal/load/pkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -520,13 +520,13 @@ func VendoredImportPath(parent *Package, path string) (found string) {

dir := filepath.Clean(parent.Dir)
root := filepath.Join(parent.Root, "src")
if !hasFilePathPrefix(dir, root) || parent.ImportPath != "command-line-arguments" && filepath.Join(root, parent.ImportPath) != dir {
if !str.HasFilePathPrefix(dir, root) || parent.ImportPath != "command-line-arguments" && filepath.Join(root, parent.ImportPath) != dir {
// Look for symlinks before reporting error.
dir = expandPath(dir)
root = expandPath(root)
}

if !hasFilePathPrefix(dir, root) || len(dir) <= len(root) || dir[len(root)] != filepath.Separator || parent.ImportPath != "command-line-arguments" && !parent.Internal.Local && filepath.Join(root, parent.ImportPath) != dir {
if !str.HasFilePathPrefix(dir, root) || len(dir) <= len(root) || dir[len(root)] != filepath.Separator || parent.ImportPath != "command-line-arguments" && !parent.Internal.Local && filepath.Join(root, parent.ImportPath) != dir {
base.Fatalf("unexpected directory layout:\n"+
" import path: %s\n"+
" root: %s\n"+
Expand Down Expand Up @@ -670,14 +670,14 @@ func disallowInternal(srcDir string, p *Package, stk *ImportStack) *Package {
i-- // rewind over slash in ".../internal"
}
parent := p.Dir[:i+len(p.Dir)-len(p.ImportPath)]
if hasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
if str.HasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
return p
}

// Look for symlinks before reporting error.
srcDir = expandPath(srcDir)
parent = expandPath(parent)
if hasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
if str.HasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
return p
}

Expand Down Expand Up @@ -770,14 +770,14 @@ func disallowVendorVisibility(srcDir string, p *Package, stk *ImportStack) *Pack
return p
}
parent := p.Dir[:truncateTo]
if hasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
if str.HasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
return p
}

// Look for symlinks before reporting error.
srcDir = expandPath(srcDir)
parent = expandPath(parent)
if hasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
if str.HasFilePathPrefix(filepath.Clean(srcDir), filepath.Clean(parent)) {
return p
}

Expand Down
32 changes: 32 additions & 0 deletions src/cmd/go/internal/str/path.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2018 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package str

import (
"path/filepath"
"strings"
)

// HasFilePathPrefix reports whether the filesystem path s begins with the
// elements in prefix.
func HasFilePathPrefix(s, prefix string) bool {
sv := strings.ToUpper(filepath.VolumeName(s))
pv := strings.ToUpper(filepath.VolumeName(prefix))
s = s[len(sv):]
prefix = prefix[len(pv):]
switch {
default:
return false
case sv != pv:
return false
case len(s) == len(prefix):
return s == prefix
case len(s) > len(prefix):
if prefix != "" && prefix[len(prefix)-1] == filepath.Separator {
return strings.HasPrefix(s, prefix)
}
return s[len(prefix)] == filepath.Separator && s[:len(prefix)] == prefix
}
}
20 changes: 20 additions & 0 deletions src/cmd/go/internal/test/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1637,11 +1637,19 @@ func computeTestInputsID(a *work.Action, testlog []byte) (cache.ActionID, error)
if !filepath.IsAbs(name) {
name = filepath.Join(pwd, name)
}
if !inDir(name, a.Package.Root) {
// Do not recheck files outside the GOPATH or GOROOT root.
break
}
fmt.Fprintf(h, "stat %s %x\n", name, hashStat(name))
case "open":
if !filepath.IsAbs(name) {
name = filepath.Join(pwd, name)
}
if !inDir(name, a.Package.Root) {
// Do not recheck files outside the GOPATH or GOROOT root.
break
}
fh, err := hashOpen(name)
if err != nil {
if cache.DebugTest {
Expand All @@ -1656,6 +1664,18 @@ func computeTestInputsID(a *work.Action, testlog []byte) (cache.ActionID, error)
return sum, nil
}

func inDir(path, dir string) bool {
if str.HasFilePathPrefix(path, dir) {
return true
}
xpath, err1 := filepath.EvalSymlinks(path)
xdir, err2 := filepath.EvalSymlinks(dir)
if err1 == nil && err2 == nil && str.HasFilePathPrefix(xpath, xdir) {
return true
}
return false
}

func hashGetenv(name string) cache.ActionID {
h := cache.NewHash("getenv")
v, ok := os.LookupEnv(name)
Expand Down

0 comments on commit 37d5627

Please sign in to comment.