Merge "Add errorHints to stdout when read-only file system errors are detected"

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 <<EOF
+genrule {
+  name: "write_to_source_tree",
+  out: ["write_to_source_tree"],
+  cmd: "touch file_in_source_tree && touch \$(out)",
+}
+EOF
+  readonly EXPECTED_OUT=out/soong/.intermediates/a/write_to_source_tree/gen/write_to_source_tree
+  readonly ERROR_LOG=${MOCK_TOP}/out/error.log
+  readonly ERROR_MSG="Read-only file system"
+  readonly ERROR_HINT_PATTERN="BUILD_BROKEN_SRC_DIR"
+  # Test in ReadOnly source tree
+  run_ninja BUILD_BROKEN_SRC_DIR_IS_WRITABLE=false ${EXPECTED_OUT} &> /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

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"
 

ui/build/config.go

@@ -161,6 +161,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",

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,
 		}
 

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 := <my/path/1> <my/path/2> #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
+}

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)
+		}
+	}
+}