From 0506361a60215defc5098a6253fdc8f51371ce88 Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Fri, 25 Jun 2021 01:39:04 +0000 Subject: [PATCH] Add errorHints to stdout when read-only file system errors are detected The source tree will eventually be made ReadOnly, and recipes that write directly to the source tree will fail. Use a pattern-match approach on the results of stdout/stderr to provide hints to the user in such a scenario. If multiple patterns are found in raw output, print error hint corresponding to first pattern match. first pattern match is chosen since the failing function will be at the top of the stack, and hence will be logged first Test: Wrote a unit test to assert errorhint is added to output. Wrote an integration test that writes to a file in the source tree 1. When source_tree is RO, the recipe fails and an error hint is printed to stdout 2. When source tree is RW, the recipe succeeds and no error hint is printed Bug: 174726238 Change-Id: Id67b48f8094cdf8a571c239ae469d60464a1e89c --- tests/bootstrap_test.sh | 34 ++++++++++++++++++++++++ tests/lib.sh | 4 +++ ui/build/config.go | 4 +++ ui/build/sandbox_linux.go | 5 +++- ui/status/ninja.go | 55 ++++++++++++++++++++++++++++++++++++++- ui/status/ninja_test.go | 50 +++++++++++++++++++++++++++++++++++ 6 files changed, 150 insertions(+), 2 deletions(-) diff --git a/tests/bootstrap_test.sh b/tests/bootstrap_test.sh index 8c8dc8208..125868490 100755 --- a/tests/bootstrap_test.sh +++ b/tests/bootstrap_test.sh @@ -486,6 +486,39 @@ function test_null_build_after_docs { fi } +function test_write_to_source_tree { + setup + mkdir -p a + cat > a/Android.bp < /dev/null && \ + fail "Write to source tree should not work in a ReadOnly source tree" + + if grep -q "${ERROR_MSG}" ${ERROR_LOG} && grep -q "${ERROR_HINT_PATTERN}" ${ERROR_LOG} ; then + echo Error message and error hint found in logs >/dev/null + else + fail "Did not find Read-only error AND error hint in error.log" + fi + + # Test in ReadWrite source tree + run_ninja BUILD_BROKEN_SRC_DIR_IS_WRITABLE=true ${EXPECTED_OUT} &> /dev/null || \ + fail "Write to source tree did not succeed in a ReadWrite source tree" + + if grep -q "${ERROR_MSG}\|${ERROR_HINT_PATTERN}" ${ERROR_LOG} ; then + fail "Found read-only error OR error hint in error.log" + fi +} + function test_bp2build_smoke { setup GENERATE_BAZEL_FILES=1 run_soong @@ -692,6 +725,7 @@ test_add_file_to_soong_build test_glob_during_bootstrapping test_soong_build_rerun_iff_environment_changes test_dump_json_module_graph +test_write_to_source_tree test_bp2build_smoke test_bp2build_generates_fake_ninja_file test_bp2build_null_build diff --git a/tests/lib.sh b/tests/lib.sh index 35ccea973..f1e1efa09 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -126,6 +126,10 @@ run_bp2build() { GENERATE_BAZEL_FILES=true build/soong/soong_ui.bash --make-mode --skip-ninja --skip-make --skip-soong-tests nothing } +run_ninja() { + build/soong/soong_ui.bash --make-mode --skip-make --skip-soong-tests "$@" +} + info "Starting Soong integration test suite $(basename $0)" info "Mock top: $MOCK_TOP" diff --git a/ui/build/config.go b/ui/build/config.go index b9aaaf84e..7e119f361 100644 --- a/ui/build/config.go +++ b/ui/build/config.go @@ -158,6 +158,10 @@ func NewConfig(ctx Context, args ...string) Config { ret.distDir = filepath.Join(ret.OutDir(), "dist") } + if srcDirIsWritable, ok := ret.environ.Get("BUILD_BROKEN_SRC_DIR_IS_WRITABLE"); ok { + ret.sandboxConfig.SetSrcDirIsRO(srcDirIsWritable == "false") + } + ret.environ.Unset( // We're already using it "USE_SOONG_UI", diff --git a/ui/build/sandbox_linux.go b/ui/build/sandbox_linux.go index b0a674864..5b2046e54 100644 --- a/ui/build/sandbox_linux.go +++ b/ui/build/sandbox_linux.go @@ -97,8 +97,11 @@ func (c *Cmd) sandboxSupported() bool { "-u", "nobody", "-g", sandboxConfig.group, "-R", "/", - "-B", sandboxConfig.srcDir, + // Mount tmp before srcDir + // srcDir is /tmp/.* in integration tests, which is a child dir of /tmp + // nsjail throws an error if a child dir is mounted before its parent "-B", "/tmp", + "-B", sandboxConfig.srcDir, "-B", sandboxConfig.outDir, } diff --git a/ui/status/ninja.go b/ui/status/ninja.go index 765679fb2..2445972fb 100644 --- a/ui/status/ninja.go +++ b/ui/status/ninja.go @@ -19,6 +19,8 @@ import ( "fmt" "io" "os" + "regexp" + "strings" "syscall" "time" @@ -158,9 +160,10 @@ func (n *NinjaReader) run() { err = fmt.Errorf("exited with code: %d", exitCode) } + outputWithErrorHint := errorHintGenerator.GetOutputWithErrorHint(msg.EdgeFinished.GetOutput(), exitCode) n.status.FinishAction(ActionResult{ Action: started, - Output: msg.EdgeFinished.GetOutput(), + Output: outputWithErrorHint, Error: err, Stats: ActionResultStats{ UserTime: msg.EdgeFinished.GetUserTime(), @@ -219,3 +222,53 @@ func readVarInt(r *bufio.Reader) (int, error) { return ret, nil } + +// key is pattern in stdout/stderr +// value is error hint +var allErrorHints = map[string]string{ + "Read-only file system": `\nWrite to a read-only file system detected. Possible fixes include +1. Generate file directly to out/ which is ReadWrite, #recommend solution +2. BUILD_BROKEN_SRC_DIR_RW_ALLOWLIST := #discouraged, subset of source tree will be RW +3. BUILD_BROKEN_SRC_DIR_IS_WRITABLE := true #highly discouraged, entire source tree will be RW +`, +} +var errorHintGenerator = *newErrorHintGenerator(allErrorHints) + +type ErrorHintGenerator struct { + allErrorHints map[string]string + allErrorHintPatternsCompiled *regexp.Regexp +} + +func newErrorHintGenerator(allErrorHints map[string]string) *ErrorHintGenerator { + var allErrorHintPatterns []string + for errorHintPattern, _ := range allErrorHints { + allErrorHintPatterns = append(allErrorHintPatterns, errorHintPattern) + } + allErrorHintPatternsRegex := strings.Join(allErrorHintPatterns[:], "|") + re := regexp.MustCompile(allErrorHintPatternsRegex) + return &ErrorHintGenerator{ + allErrorHints: allErrorHints, + allErrorHintPatternsCompiled: re, + } +} + +func (errorHintGenerator *ErrorHintGenerator) GetOutputWithErrorHint(rawOutput string, buildExitCode int) string { + if buildExitCode == 0 { + return rawOutput + } + errorHint := errorHintGenerator.getErrorHint(rawOutput) + if errorHint == nil { + return rawOutput + } + return rawOutput + *errorHint +} + +// Returns the error hint corresponding to the FIRST match in raw output +func (errorHintGenerator *ErrorHintGenerator) getErrorHint(rawOutput string) *string { + firstMatch := errorHintGenerator.allErrorHintPatternsCompiled.FindString(rawOutput) + if _, found := errorHintGenerator.allErrorHints[firstMatch]; found { + errorHint := errorHintGenerator.allErrorHints[firstMatch] + return &errorHint + } + return nil +} diff --git a/ui/status/ninja_test.go b/ui/status/ninja_test.go index c400c97ee..f3638b323 100644 --- a/ui/status/ninja_test.go +++ b/ui/status/ninja_test.go @@ -43,3 +43,53 @@ func TestNinjaReader_Close(t *testing.T) { t.Errorf("nr.Close timed out, %s > %s", g, w) } } + +// Test that error hint is added to output if available +func TestNinjaReader_CorrectErrorHint(t *testing.T) { + errorPattern1 := "pattern-1 in input" + errorHint1 := "\n Fix by doing task 1" + errorPattern2 := "pattern-2 in input" + errorHint2 := "\n Fix by doing task 2" + mockErrorHints := make(map[string]string) + mockErrorHints[errorPattern1] = errorHint1 + mockErrorHints[errorPattern2] = errorHint2 + + errorHintGenerator := *newErrorHintGenerator(mockErrorHints) + testCases := []struct { + rawOutput string + buildExitCode int + expectedFinalOutput string + testCaseErrorMessage string + }{ + { + rawOutput: "ninja build was successful", + buildExitCode: 0, + expectedFinalOutput: "ninja build was successful", + testCaseErrorMessage: "raw output changed when build was successful", + }, + { + rawOutput: "ninja build failed", + buildExitCode: 1, + expectedFinalOutput: "ninja build failed", + testCaseErrorMessage: "raw output changed even when no error hint pattern was found", + }, + { + rawOutput: "ninja build failed: " + errorPattern1 + "some footnotes", + buildExitCode: 1, + expectedFinalOutput: "ninja build failed: " + errorPattern1 + "some footnotes" + errorHint1, + testCaseErrorMessage: "error hint not added despite pattern match", + }, + { + rawOutput: "ninja build failed: " + errorPattern2 + errorPattern1, + buildExitCode: 1, + expectedFinalOutput: "ninja build failed: " + errorPattern2 + errorPattern1 + errorHint2, + testCaseErrorMessage: "error hint should be added for first pattern match in raw output", + }, + } + for _, testCase := range testCases { + actualFinalOutput := errorHintGenerator.GetOutputWithErrorHint(testCase.rawOutput, testCase.buildExitCode) + if actualFinalOutput != testCase.expectedFinalOutput { + t.Errorf(testCase.testCaseErrorMessage+"\nexpected: %s\ngot: %s", testCase.expectedFinalOutput, actualFinalOutput) + } + } +}