[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>
diff --git a/compiler_wrapper/compiler_wrapper.go b/compiler_wrapper/compiler_wrapper.go
index da9c866..a7b87dc 100644
--- a/compiler_wrapper/compiler_wrapper.go
+++ b/compiler_wrapper/compiler_wrapper.go
@@ -127,14 +127,14 @@
}
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 75f71a0..1180127 100644
--- a/compiler_wrapper/disable_werror_flag.go
+++ b/compiler_wrapper/disable_werror_flag.go
@@ -7,30 +7,21 @@
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 @@
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,37 +110,6 @@
retryStdoutBuffer.WriteTo(env.stdout())
retryStderrBuffer.WriteTo(env.stderr())
- // 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.
-
- // Buildbots use a nonzero umask, which isn't quite what we want: these directories should
- // be world-readable and world-writable.
- oldMask := syscall.Umask(0)
- 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)
- }
-
- // Have some tag to show that files aren't fully written. It would be sad if
- // an interrupted build (or out of disk space, or similar) caused tools to
- // have to be overly-defensive.
- incompleteSuffix := ".incomplete"
-
- // 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)
- if err != nil {
- return 0, wrapErrorwithSourceLocf(err, "error creating warnings file")
- }
-
- if err := tmpFile.Chmod(0666); err != nil {
- return 0, wrapErrorwithSourceLocf(err, "error chmoding the file to be world-readable/writeable")
- }
-
lines := []string{}
if originalStderrBuffer.Len() > 0 {
lines = append(lines, originalStderrBuffer.String())
@@ -164,6 +124,51 @@
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.
+
+ // Buildbots use a nonzero umask, which isn't quite what we want: these directories should
+ // be world-readable and world-writable.
+ oldMask := syscall.Umask(0)
+ defer syscall.Umask(oldMask)
+
+ // Allow root and regular users to write to this without issue.
+ 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
+ // an interrupted build (or out of disk space, or similar) caused tools to
+ // have to be overly-defensive.
+ incompleteSuffix := ".incomplete"
+
+ // 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(cfg.newWarningsDir, "warnings_report*.json"+incompleteSuffix)
+ if err != nil {
+ return 0, wrapErrorwithSourceLocf(err, "error creating warnings file")
+ }
+
+ if err := tmpFile.Chmod(0666); err != nil {
+ return 0, wrapErrorwithSourceLocf(err, "error chmoding the file to be world-readable/writeable")
+ }
+
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 28e4837..ecc1090 100644
--- a/compiler_wrapper/disable_werror_flag_test.go
+++ b/compiler_wrapper/disable_werror_flag_test.go
@@ -11,7 +11,6 @@
"io"
"io/ioutil"
"os"
- "path"
"path/filepath"
"strings"
"testing"
@@ -407,45 +406,28 @@
})
}
-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")
}
})
}
