From fc2d842dfd35756867e942aa19800708c31683ed Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 15 Apr 2021 18:36:42 -0700 Subject: [PATCH 1/3] sbox: best-effort copy output files on failure Error messages printed by failing commands may reference output files that were created by the command, for example printing a command line to copy and paste to update a baseline file. Copy output files in the sandbox to their final locations, ignoring missing files, so that the messages are valid. Bug: 185516277 Test: m out/soong/.intermediates/frameworks/base/system-api-stubs-docs-non-updatable/android_common/metalava/api_lint.timestamp with lint error Change-Id: I604a11c9b54e409ca5bc5c016cd04b3133f74a60 --- cmd/sbox/sbox.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/cmd/sbox/sbox.go b/cmd/sbox/sbox.go index 7bd086882..9736ff6cf 100644 --- a/cmd/sbox/sbox.go +++ b/cmd/sbox/sbox.go @@ -230,7 +230,7 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er } // Copy in any files specified by the manifest. - err = copyFiles(command.CopyBefore, "", tempDir) + err = copyFiles(command.CopyBefore, "", tempDir, false) if err != nil { return "", err } @@ -276,6 +276,14 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er } err = cmd.Run() + if err != nil { + // The command failed, do a best effort copy of output files out of the sandbox. This is + // especially useful for linters with baselines that print an error message on failure + // with a command to copy the output lint errors to the new baseline. Use a copy instead of + // a move to leave the sandbox intact for manual inspection + copyFiles(command.CopyAfter, tempDir, "", true) + } + if exit, ok := err.(*exec.ExitError); ok && !exit.Success() { return "", fmt.Errorf("sbox command failed with err:\n%s\n%w\n", commandDescription, err) } else if err != nil { @@ -351,12 +359,13 @@ func validateOutputFiles(copies []*sbox_proto.Copy, sandboxDir string) []error { return missingOutputErrors } -// copyFiles copies files in or out of the sandbox. -func copyFiles(copies []*sbox_proto.Copy, fromDir, toDir string) error { +// copyFiles copies files in or out of the sandbox. If allowFromNotExists is true then errors +// caused by a from path not existing are ignored. +func copyFiles(copies []*sbox_proto.Copy, fromDir, toDir string, allowFromNotExists bool) error { for _, copyPair := range copies { fromPath := joinPath(fromDir, copyPair.GetFrom()) toPath := joinPath(toDir, copyPair.GetTo()) - err := copyOneFile(fromPath, toPath, copyPair.GetExecutable()) + err := copyOneFile(fromPath, toPath, copyPair.GetExecutable(), allowFromNotExists) if err != nil { return fmt.Errorf("error copying %q to %q: %w", fromPath, toPath, err) } @@ -364,8 +373,9 @@ func copyFiles(copies []*sbox_proto.Copy, fromDir, toDir string) error { return nil } -// copyOneFile copies a file. -func copyOneFile(from string, to string, executable bool) error { +// copyOneFile copies a file and its permissions. If forceExecutable is true it adds u+x to the +// permissions. If allowFromNotExists is true it returns nil if the from path doesn't exist. +func copyOneFile(from string, to string, forceExecutable, allowFromNotExists bool) error { err := os.MkdirAll(filepath.Dir(to), 0777) if err != nil { return err @@ -373,11 +383,14 @@ func copyOneFile(from string, to string, executable bool) error { stat, err := os.Stat(from) if err != nil { + if os.IsNotExist(err) && allowFromNotExists { + return nil + } return err } perm := stat.Mode() - if executable { + if forceExecutable { perm = perm | 0100 // u+x } @@ -454,7 +467,7 @@ func copyOneRspFile(rspFile *sbox_proto.RspFile, toDir, toDirInSandbox string) e to := applyPathMappings(rspFile.PathMappings, from) // Copy the file into the sandbox. - err := copyOneFile(from, joinPath(toDir, to), false) + err := copyOneFile(from, joinPath(toDir, to), false, false) if err != nil { return err } From 4258a39bd155d3e776eb041ea7c1eff5e3c2495f Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 15 Apr 2021 18:50:11 -0700 Subject: [PATCH 2/3] sbox: print failing command line before output The full command line run inside sbox can be very long, and printing it after the errors printed by the failing command can hide the error messages. Buffer the output of the command and print the failing command line before the output if it fails. Bug: 185516277 Test: m out/soong/.intermediates/frameworks/base/system-api-stubs-docs-non-updatable/android_common/metalava/api_lint.timestamp with lint error Change-Id: I893f3dd01f1baf195e182111c5c49e92eb82f3b0 --- cmd/sbox/sbox.go | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/cmd/sbox/sbox.go b/cmd/sbox/sbox.go index 9736ff6cf..f124e40b0 100644 --- a/cmd/sbox/sbox.go +++ b/cmd/sbox/sbox.go @@ -255,12 +255,11 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er return "", err } - commandDescription := rawCommand - cmd := exec.Command("bash", "-c", rawCommand) + buf := &bytes.Buffer{} cmd.Stdin = os.Stdin - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + cmd.Stdout = buf + cmd.Stderr = buf if command.GetChdir() { cmd.Dir = tempDir @@ -284,9 +283,21 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er copyFiles(command.CopyAfter, tempDir, "", true) } + // If the command was executed but failed with an error, print a debugging message before + // the command's output so it doesn't scroll the real error message off the screen. if exit, ok := err.(*exec.ExitError); ok && !exit.Success() { - return "", fmt.Errorf("sbox command failed with err:\n%s\n%w\n", commandDescription, err) - } else if err != nil { + fmt.Fprintf(os.Stderr, + "The failing command was run inside an sbox sandbox in temporary directory\n"+ + "%s\n"+ + "The failing command line was:\n"+ + "%s\n", + tempDir, rawCommand) + } + + // Write the command's combined stdout/stderr. + os.Stdout.Write(buf.Bytes()) + + if err != nil { return "", err } @@ -298,7 +309,7 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er // build error message errorMessage := "mismatch between declared and actual outputs\n" - errorMessage += "in sbox command(" + commandDescription + ")\n\n" + errorMessage += "in sbox command(" + rawCommand + ")\n\n" errorMessage += "in sandbox " + tempDir + ",\n" errorMessage += fmt.Sprintf("failed to create %v files:\n", len(missingOutputErrors)) for _, missingOutputError := range missingOutputErrors { From 63eeda027c499b1c5f2d83c2e4a24ae8871ce7a4 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 15 Apr 2021 19:01:57 -0700 Subject: [PATCH 3/3] Fix metalava api baseline update command metalava is run inside sbox with a modified $PWD, so putting $PWD in the output message results in an incorrect path. It was also always incorrect when the output directory was an absolute path. Add a cd $ANDROID_BUILD_TOP to the command line and use relative paths instead. Bug: 185516277 Test: m out/soong/.intermediates/frameworks/base/system-api-stubs-docs-non-updatable/android_common/metalava/api_lint.timestamp with lint error Change-Id: Iefe133cea4c3a604ecd2b0ea20f4ba14ae13b425 --- java/droidstubs.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/java/droidstubs.go b/java/droidstubs.go index a9e2749a5..2676f3d6c 100644 --- a/java/droidstubs.go +++ b/java/droidstubs.go @@ -516,9 +516,6 @@ func (d *Droidstubs) GenerateAndroidBuildActions(ctx android.ModuleContext) { d.apiLintTimestamp = android.PathForModuleOut(ctx, "metalava", "api_lint.timestamp") // Note this string includes a special shell quote $' ... ', which decodes the "\n"s. - // However, because $' ... ' doesn't expand environmental variables, we can't just embed - // $PWD, so we have to terminate $'...', use "$PWD", then start $' ... ' again, - // which is why we have '"$PWD"$' in it. // // TODO: metalava also has a slightly different message hardcoded. Should we unify this // message and metalava's one? @@ -539,9 +536,9 @@ func (d *Droidstubs) GenerateAndroidBuildActions(ctx android.ModuleContext) { msg += fmt.Sprintf(``+ `2. You can update the baseline by executing the following\n`+ ` command:\n`+ - ` cp \\\n`+ - ` "'"$PWD"$'/%s" \\\n`+ - ` "'"$PWD"$'/%s"\n`+ + ` (cd $ANDROID_BUILD_TOP && cp \\\n`+ + ` "%s" \\\n`+ + ` "%s")\n`+ ` To submit the revised baseline.txt to the main Android\n`+ ` repository, you will need approval.\n`, updatedBaselineOutput, baselineFile.Path()) } else {