cmd/go/internal/work: disallow testgo binary from installing to GOROOT

Installing to GOROOT makes tests non-parallelizable, since each test
depends on the installed contents of GOROOT already being up-to-date
and may reasonably assume that those contents do not change over the
course of the test.

Fixes #37573
Updates #30316

Change-Id: I2afe95ad11347bee3bb7c2d77a657db6d691cf05
Reviewed-on: https://go-review.googlesource.com/c/go/+/225897
Reviewed-by: Michael Matloob <matloob@golang.org>

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 641cab8ddd0e538075dd4760cf0657fe041e0eae..39e387b9e4b4faf1a7b7b5899f56efea8050fda2 100644 (file)
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -199,6 +199,7 @@ func TestMain(m *testing.M) {
                        return strings.TrimSpace(string(out))
                }
                testGOROOT = goEnv("GOROOT")
+               os.Setenv("TESTGO_GOROOT", testGOROOT)
 
                // The whole GOROOT/pkg tree was installed using the GOHOSTOS/GOHOSTARCH
                // toolchain (installed in GOROOT/pkg/tool/GOHOSTOS_GOHOSTARCH).

diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index d781ad23067ed07e60399aac926b3dfd87ae1938..dbe31a60164db1a4d451e94918d38401afdd6f1a 100644 (file)
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -8,11 +8,6 @@ package work
 
 import (
        "bytes"
-       "cmd/go/internal/base"
-       "cmd/go/internal/cache"
-       "cmd/go/internal/cfg"
-       "cmd/go/internal/load"
-       "cmd/go/internal/str"
        "encoding/json"
        "errors"
        "fmt"
@@ -30,6 +25,12 @@ import (
        "strings"
        "sync"
        "time"
+
+       "cmd/go/internal/base"
+       "cmd/go/internal/cache"
+       "cmd/go/internal/cfg"
+       "cmd/go/internal/load"
+       "cmd/go/internal/str"
 )
 
 // actionList returns the list of actions in the dag rooted at root
@@ -490,6 +491,10 @@ func (b *Builder) build(a *Action) (err error) {
                return nil
        }
 
+       if err := allowInstall(a); err != nil {
+               return err
+       }
+
        // make target directory
        dir, _ := filepath.Split(a.Target)
        if dir != "" {
@@ -1192,6 +1197,10 @@ func (b *Builder) link(a *Action) (err error) {
                return err
        }
 
+       if err := allowInstall(a); err != nil {
+               return err
+       }
+
        // make target directory
        dir, _ := filepath.Split(a.Target)
        if dir != "" {
@@ -1366,6 +1375,10 @@ func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string,
 }
 
 func (b *Builder) installShlibname(a *Action) error {
+       if err := allowInstall(a); err != nil {
+               return err
+       }
+
        // TODO: BuildN
        a1 := a.Deps[0]
        err := ioutil.WriteFile(a.Target, []byte(filepath.Base(a1.Target)+"\n"), 0666)
@@ -1416,6 +1429,10 @@ func (b *Builder) linkShared(a *Action) (err error) {
        }
        defer b.flushOutput(a)
 
+       if err := allowInstall(a); err != nil {
+               return err
+       }
+
        if err := b.Mkdir(a.Objdir); err != nil {
                return err
        }
@@ -1481,8 +1498,12 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) {
                // advertise it by touching the mtimes (usually the libraries are up
                // to date).
                if !a.buggyInstall && !b.IsCmdList {
-                       now := time.Now()
-                       os.Chtimes(a.Target, now, now)
+                       if cfg.BuildN {
+                               b.Showcmd("", "touch %s", a.Target)
+                       } else if err := allowInstall(a); err == nil {
+                               now := time.Now()
+                               os.Chtimes(a.Target, now, now)
+                       }
                }
                return nil
        }
@@ -1493,6 +1514,9 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) {
                a.built = a1.built
                return nil
        }
+       if err := allowInstall(a); err != nil {
+               return err
+       }
 
        if err := b.Mkdir(a.Objdir); err != nil {
                return err
@@ -1522,6 +1546,13 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) {
        return b.moveOrCopyFile(a.Target, a1.built, perm, false)
 }
 
+// allowInstall returns a non-nil error if this invocation of the go command is
+// allowed to install a.Target.
+//
+// (The build of cmd/go running under its own test is forbidden from installing
+// to its original GOROOT.)
+var allowInstall = func(*Action) error { return nil }
+
 // cleanup removes a's object dir to keep the amount of
 // on-disk garbage down in a large build. On an operating system
 // with aggressive buffering, cleaning incrementally like
@@ -1685,6 +1716,10 @@ func (b *Builder) installHeader(a *Action) error {
                return nil
        }
 
+       if err := allowInstall(a); err != nil {
+               return err
+       }
+
        dir, _ := filepath.Split(a.Target)
        if dir != "" {
                if err := b.Mkdir(dir); err != nil {

diff --git a/src/cmd/go/internal/work/testgo.go b/src/cmd/go/internal/work/testgo.go
index 3e623c662100dc6101ceb51ebdcda5473f9daa19..931f49a0691d278b490dad919b40e0e1e7e0716b 100644 (file)
--- a/src/cmd/go/internal/work/testgo.go
+++ b/src/cmd/go/internal/work/testgo.go
@@ -8,10 +8,41 @@
 
 package work
 
-import "os"
+import (
+       "cmd/go/internal/cfg"
+       "cmd/go/internal/search"
+       "fmt"
+       "os"
+       "path/filepath"
+       "runtime"
+)
 
 func init() {
        if v := os.Getenv("TESTGO_VERSION"); v != "" {
                runtimeVersion = v
        }
+
+       if testGOROOT := os.Getenv("TESTGO_GOROOT"); testGOROOT != "" {
+               // Disallow installs to the GOROOT from which testgo was built.
+               // Installs to other GOROOTs — such as one set explicitly within a test — are ok.
+               allowInstall = func(a *Action) error {
+                       if cfg.BuildN {
+                               return nil
+                       }
+
+                       rel := search.InDir(a.Target, testGOROOT)
+                       if rel == "" {
+                               return nil
+                       }
+
+                       callerPos := ""
+                       if _, file, line, ok := runtime.Caller(1); ok {
+                               if shortFile := search.InDir(file, filepath.Join(testGOROOT, "src")); shortFile != "" {
+                                       file = shortFile
+                               }
+                               callerPos = fmt.Sprintf("%s:%d: ", file, line)
+                       }
+                       return fmt.Errorf("%stestgo must not write to GOROOT (installing to %s)", callerPos, filepath.Join("GOROOT", rel))
+               }
+       }
 }

diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go
index 87afb6aec8b419d7e3ded88f0418415c9a023a09..ebadce867b3ca23b3bdbe7f90a4acdb9f27843cd 100644 (file)
--- a/src/cmd/go/script_test.go
+++ b/src/cmd/go/script_test.go
@@ -126,6 +126,7 @@ func (ts *testScript) setup() {
                "GOPROXY=" + proxyURL,
                "GOPRIVATE=",
                "GOROOT=" + testGOROOT,
+               "TESTGO_GOROOT=" + testGOROOT,
                "GOSUMDB=" + testSumDBVerifierKey,
                "GONOPROXY=",
                "GONOSUMDB=",

diff --git a/src/cmd/go/testdata/script/README b/src/cmd/go/testdata/script/README
index f4c92e65ab8c5efc0a3e2cc675e8413f4597b003..e22ddcaf2e8eb6c0dcb2407cbc904ca740c57a26 100644 (file)
--- a/src/cmd/go/testdata/script/README
+++ b/src/cmd/go/testdata/script/README
@@ -34,6 +34,7 @@ Scripts also have access to these other environment variables:
        GOPATH=$WORK/gopath
        GOPROXY=<local module proxy serving from cmd/go/testdata/mod>
        GOROOT=<actual GOROOT>
+       TESTGO_GOROOT=<GOROOT used to build cmd/go, for use in tests that may change GOROOT>
        HOME=/no-home
        PATH=<actual PATH>
        TMPDIR=$WORK/tmp

diff --git a/src/cmd/go/testdata/script/get_update_all.txt b/src/cmd/go/testdata/script/get_update_all.txt
index 1f2f5bf1abe3286f69d242158ef13e06f7f0404c..d0b9860ade0f9ad576576ec64706cc1b7879e21c 100644 (file)
--- a/src/cmd/go/testdata/script/get_update_all.txt
+++ b/src/cmd/go/testdata/script/get_update_all.txt
@@ -3,5 +3,5 @@
 
 [!net] skip
 
-go get -u .../
-! stderr 'duplicate loads of' # make sure old packages are removed from cache
\ No newline at end of file
+go get -u -n .../
+! stderr 'duplicate loads of' # make sure old packages are removed from cache