Unify command error handling

BUG=chromium:773875
TEST=unit test

Change-Id: Ibe32309c021d72e08cecc7d6830756fa1503e809
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/1695801
Reviewed-by: Tobias Bosch <tbosch@google.com>
Tested-by: Tobias Bosch <tbosch@google.com>

7 files changed, 89 insertions, 62 deletions

diff --git a/compiler_wrapper/clang_syntax_flag.go b/compiler_wrapper/clang_syntax_flag.go
index 7a3d0a31..5ea1cca1 100644
--- a/compiler_wrapper/clang_syntax_flag.go
+++ b/compiler_wrapper/clang_syntax_flag.go
@@ -17,12 +17,6 @@ func checkClangSyntax(env env, clangCmd *command) (exitCode int, err error) {
 		args:       append(clangCmd.args, "-fsyntax-only", "-stdlib=libstdc++"),
 		envUpdates: clangCmd.envUpdates,
 	}
-	if err := env.run(clangSyntaxCmd, env.stdout(), env.stderr()); err != nil {
-		if exitCode, ok := getExitCode(err); ok {
-			return exitCode, nil
-		}
-		return exitCode, wrapErrorwithSourceLocf(err, "failed to call clang for syntax check. Command: %#v",
-			clangSyntaxCmd)
-	}
-	return exitCode, nil
+	return wrapSubprocessErrorWithSourceLoc(clangSyntaxCmd,
+		env.run(clangSyntaxCmd, env.stdout(), env.stderr()))
 }
diff --git a/compiler_wrapper/clang_tidy_flag.go b/compiler_wrapper/clang_tidy_flag.go
index 9c1761ee..4a648ce1 100644
--- a/compiler_wrapper/clang_tidy_flag.go
+++ b/compiler_wrapper/clang_tidy_flag.go
@@ -73,18 +73,14 @@ func runClangTidy(env env, clangCmd *command, cSrcFile string) error {
 		envUpdates: clangCmd.envUpdates,
 	}
 
