x/tools: don't parse CombinedOutput

This change fixes a bug in internal/testenv in which it would
gather the combined output (2>&1) of the "go env" command and
parse it as an environment variable. However, certain environment
variables (e.g. GODEBUG) cause "go env" to log to stderr,
so that the parser reads garbage. Use Output instead.

Also, preemptively fix a number of similar occurrences in
x/tools. CombinedOutput should be used only when the whole
output is ultimately sent to stderr or a log for human eyes,
or for tests that look for specific error messages in the
unstructured combined log. In those cases, the scope of
the 'out' variable can be reduced to avoid temptation.

Fixes golang/go#65729

Change-Id: Ifc0fd494fcde0e339bb5283e39c7696a34f5a699

.

Change-Id: I6eadd0e76498dc5f4d91e0904af2d52e610df683
Reviewed-on: https://go-review.googlesource.com/c/tools/+/564516
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>

go/analysis/unitchecker/unitchecker_test.go

@@ -169,6 +169,13 @@ func _() {
 		cmd.Env = append(exported.Config.Env, "ENTRYPOINT=minivet")
 		cmd.Dir = exported.Config.Dir
 
+		// TODO(golang/go#65729): this is unsound: any extra
+		// logging by the child process (e.g. due to GODEBUG
+		// options) will add noise to stderr, causing the
+		// CombinedOutput to be unparseable as JSON. But we
+		// can't simply use Output here as some of the tests
+		// look for substrings of stderr. Rework the test to
+		// be specific about which output stream to match.
 		out, err := cmd.CombinedOutput()
 		exitcode := 0
 		if exitErr, ok := err.(*exec.ExitError); ok {

go/gcexportdata/gcexportdata.go

@@ -47,7 +47,7 @@ import (
 func Find(importPath, srcDir string) (filename, path string) {
 	cmd := exec.Command("go", "list", "-json", "-export", "--", importPath)
 	cmd.Dir = srcDir
-	out, err := cmd.CombinedOutput()
+	out, err := cmd.Output()
 	if err != nil {
 		return "", ""
 	}

go/internal/cgo/cgo_pkgconfig.go

@@ -15,7 +15,7 @@ import (
 // pkgConfig runs pkg-config with the specified arguments and returns the flags it prints.
 func pkgConfig(mode string, pkgs []string) (flags []string, err error) {
 	cmd := exec.Command("pkg-config", append([]string{mode}, pkgs...)...)
-	out, err := cmd.CombinedOutput()
+	out, err := cmd.Output()
 	if err != nil {
 		s := fmt.Sprintf("%s failed: %v", strings.Join(cmd.Args, " "), err)
 		if len(out) > 0 {

go/internal/gccgoimporter/importer_test.go

@@ -140,7 +140,7 @@ func TestObjImporter(t *testing.T) {
 		t.Skip("no support yet for debug/xcoff")
 	}
 
-	verout, err := exec.Command(gpath, "--version").CombinedOutput()
+	verout, err := exec.Command(gpath, "--version").Output()
 	if err != nil {
 		t.Logf("%s", verout)
 		t.Fatal(err)
@@ -182,8 +182,7 @@ func TestObjImporter(t *testing.T) {
 		afile := filepath.Join(artmpdir, "lib"+test.pkgpath+".a")
 
 		cmd := exec.Command(gpath, "-fgo-pkgpath="+test.pkgpath, "-c", "-o", ofile, gofile)
-		out, err := cmd.CombinedOutput()
-		if err != nil {
+		if out, err := cmd.CombinedOutput(); err != nil {
 			t.Logf("%s", out)
 			t.Fatalf("gccgo %s failed: %s", gofile, err)
 		}
@@ -191,8 +190,7 @@ func TestObjImporter(t *testing.T) {
 		runImporterTest(t, imp, initmap, &test)
 
 		cmd = exec.Command("ar", "cr", afile, ofile)
-		out, err = cmd.CombinedOutput()
-		if err != nil {
+		if out, err := cmd.CombinedOutput(); err != nil {
 			t.Logf("%s", out)
 			t.Fatalf("ar cr %s %s failed: %s", afile, ofile, err)
 		}

internal/diff/difftest/difftest_test.go

@@ -64,7 +64,7 @@ func getDiffOutput(a, b string) (string, error) {
 	}
 	cmd := exec.Command("diff", "-u", fileA.Name(), fileB.Name())
 	cmd.Env = append(cmd.Env, "LANG=en_US.UTF-8")
-	out, err := cmd.CombinedOutput()
+	out, err := cmd.Output()
 	if err != nil {
 		if _, ok := err.(*exec.ExitError); !ok {
 			return "", fmt.Errorf("failed to run diff -u %v %v: %v\n%v", fileA.Name(), fileB.Name(), err, string(out))

internal/gcimporter/gcimporter_test.go

@@ -84,8 +84,7 @@ func compilePkg(t *testing.T, dirname, filename, outdirname string, packagefiles
 	importreldir := strings.ReplaceAll(outdirname, string(os.PathSeparator), "/")
 	cmd := exec.Command("go", "tool", "compile", "-p", pkg, "-D", importreldir, "-importcfg", importcfgfile, "-o", outname, filename)
 	cmd.Dir = dirname
-	out, err := cmd.CombinedOutput()
-	if err != nil {
+	if out, err := cmd.CombinedOutput(); err != nil {
 		t.Logf("%s", out)
 		t.Fatalf("go tool compile %s failed: %s", filename, err)
 	}

internal/testenv/testenv.go

@@ -83,7 +83,7 @@ func hasTool(tool string) error {
 				// GOROOT. Otherwise, 'some/path/go test ./...' will test against some
 				// version of the 'go' binary other than 'some/path/go', which is almost
 				// certainly not what the user intended.
-				out, err := exec.Command(tool, "env", "GOROOT").CombinedOutput()
+				out, err := exec.Command(tool, "env", "GOROOT").Output()
 				if err != nil {
 					checkGoBuild.err = err
 					return
@@ -141,7 +141,7 @@ func cgoEnabled(bypassEnvironment bool) (bool, error) {
 	if bypassEnvironment {
 		cmd.Env = append(append([]string(nil), os.Environ()...), "CGO_ENABLED=")
 	}
-	out, err := cmd.CombinedOutput()
+	out, err := cmd.Output()
 	if err != nil {
 		return false, err
 	}
@@ -199,6 +199,9 @@ func NeedsTool(t testing.TB, tool string) {
 
 	t.Helper()
 	if allowMissingTool(tool) {
+		// TODO(adonovan): might silently skipping because of
+		// (e.g.) mismatched go env GOROOT and runtime.GOROOT
+		// risk some users not getting the coverage they expect?
 		t.Skipf("skipping because %s tool not available: %v", tool, err)
 	} else {
 		t.Fatalf("%s tool not available: %v", tool, err)

present/parse_test.go

@@ -79,7 +79,7 @@ func diff(prefix string, name1 string, b1 []byte, name2 string, b2 []byte) ([]by
 		cmd = "/bin/ape/diff"
 	}
 
-	data, err := exec.Command(cmd, "-u", f1, f2).CombinedOutput()
+	data, err := exec.Command(cmd, "-u", f1, f2).Output()
 	if len(data) > 0 {
 		// diff exits with a non-zero status when the files don't match.
 		// Ignore that failure as long as we get output.

refactor/rename/rename.go

@@ -588,7 +588,7 @@ func diff(filename string, content []byte) error {
 	}
 	defer os.Remove(renamed)
 
-	diff, err := exec.Command(DiffCmd, "-u", filename, renamed).CombinedOutput()
+	diff, err := exec.Command(DiffCmd, "-u", filename, renamed).Output()
 	if len(diff) > 0 {
 		// diff exits with a non-zero status when the files don't match.
 		// Ignore that failure as long as we get output.