]> Cypherpunks repositories - gostls13.git/commitdiff
internal/testenv: remove HasExec and simplify other support checks again
authorBryan C. Mills <bcmills@google.com>
Tue, 2 May 2023 17:07:26 +0000 (13:07 -0400)
committerGopher Robot <gobot@golang.org>
Thu, 4 May 2023 19:52:29 +0000 (19:52 +0000)
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 <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>

src/internal/testenv/exec.go
src/internal/testenv/testenv.go
src/internal/testenv/testenv_test.go

index 882791ddca12175fc9a4e34daee7438103700765..481be2e649fbefac7c6e18a471dbb3f1f9627811 100644 (file)
@@ -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
index d03bb0550aa98e10c081ed99f04f77e9cb3df594..70606242d66b725eeaa6824fe8d39df1d250ba1c 100644 (file)
@@ -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))
index ebc27f159ad80c14ed9dfe821d011e26b18e685f..be766288e8874c6fb9a164040d2f73c1cfc469cb 100644 (file)
@@ -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)
+               }
+       }
+}