[android wrapper] Write warning report to stdout on Android

Write the warning report to stdout (surrounded by a tag
<LLVM_NEXT_ERROR_REPORT>).  Currently OUT_DIR only gets passed to local
compiles, and so remote compiles can't do the double build.  Even if
they write to /tmp, RBE or Goma won't know to copy them as part of the
output.

Packaging the warnings reports is a problem as well since the additional
build step needs extra surgery in soong (it's not possible to add a
dangling, optional build step in Ninja).

Writing the reports to stdout conveniently solves both these issues.  We
can just scrape the reports with a script.

Originall reviewed at
https://android-review.googlesource.com/c/platform/external/toolchain-utils/+/1328433/2.

BUG=None
TEST=build master with r391452 and check progress past new errors.

Change-Id: I5f1f71faba002836067a82f6aa4b26d5ba8b7b99
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/2239371
Commit-Queue: Pirama Arumuga Nainar <pirama@google.com>
Tested-by: Pirama Arumuga Nainar <pirama@google.com>
Reviewed-by: George Burgess <gbiv@chromium.org>

3 files changed, 47 insertions, 60 deletions

diff --git a/compiler_wrapper/compiler_wrapper.go b/compiler_wrapper/compiler_wrapper.go
index da9c8660..a7b87dc0 100644
--- a/compiler_wrapper/compiler_wrapper.go
+++ b/compiler_wrapper/compiler_wrapper.go
@@ -127,14 +127,14 @@ func callCompilerInternal(env env, cfg *config, inputCmd *command) (exitCode int
 	}
 	rusageLogfileName := getRusageLogFilename(env)
 	bisectStage := getBisectStage(env)