-	if err := env.run(clangTidyCmd, env.stdout(), env.stderr()); err != nil {
-		if _, ok := getExitCode(err); ok {
-			// Note: We continue on purpose when clang-tidy fails
-			// to maintain compatibility with the previous wrapper.
-			fmt.Fprintf(env.stderr(), "clang-tidy failed")
-		} else {
-			return wrapErrorwithSourceLocf(err, "failed to call clang tidy. Command: %#v",
-				clangTidyCmd)
-		}
+	exitCode, err := wrapSubprocessErrorWithSourceLoc(clangTidyCmd,
+		env.run(clangTidyCmd, env.stdout(), env.stderr()))
+	if err == nil && exitCode != 0 {
+		// Note: We continue on purpose when clang-tidy fails
+		// to maintain compatibility with the previous wrapper.
+		fmt.Fprintf(env.stderr(), "clang-tidy failed")
 	}
-
-	return nil
+	return err
 }
 
 func hasAtLeastOneSuffix(s string, suffixes []string) bool {
diff --git a/compiler_wrapper/compiler_wrapper.go b/compiler_wrapper/compiler_wrapper.go
index 3f94e185..e24faef3 100644
--- a/compiler_wrapper/compiler_wrapper.go
+++ b/compiler_wrapper/compiler_wrapper.go
@@ -4,8 +4,6 @@ import (
 	"fmt"
 	"io"
 	"path/filepath"
-	"strings"
-	"syscall"
 )
 
 func callCompiler(env env, cfg *config, inputCmd *command) int {
@@ -79,18 +77,9 @@ func callCompilerInternal(env env, cfg *config, inputCmd *command) (exitCode int
 	if shouldForceDisableWError(env) {
 		return doubleBuildWithWNoError(env, cfg, compilerCmd)
 	}
-	if err := env.exec(compilerCmd); err != nil {
-		if userErr, ok := getCCacheError(compilerCmd, err); ok {
-			return 0, userErr
-		}
-		// Note: This case only happens when the underlying env is not
-		// really doing an exec, e.g. commandRecordingEnv.
-		if exitCode, ok := getExitCode(err); ok {
-			return exitCode, nil
-		}
-		return exitCode, wrapErrorwithSourceLocf(err, "failed to execute %#v", compilerCmd)
-	}
-	return 0, err
+	// Note: We return an exit code only if the underlying env is not
+	// really doing an exec, e.g. commandRecordingEnv.
+	return wrapSubprocessErrorWithSourceLoc(compilerCmd, env.exec(compilerCmd))
 }
 
 func calcClangCommand(forceLocal bool, builder *commandBuilder) (*command, error) {
@@ -142,17 +131,6 @@ func getAbsWrapperDir(env env, wrapperPath string) (string, error) {
 	return filepath.Dir(evaledCmdPath), nil
 }
 
-func getCCacheError(compilerCmd *command, compilerCmdErr error) (ccacheErr userError, ok bool) {
-	if en, ok := compilerCmdErr.(syscall.Errno); ok && en == syscall.ENOENT &&
-		strings.Contains(compilerCmd.path, "ccache") {
-		ccacheErr =
-			newUserErrorf("ccache not found under %s. Please install it",
-				compilerCmd.path)
-		return ccacheErr, ok
-	}
-	return ccacheErr, false
-}
-
 func printCompilerError(writer io.Writer, compilerErr error) {
 	if _, ok := compilerErr.(userError); ok {
 		fmt.Fprintf(writer, "%s\n", compilerErr)
diff --git a/compiler_wrapper/disable_werror_flag.go b/compiler_wrapper/disable_werror_flag.go
index 3115e692..28b257b0 100644
--- a/compiler_wrapper/disable_werror_flag.go
+++ b/compiler_wrapper/disable_werror_flag.go
@@ -15,10 +15,10 @@ func shouldForceDisableWError(env env) bool {
 func doubleBuildWithWNoError(env env, cfg *config, originalCmd *command) (exitCode int, err error) {
 	originalStdoutBuffer := &bytes.Buffer{}
 	originalStderrBuffer := &bytes.Buffer{}
-	err = env.run(originalCmd, originalStdoutBuffer, originalStderrBuffer)
-	originalExitCode, ok := getExitCode(err)
-	if !ok {
-		return 0, wrapErrorwithSourceLocf(err, "failed to execute %#v", originalCmd)
+	originalExitCode, err := wrapSubprocessErrorWithSourceLoc(originalCmd,
+		env.run(originalCmd, originalStdoutBuffer, originalStderrBuffer))
+	if err != nil {
+		return 0, err
 	}
 	// The only way we can do anything useful is if it looks like the failure
 	// was -Werror-related.
@@ -35,10 +35,10 @@ func doubleBuildWithWNoError(env env, cfg *config, originalCmd *command) (exitCo
 		args:       append(originalCmd.args, "-Wno-error"),
 		envUpdates: originalCmd.envUpdates,
 	}
-	err = env.run(retryCommand, retryStdoutBuffer, retryStderrBuffer)
-	retryExitCode, ok := getExitCode(err)
-	if !ok {
-		return 0, wrapErrorwithSourceLocf(err, "failed to execute %#v", retryCommand)
+	retryExitCode, err := wrapSubprocessErrorWithSourceLoc(retryCommand,
+		env.run(retryCommand, retryStdoutBuffer, retryStderrBuffer))
+	if err != nil {
+		return 0, err
 	}
 	// If -Wno-error fixed us, pretend that we never ran without -Wno-error.
 	// Otherwise, pretend that we never ran the second invocation. Since -Werror
diff --git a/compiler_wrapper/errors.go b/compiler_wrapper/errors.go
index 525b986e..7095e37d 100644
--- a/compiler_wrapper/errors.go
+++ b/compiler_wrapper/errors.go
@@ -30,6 +30,20 @@ func wrapErrorwithSourceLocf(err error, format string, v ...interface{}) error {
 	return newErrorwithSourceLocfInternal(2, "%s: %s", fmt.Sprintf(format, v...), err.Error())
 }
 
+func wrapSubprocessErrorWithSourceLoc(cmd *command, subprocessErr error) (exitCode int, err error) {
+	if subprocessErr == nil {
+		return 0, nil
+	}
+	if userErr, ok := getCCacheError(cmd, subprocessErr); ok {
+		return 0, userErr
+	}
+	if exitCode, ok := getExitCode(subprocessErr); ok {
+		return exitCode, nil
+	}
+	err = newErrorwithSourceLocfInternal(2, "failed to execute %#v: %s", cmd, subprocessErr)
+	return 0, err
+}
+
 // Based on the implementation of log.Output
 func newErrorwithSourceLocfInternal(skip int, format string, v ...interface{}) error {
 	_, file, line, ok := runtime.Caller(skip)
@@ -55,3 +69,14 @@ func getExitCode(err error) (exitCode int, ok bool) {
 	}
 	return 0, false
 }
+
+func getCCacheError(compilerCmd *command, compilerCmdErr error) (ccacheErr userError, ok bool) {
+	if en, ok := compilerCmdErr.(syscall.Errno); ok && en == syscall.ENOENT &&
+		strings.Contains(compilerCmd.path, "ccache") {
+		ccacheErr =
+			newUserErrorf("ccache not found under %s. Please install it",
+				compilerCmd.path)
+		return ccacheErr, true
+	}
+	return ccacheErr, false
+}
diff --git a/compiler_wrapper/errors_test.go b/compiler_wrapper/errors_test.go
index 626d3148..29c941df 100644
--- a/compiler_wrapper/errors_test.go
+++ b/compiler_wrapper/errors_test.go
@@ -2,12 +2,14 @@ package main
 
 import (
 	"errors"
+	"fmt"
+	"syscall"
 	"testing"
 )
 
 func TestNewErrorwithSourceLocfMessage(t *testing.T) {
 	err := newErrorwithSourceLocf("a%sc", "b")
-	if err.Error() != "errors_test.go:9: abc" {
+	if err.Error() != "errors_test.go:11: abc" {
 		t.Errorf("Error message incorrect. Got: %s", err.Error())
 	}
 }
@@ -15,7 +17,7 @@ func TestNewErrorwithSourceLocfMessage(t *testing.T) {
 func TestWrapErrorwithSourceLocfMessage(t *testing.T) {
 	cause := errors.New("someCause")
 	err := wrapErrorwithSourceLocf(cause, "a%sc", "b")
-	if err.Error() != "errors_test.go:17: abc: someCause" {
+	if err.Error() != "errors_test.go:19: abc: someCause" {
 		t.Errorf("Error message incorrect. Got: %s", err.Error())
 	}
 }
@@ -26,3 +28,41 @@ func TestNewUserErrorf(t *testing.T) {
 		t.Errorf("Error message incorrect. Got: %s", err.Error())
 	}
 }
+
+func TestSubprocessOk(t *testing.T) {
+	exitCode, err := wrapSubprocessErrorWithSourceLoc(nil, nil)
+	if exitCode != 0 {
+		t.Errorf("unexpected exit code. Got: %d", exitCode)
+	}
+	if err != nil {
+		t.Errorf("unexpected error. Got: %s", err)
+	}
+}
+
+func TestSubprocessExitCodeError(t *testing.T) {
+	exitCode, err := wrapSubprocessErrorWithSourceLoc(nil, newExitCodeError(23))
+	if exitCode != 23 {
+		t.Errorf("unexpected exit code. Got: %d", exitCode)
+	}
+	if err != nil {
+		t.Errorf("unexpected error. Got: %s", err)
+	}
+}
+
+func TestSubprocessCCacheError(t *testing.T) {
+	_, err := wrapSubprocessErrorWithSourceLoc(&command{path: "/usr/bin/ccache"}, syscall.ENOENT)
+	if _, ok := err.(userError); !ok {
+		t.Errorf("unexpected error type. Got: %T", err)
+	}
+	if err.Error() != "ccache not found under /usr/bin/ccache. Please install it" {
+		t.Errorf("unexpected error message. Got: %s", err)
+	}
+}
+
+func TestSubprocessGeneralError(t *testing.T) {
+	cmd := &command{path: "somepath"}
+	_, err := wrapSubprocessErrorWithSourceLoc(cmd, errors.New("someerror"))
+	if err.Error() != fmt.Sprintf("errors_test.go:64: failed to execute %#v: someerror", cmd) {
+		t.Errorf("Error message incorrect. Got: %s", err.Error())
+	}
+}
diff --git a/compiler_wrapper/oldwrapper.go b/compiler_wrapper/oldwrapper.go
index 56559a31..f59d0cc2 100644
--- a/compiler_wrapper/oldwrapper.go
+++ b/compiler_wrapper/oldwrapper.go
@@ -329,16 +329,10 @@ sys.exit(main())
 
 	// Note: Using a self executable wrapper does not work due to a race condition
 	// on unix systems. See https://github.com/golang/go/issues/22315
-	if err := env.run(&command{
+	oldWrapperCmd := &command{
 		path:       "/usr/bin/python2",
 		args:       append([]string{"-S", mockFile.Name()}, inputCmd.args...),
 		envUpdates: inputCmd.envUpdates,
-	}, stdout, stderr); err != nil {
-		if exitCode, ok := getExitCode(err); ok {
-			return exitCode, nil
-		}
-		return 0, wrapErrorwithSourceLocf(err, "failed to call old wrapper. Command: %#v",
-			inputCmd)
 	}
-	return 0, nil
+	return wrapSubprocessErrorWithSourceLoc(oldWrapperCmd, env.run(oldWrapperCmd, stdout, stderr))
 }