From: Bryan C. Mills Date: Tue, 2 May 2023 17:07:26 +0000 (-0400) Subject: internal/testenv: remove HasExec and simplify other support checks again X-Git-Tag: go1.21rc1~677 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=79723f389b630e61c5e9635235b7f4c624ac1379;p=gostls13.git internal/testenv: remove HasExec and simplify other support checks again HasExec is an attractive nuisance: it is tempting to check in a TestMain function, but TestMain really shouldn't be running subprocesses eagerly anyway (they add needless overhead for operations like 'go test -list=.'), and the trick of re-executing the test binary to determine whether 'exec' works ends up in infinite recursion if TestMain itself calls HasExec. Instead, tests that need to execute a subprocess should call MustHaveExec or MustHaveExecPath from within a specific test, or just try to exec the program and check its error status (perhaps using testenv.SyscallIsNotSupported). While I'm in here and testing on the SlowBots anyway, a few other cleanups relating to subprocesses: - Add more t.Helper calls to support checks where appropriate. - Remove findGoTool, which can be simplified to exec.LookPath as of CL 404134. - Add tests confirming the expected behavior of the support functions on the Go project's builders. Change-Id: I163c701b2dd6eb6b7a036c6848f99b64dd9f0838 Reviewed-on: https://go-review.googlesource.com/c/go/+/491660 Reviewed-by: Ian Lance Taylor TryBot-Result: Gopher Robot Run-TryBot: Bryan Mills Auto-Submit: Bryan Mills --- diff --git a/src/internal/testenv/exec.go b/src/internal/testenv/exec.go index 882791ddca..481be2e649 100644 --- a/src/internal/testenv/exec.go +++ b/src/internal/testenv/exec.go @@ -16,13 +16,25 @@ import ( "time" ) -// HasExec reports whether the current system can start new processes +// MustHaveExec checks that the current system can start new processes // using os.StartProcess or (more commonly) exec.Command. -func HasExec() bool { +// If not, MustHaveExec calls t.Skip with an explanation. +// +// On some platforms MustHaveExec checks for exec support by re-executing the +// current executable, which must be a binary built by 'go test'. +// We intentionally do not provide a HasExec function because of the risk of +// inappropriate recursion in TestMain functions. +// +// To check for exec support outside of a test, just try to exec the command. +// If exec is not supported, testenv.SyscallIsNotSupported will return true +// for the resulting error. +func MustHaveExec(t testing.TB) { tryExecOnce.Do(func() { tryExecOk = tryExec() }) - return tryExecOk + if !tryExecOk { + t.Skipf("skipping test: cannot exec subprocess on %s/%s", runtime.GOOS, runtime.GOARCH) + } } var ( @@ -56,16 +68,8 @@ func init() { return } - // We know that this is a test executable. - // We should be able to run it with a no-op flag and the original test - // execution environment to check for overall exec support. - - // Save the original environment during init for use in the check. A test - // binary may modify its environment before calling HasExec to change its - // behavior// (such as mimicking a command-line tool), and that modified - // environment might cause our self-test to behave unpredictably. - origEnv := os.Environ() - + // We know that this is a test executable. We should be able to run it with a + // no-op flag to check for overall exec support. tryExec = func() bool { exe, err := os.Executable() if err != nil { @@ -80,15 +84,6 @@ func init() { } } -// MustHaveExec checks that the current system can start new processes -// using os.StartProcess or (more commonly) exec.Command. -// If not, MustHaveExec calls t.Skip with an explanation. -func MustHaveExec(t testing.TB) { - if !HasExec() { - t.Skipf("skipping test: cannot exec subprocess on %s/%s", runtime.GOOS, runtime.GOARCH) - } -} - var execPaths sync.Map // path -> error // MustHaveExecPath checks that the current system can start the named executable diff --git a/src/internal/testenv/testenv.go b/src/internal/testenv/testenv.go index d03bb0550a..70606242d6 100644 --- a/src/internal/testenv/testenv.go +++ b/src/internal/testenv/testenv.go @@ -27,6 +27,12 @@ import ( "testing" ) +// Save the original environment during init for use in checks. A test +// binary may modify its environment before calling HasExec to change its +// behavior (such as mimicking a command-line tool), and that modified +// environment might cause environment checks to behave erratically. +var origEnv = os.Environ() + // Builder reports the name of the builder running this test // (for example, "linux-amd64" or "windows-386-gce"). // If the test is not running on the build infrastructure, @@ -46,59 +52,62 @@ func HasGoBuild() bool { return false } - if !HasExec() { - // If we can't exec anything at all, we certainly can't exec 'go build'. - return false - } - - if platform.MustLinkExternal(runtime.GOOS, runtime.GOARCH, false) { - // We can assume that we always have a complete Go toolchain available. - // However, this platform requires a C linker to build even pure Go - // programs, including tests. Do we have one in the test environment? - // (On Android, for example, the device running the test might not have a - // C toolchain installed.) - // - // If CC is set explicitly, assume that we do. Otherwise, use 'go env CC' - // to determine which toolchain it would use by default. - if os.Getenv("CC") == "" { - if _, err := findCC(); err != nil { - return false - } - } - } - - return true -} - -func findCC() (string, error) { - ccOnce.Do(func() { - goTool, err := findGoTool() + goBuildOnce.Do(func() { + // To run 'go build', we need to be able to exec a 'go' command. + // We somewhat arbitrarily choose to exec 'go tool -n compile' because that + // also confirms that cmd/go can find the compiler. (Before CL 472096, + // we sometimes ended up with cmd/go installed in the test environment + // without a cmd/compile it could use to actually build things.) + cmd := exec.Command("go", "tool", "-n", "compile") + cmd.Env = origEnv + out, err := cmd.Output() if err != nil { - ccErr = err + goBuildErr = fmt.Errorf("%v: %w", cmd, err) return } - - cmd := exec.Command(goTool, "env", "CC") - out, err := cmd.Output() out = bytes.TrimSpace(out) - if err != nil { - ccErr = fmt.Errorf("%v: %w", cmd, err) + if len(out) == 0 { + goBuildErr = fmt.Errorf("%v: no tool reported", cmd) return - } else if len(out) == 0 { - ccErr = fmt.Errorf("%v: no CC reported", cmd) + } + if _, err := exec.LookPath(string(out)); err != nil { + goBuildErr = err return } - cc := string(out) - ccPath, ccErr = exec.LookPath(cc) + if platform.MustLinkExternal(runtime.GOOS, runtime.GOARCH, false) { + // We can assume that we always have a complete Go toolchain available. + // However, this platform requires a C linker to build even pure Go + // programs, including tests. Do we have one in the test environment? + // (On Android, for example, the device running the test might not have a + // C toolchain installed.) + // + // If CC is set explicitly, assume that we do. Otherwise, use 'go env CC' + // to determine which toolchain it would use by default. + if os.Getenv("CC") == "" { + cmd := exec.Command("go", "env", "CC") + cmd.Env = origEnv + out, err := cmd.Output() + if err != nil { + goBuildErr = fmt.Errorf("%v: %w", cmd, err) + return + } + out = bytes.TrimSpace(out) + if len(out) == 0 { + goBuildErr = fmt.Errorf("%v: no CC reported", cmd) + return + } + _, goBuildErr = exec.LookPath(string(out)) + } + } }) - return ccPath, ccErr + + return goBuildErr == nil } var ( - ccOnce sync.Once - ccPath string - ccErr error + goBuildOnce sync.Once + goBuildErr error ) // MustHaveGoBuild checks that the current system can build programs with “go build” @@ -106,10 +115,12 @@ var ( // If not, MustHaveGoBuild calls t.Skip with an explanation. func MustHaveGoBuild(t testing.TB) { if os.Getenv("GO_GCFLAGS") != "" { + t.Helper() t.Skipf("skipping test: 'go build' not compatible with setting $GO_GCFLAGS") } if !HasGoBuild() { - t.Skipf("skipping test: 'go build' not available on %s/%s", runtime.GOOS, runtime.GOARCH) + t.Helper() + t.Skipf("skipping test: 'go build' unavailable: %v", goBuildErr) } } @@ -193,6 +204,9 @@ func findGOROOT() (string, error) { // runs the test in the directory containing the packaged under test.) That // means that if we start walking up the tree, we should eventually find // GOROOT/src/go.mod, and we can report the parent directory of that. + // + // Notably, this works even if we can't run 'go env GOROOT' as a + // subprocess. cwd, err := os.Getwd() if err != nil { @@ -243,7 +257,8 @@ func findGOROOT() (string, error) { // GOROOT reports the path to the directory containing the root of the Go // project source tree. This is normally equivalent to runtime.GOROOT, but -// works even if the test binary was built with -trimpath. +// works even if the test binary was built with -trimpath and cannot exec +// 'go env GOROOT'. // // If GOROOT cannot be found, GOROOT skips t if t is non-nil, // or panics otherwise. @@ -264,32 +279,9 @@ func GoTool() (string, error) { if !HasGoBuild() { return "", errors.New("platform cannot run go tool") } - return findGoTool() -} - -func findGoTool() (string, error) { goToolOnce.Do(func() { - goToolPath, goToolErr = func() (string, error) { - var exeSuffix string - if runtime.GOOS == "windows" { - exeSuffix = ".exe" - } - goroot, err := findGOROOT() - if err != nil { - return "", fmt.Errorf("cannot find go tool: %w", err) - } - path := filepath.Join(goroot, "bin", "go"+exeSuffix) - if _, err := os.Stat(path); err == nil { - return path, nil - } - goBin, err := exec.LookPath("go" + exeSuffix) - if err != nil { - return "", errors.New("cannot find go tool: " + err.Error()) - } - return goBin, nil - }() + goToolPath, goToolErr = exec.LookPath("go") }) - return goToolPath, goToolErr } @@ -319,9 +311,11 @@ func HasExternalNetwork() bool { // If not, MustHaveExternalNetwork calls t.Skip with an explanation. func MustHaveExternalNetwork(t testing.TB) { if runtime.GOOS == "js" || runtime.GOOS == "wasip1" { + t.Helper() t.Skipf("skipping test: no external network on %s", runtime.GOOS) } if testing.Short() { + t.Helper() t.Skipf("skipping test: no external network in -short mode") } } @@ -334,6 +328,7 @@ func HasCGO() bool { return } cmd := exec.Command(goTool, "env", "CGO_ENABLED") + cmd.Env = origEnv out, err := cmd.Output() if err != nil { panic(fmt.Sprintf("%v: %v", cmd, out)) diff --git a/src/internal/testenv/testenv_test.go b/src/internal/testenv/testenv_test.go index ebc27f159a..be766288e8 100644 --- a/src/internal/testenv/testenv_test.go +++ b/src/internal/testenv/testenv_test.go @@ -5,10 +5,12 @@ package testenv_test import ( + "internal/platform" "internal/testenv" "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -51,3 +53,105 @@ func TestGoToolLocation(t *testing.T) { t.Fatalf("%q is not the same file as %q", absWant, goTool) } } + +func TestHasGoBuild(t *testing.T) { + if !testenv.HasGoBuild() { + switch runtime.GOOS { + case "js", "wasip1": + // No exec syscall, so these shouldn't be able to 'go build'. + t.Logf("HasGoBuild is false on %s", runtime.GOOS) + return + } + + b := testenv.Builder() + if b == "" { + // We shouldn't make assumptions about what kind of sandbox or build + // environment external Go users may be running in. + t.Skipf("skipping: 'go build' unavailable") + } + + // Since we control the Go builders, we know which ones ought + // to be able to run 'go build'. Check that they can. + // + // (Note that we don't verify that any builders *can't* run 'go build'. + // If a builder starts running 'go build' tests when it shouldn't, + // we will presumably find out about it when those tests fail.) + switch runtime.GOOS { + case "ios": + if strings.HasSuffix(b, "-corellium") { + // The corellium environment is self-hosting, so it should be able + // to build even though real "ios" devices can't exec. + } else { + // The usual iOS sandbox does not allow the app to start another + // process. If we add builders on stock iOS devices, they presumably + // will not be able to exec, so we may as well allow that now. + t.Logf("HasGoBuild is false on %s", b) + return + } + case "android": + if strings.HasSuffix(b, "-emu") && platform.MustLinkExternal(runtime.GOOS, runtime.GOARCH, false) { + // As of 2023-05-02, the test environment on the emulated builders is + // missing a C linker. + t.Logf("HasGoBuild is false on %s", b) + return + } + } + t.Fatalf("HasGoBuild unexpectedly false on %s", b) + } + + t.Logf("HasGoBuild is true; checking consistency with other functions") + + hasExec := false + hasExecGo := false + t.Run("MustHaveExec", func(t *testing.T) { + testenv.MustHaveExec(t) + hasExec = true + }) + t.Run("MustHaveExecPath", func(t *testing.T) { + testenv.MustHaveExecPath(t, "go") + hasExecGo = true + }) + if !hasExec { + t.Errorf(`MustHaveExec(t) skipped unexpectedly`) + } + if !hasExecGo { + t.Errorf(`MustHaveExecPath(t, "go") skipped unexpectedly`) + } + + dir := t.TempDir() + mainGo := filepath.Join(dir, "main.go") + if err := os.WriteFile(mainGo, []byte("package main\nfunc main() {}\n"), 0644); err != nil { + t.Fatal(err) + } + cmd := testenv.Command(t, "go", "build", "-o", os.DevNull, mainGo) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%v: %v\n%s", cmd, err, out) + } +} + +func TestMustHaveExec(t *testing.T) { + hasExec := false + t.Run("MustHaveExec", func(t *testing.T) { + testenv.MustHaveExec(t) + t.Logf("MustHaveExec did not skip") + hasExec = true + }) + + switch runtime.GOOS { + case "js", "wasip1": + if hasExec { + // js and wasip1 lack an “exec” syscall. + t.Errorf("expected MustHaveExec to skip on %v", runtime.GOOS) + } + case "ios": + if b := testenv.Builder(); strings.HasSuffix(b, "-corellium") && !hasExec { + // Most ios environments can't exec, but the corellium builder can. + t.Errorf("expected MustHaveExec not to skip on %v", b) + } + default: + if b := testenv.Builder(); b != "" && !hasExec { + t.Errorf("expected MustHaveExec not to skip on %v", b) + } + } +}