x/tools: address review of CL 564515 (CombinedOutput)

I fumbled with git and forked the CL, stranding some review
comments from bcmills on the ill-fated fork.

Updates golang/go#65729

Change-Id: I6d0bf431f841dacb94e9e13a90bf39f8e2ed2fbf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/564339
Reviewed-by: Bryan Mills <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

go/internal/cgo/cgo_pkgconfig.go

@@ -21,6 +21,9 @@ func pkgConfig(mode string, pkgs []string) (flags []string, err error) {
 		if len(out) > 0 {
 			s = fmt.Sprintf("%s: %s", s, out)
 		}
+		if err, ok := err.(*exec.ExitError); ok && len(err.Stderr) > 0 {
+			s = fmt.Sprintf("%s\nstderr:\n%s", s, err.Stderr)
+		}
 		return nil, errors.New(s)
 	}
 	if len(out) > 0 {

go/internal/gccgoimporter/importer_test.go

@@ -143,6 +143,9 @@ func TestObjImporter(t *testing.T) {
 	verout, err := exec.Command(gpath, "--version").Output()
 	if err != nil {
 		t.Logf("%s", verout)
+		if exit, ok := err.(*exec.ExitError); ok && len(exit.Stderr) > 0 {
+			t.Logf("stderr:\n%s", exit.Stderr)
+		}
 		t.Fatal(err)
 	}
 	vers := regexp.MustCompile(`([0-9]+)\.([0-9]+)`).FindSubmatch(verout)

internal/diff/difftest/difftest_test.go

@@ -66,9 +66,15 @@ func getDiffOutput(a, b string) (string, error) {
 	cmd.Env = append(cmd.Env, "LANG=en_US.UTF-8")
 	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))
+		exit, ok := err.(*exec.ExitError)
+		if !ok {
+			return "", fmt.Errorf("can't exec %s: %v", cmd, err)
 		}
+		if len(out) == 0 {
+			// Nonzero exit with no output: terminated by signal?
+			return "", fmt.Errorf("%s failed: %v; stderr:\n%s", cmd, err, exit.Stderr)
+		}
+		// nonzero exit + output => files differ
 	}
 	diff := string(out)
 	if len(diff) <= 0 {

internal/testenv/testenv.go

@@ -85,6 +85,9 @@ func hasTool(tool string) error {
 				// certainly not what the user intended.
 				out, err := exec.Command(tool, "env", "GOROOT").Output()
 				if err != nil {
+					if exit, ok := err.(*exec.ExitError); ok && len(exit.Stderr) > 0 {
+						err = fmt.Errorf("%w\nstderr:\n%s)", err, exit.Stderr)
+					}
 					checkGoBuild.err = err
 					return
 				}
@@ -143,6 +146,9 @@ func cgoEnabled(bypassEnvironment bool) (bool, error) {
 	}
 	out, err := cmd.Output()
 	if err != nil {
+		if exit, ok := err.(*exec.ExitError); ok && len(exit.Stderr) > 0 {
+			err = fmt.Errorf("%w\nstderr:\n%s", err, exit.Stderr)
+		}
 		return false, err
 	}
 	enabled := strings.TrimSpace(string(out))
@@ -199,9 +205,12 @@ 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?
+		// TODO(adonovan): if we skip because of (e.g.)
+		// mismatched go env GOROOT and runtime.GOROOT, don't
+		// we risk some users not getting the coverage they expect?
+		// bcmills notes: this shouldn't be a concern as of CL 404134 (Go 1.19).
+		// We could probably safely get rid of that GOPATH consistency
+		// check entirely at this point.
 		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

@@ -6,6 +6,7 @@ package present
 
 import (
 	"bytes"
+	"fmt"
 	"html/template"
 	"os"
 	"os/exec"
@@ -84,6 +85,8 @@ func diff(prefix string, name1 string, b1 []byte, name2 string, b2 []byte) ([]by
 		// diff exits with a non-zero status when the files don't match.
 		// Ignore that failure as long as we get output.
 		err = nil
+	} else if exit, ok := err.(*exec.ExitError); ok && len(exit.Stderr) > 0 {
+		err = fmt.Errorf("%w\nstderr:\n%s)", err, exit.Stderr)
 	}
 
 	data = bytes.Replace(data, []byte(f1), []byte(name1), -1)

refactor/rename/rename.go

@@ -596,6 +596,9 @@ func diff(filename string, content []byte) error {
 		return nil
 	}
 	if err != nil {
+		if exit, ok := err.(*exec.ExitError); ok && len(exit.Stderr) > 0 {
+			err = fmt.Errorf("%w\nstderr:\n%s", err, exit.Stderr)
+		}
 		return fmt.Errorf("computing diff: %v", err)
 	}
 	return nil