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 <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>

1 files changed, 17 insertions, 22 deletions

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