-	if newWarningsDir, ok := getNewWarningsDir(env, cfg); ok {
+	if shouldForceDisableWerror(env, cfg) {
 		if rusageLogfileName != "" {
 			return 0, newUserErrorf("GETRUSAGE is meaningless with FORCE_DISABLE_WERROR")
 		}
 		if bisectStage != "" {
 			return 0, newUserErrorf("BISECT_STAGE is meaningless with FORCE_DISABLE_WERROR")
 		}
-		return doubleBuildWithWNoError(env, cfg, newWarningsDir, compilerCmd)
+		return doubleBuildWithWNoError(env, cfg, compilerCmd)
 	}
 	if shouldCompileWithFallback(env) {
 		if rusageLogfileName != "" {
diff --git a/compiler_wrapper/disable_werror_flag.go b/compiler_wrapper/disable_werror_flag.go
index 75f71a0d..1180127c 100644
--- a/compiler_wrapper/disable_werror_flag.go
+++ b/compiler_wrapper/disable_werror_flag.go
@@ -7,30 +7,21 @@ package main
 import (
 	"bytes"
 	"encoding/json"
+	"io"
 	"io/ioutil"
 	"os"
-	"path"
 	"strings"
 	"syscall"
 )
 
 const numWErrorEstimate = 30
 
-func getNewWarningsDir(env env, cfg *config) (dirName string, ok bool) {
+func shouldForceDisableWerror(env env, cfg *config) bool {
 	if cfg.isAndroidWrapper {
-		if cfg.useLlvmNext {
-			value, _ := env.getenv("OUT_DIR")
-			if value != "" {
-				return path.Join(value, "warnings_reports"), true
-			}
-		}
-	} else {
-		value, _ := env.getenv("FORCE_DISABLE_WERROR")
-		if value != "" {
-			return cfg.newWarningsDir, true
-		}
+		return cfg.useLlvmNext
 	}
-	return "", false
+	value, _ := env.getenv("FORCE_DISABLE_WERROR")
+	return value != ""
 }
 
 func disableWerrorFlags(originalArgs []string) []string {
@@ -62,7 +53,7 @@ func isLikelyAConfTest(cfg *config, cmd *command) bool {
 	return false
 }
 
-func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, originalCmd *command) (exitCode int, err error) {
+func doubleBuildWithWNoError(env env, cfg *config, originalCmd *command) (exitCode int, err error) {
 	originalStdoutBuffer := &bytes.Buffer{}
 	originalStderrBuffer := &bytes.Buffer{}
 	// TODO: This is a bug in the old wrapper that it drops the ccache path
@@ -119,6 +110,34 @@ func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, origin
 	retryStdoutBuffer.WriteTo(env.stdout())
 	retryStderrBuffer.WriteTo(env.stderr())
 
+	lines := []string{}
+	if originalStderrBuffer.Len() > 0 {
+		lines = append(lines, originalStderrBuffer.String())
+	}
+	if originalStdoutBuffer.Len() > 0 {
+		lines = append(lines, originalStdoutBuffer.String())
+	}
+	outputToLog := strings.Join(lines, "\n")
+
+	jsonData := warningsJSONData{
+		Cwd:     env.getwd(),
+		Command: append([]string{originalCmd.Path}, originalCmd.Args...),
+		Stdout:  outputToLog,
+	}
+
+	// Write warning report to stdout for Android.  On Android,
+	// double-build can be requested on remote builds as well, where there
+	// is no canonical place to write the warnings report.
+	if cfg.isAndroidWrapper {
+		stdout := env.stdout()
+		io.WriteString(stdout, "<LLVM_NEXT_ERROR_REPORT>")
+		if err := json.NewEncoder(stdout).Encode(jsonData); err != nil {
+			return 0, wrapErrorwithSourceLocf(err, "error in json.Marshal")
+		}
+		io.WriteString(stdout, "</LLVM_NEXT_ERROR_REPORT>")
+		return retryExitCode, nil
+	}
+
 	// All of the below is basically logging. If we fail at any point, it's
 	// reasonable for that to fail the build. This is all meant for FYI-like
 	// builders in the first place.
@@ -129,8 +148,8 @@ func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, origin
 	defer syscall.Umask(oldMask)
 
 	// Allow root and regular users to write to this without issue.
-	if err := os.MkdirAll(newWarningsDir, 0777); err != nil {
-		return 0, wrapErrorwithSourceLocf(err, "error creating warnings directory %s", newWarningsDir)
+	if err := os.MkdirAll(cfg.newWarningsDir, 0777); err != nil {
+		return 0, wrapErrorwithSourceLocf(err, "error creating warnings directory %s", cfg.newWarningsDir)
 	}
 
 	// Have some tag to show that files aren't fully written. It would be sad if
@@ -141,7 +160,7 @@ func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, origin
 	// Coming up with a consistent name for this is difficult (compiler command's
 	// SHA can clash in the case of identically named files in different
 	// directories, or similar); let's use a random one.
-	tmpFile, err := ioutil.TempFile(newWarningsDir, "warnings_report*.json"+incompleteSuffix)
+	tmpFile, err := ioutil.TempFile(cfg.newWarningsDir, "warnings_report*.json"+incompleteSuffix)
 	if err != nil {
 		return 0, wrapErrorwithSourceLocf(err, "error creating warnings file")
 	}
@@ -150,20 +169,6 @@ func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, origin
 		return 0, wrapErrorwithSourceLocf(err, "error chmoding the file to be world-readable/writeable")
 	}
 
-	lines := []string{}
-	if originalStderrBuffer.Len() > 0 {
-		lines = append(lines, originalStderrBuffer.String())
-	}
-	if originalStdoutBuffer.Len() > 0 {
-		lines = append(lines, originalStdoutBuffer.String())
-	}
-	outputToLog := strings.Join(lines, "\n")
-
-	jsonData := warningsJSONData{
-		Cwd:     env.getwd(),
-		Command: append([]string{originalCmd.Path}, originalCmd.Args...),
-		Stdout:  outputToLog,
-	}
 	enc := json.NewEncoder(tmpFile)
 	if err := enc.Encode(jsonData); err != nil {
 		_ = tmpFile.Close()
diff --git a/compiler_wrapper/disable_werror_flag_test.go b/compiler_wrapper/disable_werror_flag_test.go
index 28e48371..ecc1090b 100644
--- a/compiler_wrapper/disable_werror_flag_test.go
+++ b/compiler_wrapper/disable_werror_flag_test.go
@@ -11,7 +11,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -407,45 +406,28 @@ func TestDoubleBuildWerrorChmodsThingsAppropriately(t *testing.T) {
 	})
 }
 
-func TestAndroidGetNewWarningsDir(t *testing.T) {
+func TestAndroidDisableWerror(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
-		outDir := "/foo/bar"
-		expectedDir := path.Join(outDir, "warnings_reports")
-
-		ctx.env = []string{"OUT_DIR=" + outDir}
 		ctx.cfg.isAndroidWrapper = true
 
 		// Disable werror ON
 		ctx.cfg.useLlvmNext = true
-		dir, ok := getNewWarningsDir(ctx, ctx.cfg)
-		if !ok || dir != expectedDir {
-			t.Errorf("disable Werror not enabled for Android with useLlvmNext (dirName=%q ok=%t), expectedDir=%q", dir, ok, expectedDir)
+		if !shouldForceDisableWerror(ctx, ctx.cfg) {
+			t.Errorf("disable Werror not enabled for Android with useLlvmNext")
 		}
 
 		// Disable werror OFF
 		ctx.cfg.useLlvmNext = false
-		dir, ok = getNewWarningsDir(ctx, ctx.cfg)
-		if ok || dir != "" {
-			t.Errorf("disable Werror incorrectly enabled for Android without useLlvmNext (dirName=%q ok=%t)", dir, ok)
+		if shouldForceDisableWerror(ctx, ctx.cfg) {
+			t.Errorf("disable-Werror enabled for Android without useLlvmNext")
 		}
 	})
 }
 
-func TestChromeOSGetNewWarningsDirOn(t *testing.T) {
-	withForceDisableWErrorTestContext(t, func(ctx *testContext) {
-		dir, ok := getNewWarningsDir(ctx, ctx.cfg)
-		if !ok || dir != ctx.cfg.newWarningsDir {
-			t.Errorf("disable Werror not enabled for ChromeOS with FORCE_DISABLE_WERROR set (dirName=%q ok=%t) expectedDir=%q", dir, ok, ctx.cfg.newWarningsDir)
-		}
-
-	})
-}
-
-func TestChromeOSGetNewWarningsDirOff(t *testing.T) {
+func TestChromeOSNoForceDisableWerror(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
-		dir, ok := getNewWarningsDir(ctx, ctx.cfg)
-		if ok || dir != "" {
-			t.Errorf("disable Werror incorrectly enabled for ChromeOS without FORCE_DISABLE_WERROR set (dirName=%q ok=%t)", dir, ok)
+		if shouldForceDisableWerror(ctx, ctx.cfg) {
+			t.Errorf("disable Werror enabled for ChromeOS without FORCE_DISABLE_WERROR set")
 		}
 	})
 }