diff --git a/android/rule_builder.go b/android/rule_builder.go index 098c1fcc4..11da36cc0 100644 --- a/android/rule_builder.go +++ b/android/rule_builder.go @@ -101,12 +101,7 @@ func (r *RuleBuilder) MissingDeps(missingDeps []string) { } // Restat marks the rule as a restat rule, which will be passed to ModuleContext.Rule in BuildParams.Restat. -// -// Restat is not compatible with Sbox() func (r *RuleBuilder) Restat() *RuleBuilder { - if r.sbox { - panic("Restat() is not compatible with Sbox()") - } r.restat = true return r } @@ -141,8 +136,6 @@ func (r *RuleBuilder) Rewrapper(params *remoteexec.REParams) *RuleBuilder { // point to a location where sbox's manifest will be written and must be outside outputDir. sbox // will ensure that all outputs have been written, and will discard any output files that were not // specified. -// -// Sbox is not compatible with Restat() func (r *RuleBuilder) Sbox(outputDir WritablePath, manifestPath WritablePath) *RuleBuilder { if r.sbox { panic("Sbox() may not be called more than once") @@ -150,9 +143,6 @@ func (r *RuleBuilder) Sbox(outputDir WritablePath, manifestPath WritablePath) *R if len(r.commands) > 0 { panic("Sbox() may not be called after Command()") } - if r.restat { - panic("Sbox() is not compatible with Restat()") - } r.sbox = true r.outDir = outputDir r.sboxManifestPath = manifestPath @@ -636,11 +626,14 @@ func (r *RuleBuilder) Build(name string, desc string) { ctx: r.ctx, }, } - sboxCmd.Text("rm -rf").Output(r.outDir) - sboxCmd.Text("&&") sboxCmd.builtToolWithoutDeps("sbox"). - Flag("--sandbox-path").Text(shared.TempDirForOutDir(PathForOutput(r.ctx).String())). - Flag("--manifest").Input(r.sboxManifestPath) + FlagWithArg("--sandbox-path ", shared.TempDirForOutDir(PathForOutput(r.ctx).String())). + FlagWithArg("--output-dir ", r.outDir.String()). + FlagWithInput("--manifest ", r.sboxManifestPath) + + if r.restat { + sboxCmd.Flag("--write-if-changed") + } // Replace the command string, and add the sbox tool and manifest textproto to the // dependencies of the final sbox rule. diff --git a/android/rule_builder_test.go b/android/rule_builder_test.go index 3766bb03b..86647eb22 100644 --- a/android/rule_builder_test.go +++ b/android/rule_builder_test.go @@ -678,32 +678,32 @@ func TestRuleBuilder_Build(t *testing.T) { }) t.Run("sbox", func(t *testing.T) { outDir := "out/soong/.intermediates/foo_sbox" - outFile := filepath.Join(outDir, "gen/foo_sbox") - depFile := filepath.Join(outDir, "gen/foo_sbox.d") + sboxOutDir := filepath.Join(outDir, "gen") + outFile := filepath.Join(sboxOutDir, "foo_sbox") + depFile := filepath.Join(sboxOutDir, "foo_sbox.d") rspFile := filepath.Join(outDir, "rsp") rspFile2 := filepath.Join(outDir, "rsp2") manifest := filepath.Join(outDir, "sbox.textproto") sbox := filepath.Join("out", "soong", "host", result.Config.PrebuiltOS(), "bin/sbox") sandboxPath := shared.TempDirForOutDir("out/soong") - cmd := `rm -rf ` + outDir + `/gen && ` + - sbox + ` --sandbox-path ` + sandboxPath + ` --manifest ` + manifest + cmd := sbox + ` --sandbox-path ` + sandboxPath + ` --output-dir ` + sboxOutDir + ` --manifest ` + manifest module := result.ModuleForTests("foo_sbox", "") check(t, module.Output("gen/foo_sbox"), module.Output(rspFile2), cmd, outFile, depFile, rspFile, rspFile2, false, []string{manifest}, []string{sbox}) }) t.Run("sbox_inputs", func(t *testing.T) { outDir := "out/soong/.intermediates/foo_sbox_inputs" - outFile := filepath.Join(outDir, "gen/foo_sbox_inputs") - depFile := filepath.Join(outDir, "gen/foo_sbox_inputs.d") + sboxOutDir := filepath.Join(outDir, "gen") + outFile := filepath.Join(sboxOutDir, "foo_sbox_inputs") + depFile := filepath.Join(sboxOutDir, "foo_sbox_inputs.d") rspFile := filepath.Join(outDir, "rsp") rspFile2 := filepath.Join(outDir, "rsp2") manifest := filepath.Join(outDir, "sbox.textproto") sbox := filepath.Join("out", "soong", "host", result.Config.PrebuiltOS(), "bin/sbox") sandboxPath := shared.TempDirForOutDir("out/soong") - cmd := `rm -rf ` + outDir + `/gen && ` + - sbox + ` --sandbox-path ` + sandboxPath + ` --manifest ` + manifest + cmd := sbox + ` --sandbox-path ` + sandboxPath + ` --output-dir ` + sboxOutDir + ` --manifest ` + manifest module := result.ModuleForTests("foo_sbox_inputs", "") check(t, module.Output("gen/foo_sbox_inputs"), module.Output(rspFile2), diff --git a/cmd/sbox/sbox.go b/cmd/sbox/sbox.go index 4fa748660..418826c5c 100644 --- a/cmd/sbox/sbox.go +++ b/cmd/sbox/sbox.go @@ -38,9 +38,11 @@ import ( ) var ( - sandboxesRoot string - manifestFile string - keepOutDir bool + sandboxesRoot string + outputDir string + manifestFile string + keepOutDir bool + writeIfChanged bool ) const ( @@ -51,10 +53,14 @@ const ( func init() { flag.StringVar(&sandboxesRoot, "sandbox-path", "", "root of temp directory to put the sandbox into") + flag.StringVar(&outputDir, "output-dir", "", + "directory which will contain all output files and only output files") flag.StringVar(&manifestFile, "manifest", "", "textproto manifest describing the sandboxed command(s)") flag.BoolVar(&keepOutDir, "keep-out-dir", false, "whether to keep the sandbox directory when done") + flag.BoolVar(&writeIfChanged, "write-if-changed", false, + "only write the output files if they have changed") } func usageViolation(violation string) { @@ -241,6 +247,12 @@ func runCommand(command *sbox_proto.Command, tempDir string, commandIndex int) ( return "", fmt.Errorf("command is required") } + // Remove files from the output directory + err = clearOutputDirectory(command.CopyAfter, outputDir, writeType(writeIfChanged)) + if err != nil { + return "", err + } + pathToTempDirInSbox := tempDir if command.GetChdir() { pathToTempDirInSbox = "." @@ -252,7 +264,7 @@ func runCommand(command *sbox_proto.Command, tempDir string, commandIndex int) ( } // Copy in any files specified by the manifest. - err = copyFiles(command.CopyBefore, "", tempDir, false) + err = copyFiles(command.CopyBefore, "", tempDir, requireFromExists, alwaysWrite) if err != nil { return "", err } @@ -306,7 +318,7 @@ func runCommand(command *sbox_proto.Command, tempDir string, commandIndex int) ( // 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) + copyFiles(command.CopyAfter, tempDir, "", allowFromNotExists, writeType(writeIfChanged)) } // If the command was executed but failed with an error, print a debugging message before @@ -327,39 +339,16 @@ func runCommand(command *sbox_proto.Command, tempDir string, commandIndex int) ( return "", err } - missingOutputErrors := validateOutputFiles(command.CopyAfter, tempDir) - - if len(missingOutputErrors) > 0 { - // find all created files for making a more informative error message - createdFiles := findAllFilesUnder(tempDir) - - // build error message - errorMessage := "mismatch between declared and actual outputs\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 { - errorMessage += " " + missingOutputError.Error() + "\n" - } - if len(createdFiles) < 1 { - errorMessage += "created 0 files." - } else { - errorMessage += fmt.Sprintf("did create %v files:\n", len(createdFiles)) - creationMessages := createdFiles - maxNumCreationLines := 10 - if len(creationMessages) > maxNumCreationLines { - creationMessages = creationMessages[:maxNumCreationLines] - creationMessages = append(creationMessages, fmt.Sprintf("...%v more", len(createdFiles)-maxNumCreationLines)) - } - for _, creationMessage := range creationMessages { - errorMessage += " " + creationMessage + "\n" - } - } - - return "", errors.New(errorMessage) + err = validateOutputFiles(command.CopyAfter, tempDir, outputDir, rawCommand) + if err != nil { + return "", err } + // the created files match the declared files; now move them - err = moveFiles(command.CopyAfter, tempDir, "") + err = moveFiles(command.CopyAfter, tempDir, "", writeType(writeIfChanged)) + if err != nil { + return "", err + } return depFile, nil } @@ -380,8 +369,9 @@ func makeOutputDirs(copies []*sbox_proto.Copy, sandboxDir string) error { // validateOutputFiles verifies that all files that have a rule to be copied out of the sandbox // were created by the command. -func validateOutputFiles(copies []*sbox_proto.Copy, sandboxDir string) []error { +func validateOutputFiles(copies []*sbox_proto.Copy, sandboxDir, outputDir, rawCommand string) error { var missingOutputErrors []error + var incorrectOutputDirectoryErrors []error for _, copyPair := range copies { fromPath := joinPath(sandboxDir, copyPair.GetFrom()) fileInfo, err := os.Stat(fromPath) @@ -392,17 +382,91 @@ func validateOutputFiles(copies []*sbox_proto.Copy, sandboxDir string) []error { if fileInfo.IsDir() { missingOutputErrors = append(missingOutputErrors, fmt.Errorf("%s: not a file", fromPath)) } + + toPath := copyPair.GetTo() + if rel, err := filepath.Rel(outputDir, toPath); err != nil { + return err + } else if strings.HasPrefix(rel, "../") { + incorrectOutputDirectoryErrors = append(incorrectOutputDirectoryErrors, + fmt.Errorf("%s is not under %s", toPath, outputDir)) + } } - return missingOutputErrors + + const maxErrors = 10 + + if len(incorrectOutputDirectoryErrors) > 0 { + errorMessage := "" + more := 0 + if len(incorrectOutputDirectoryErrors) > maxErrors { + more = len(incorrectOutputDirectoryErrors) - maxErrors + incorrectOutputDirectoryErrors = incorrectOutputDirectoryErrors[:maxErrors] + } + + for _, err := range incorrectOutputDirectoryErrors { + errorMessage += err.Error() + "\n" + } + if more > 0 { + errorMessage += fmt.Sprintf("...%v more", more) + } + + return errors.New(errorMessage) + } + + if len(missingOutputErrors) > 0 { + // find all created files for making a more informative error message + createdFiles := findAllFilesUnder(sandboxDir) + + // build error message + errorMessage := "mismatch between declared and actual outputs\n" + errorMessage += "in sbox command(" + rawCommand + ")\n\n" + errorMessage += "in sandbox " + sandboxDir + ",\n" + errorMessage += fmt.Sprintf("failed to create %v files:\n", len(missingOutputErrors)) + for _, missingOutputError := range missingOutputErrors { + errorMessage += " " + missingOutputError.Error() + "\n" + } + if len(createdFiles) < 1 { + errorMessage += "created 0 files." + } else { + errorMessage += fmt.Sprintf("did create %v files:\n", len(createdFiles)) + creationMessages := createdFiles + if len(creationMessages) > maxErrors { + creationMessages = creationMessages[:maxErrors] + creationMessages = append(creationMessages, fmt.Sprintf("...%v more", len(createdFiles)-maxErrors)) + } + for _, creationMessage := range creationMessages { + errorMessage += " " + creationMessage + "\n" + } + } + + return errors.New(errorMessage) + } + + return nil } -// 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 { +type existsType bool + +const ( + requireFromExists existsType = false + allowFromNotExists = true +) + +type writeType bool + +const ( + alwaysWrite writeType = false + onlyWriteIfChanged = true +) + +// copyFiles copies files in or out of the sandbox. If exists is allowFromNotExists then errors +// caused by a from path not existing are ignored. If write is onlyWriteIfChanged then the output +// file is compared to the input file and not written to if it is the same, avoiding updating +// the timestamp. +func copyFiles(copies []*sbox_proto.Copy, fromDir, toDir string, exists existsType, write writeType) error { for _, copyPair := range copies { fromPath := joinPath(fromDir, copyPair.GetFrom()) toPath := joinPath(toDir, copyPair.GetTo()) - err := copyOneFile(fromPath, toPath, copyPair.GetExecutable(), allowFromNotExists) + err := copyOneFile(fromPath, toPath, copyPair.GetExecutable(), exists, write) if err != nil { return fmt.Errorf("error copying %q to %q: %w", fromPath, toPath, err) } @@ -411,8 +475,11 @@ func copyFiles(copies []*sbox_proto.Copy, fromDir, toDir string, allowFromNotExi } // 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 { +// permissions. If exists is allowFromNotExists it returns nil if the from path doesn't exist. +// If write is onlyWriteIfChanged then the output file is compared to the input file and not written to +// if it is the same, avoiding updating the timestamp. +func copyOneFile(from string, to string, forceExecutable bool, exists existsType, + write writeType) error { err := os.MkdirAll(filepath.Dir(to), 0777) if err != nil { return err @@ -420,7 +487,7 @@ func copyOneFile(from string, to string, forceExecutable, allowFromNotExists boo stat, err := os.Stat(from) if err != nil { - if os.IsNotExist(err) && allowFromNotExists { + if os.IsNotExist(err) && exists == allowFromNotExists { return nil } return err @@ -431,6 +498,10 @@ func copyOneFile(from string, to string, forceExecutable, allowFromNotExists boo perm = perm | 0100 // u+x } + if write == onlyWriteIfChanged && filesHaveSameContents(from, to) { + return nil + } + in, err := os.Open(from) if err != nil { return err @@ -504,7 +575,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, false) + err := copyOneFile(from, joinPath(toDir, to), false, requireFromExists, alwaysWrite) if err != nil { return err } @@ -551,9 +622,10 @@ func applyPathMappings(pathMappings []*sbox_proto.PathMapping, path string) stri // moveFiles moves files specified by a set of copy rules. It uses os.Rename, so it is restricted // to moving files where the source and destination are in the same filesystem. This is OK for -// sbox because the temporary directory is inside the out directory. It updates the timestamp -// of the new file. -func moveFiles(copies []*sbox_proto.Copy, fromDir, toDir string) error { +// sbox because the temporary directory is inside the out directory. If write is onlyWriteIfChanged +// then the output file is compared to the input file and not written to if it is the same, avoiding +// updating the timestamp. Otherwise it always updates the timestamp of the new file. +func moveFiles(copies []*sbox_proto.Copy, fromDir, toDir string, write writeType) error { for _, copyPair := range copies { fromPath := joinPath(fromDir, copyPair.GetFrom()) toPath := joinPath(toDir, copyPair.GetTo()) @@ -562,6 +634,10 @@ func moveFiles(copies []*sbox_proto.Copy, fromDir, toDir string) error { return err } + if write == onlyWriteIfChanged && filesHaveSameContents(fromPath, toPath) { + continue + } + err = os.Rename(fromPath, toPath) if err != nil { return err @@ -578,6 +654,37 @@ func moveFiles(copies []*sbox_proto.Copy, fromDir, toDir string) error { return nil } +// clearOutputDirectory removes all files in the output directory if write is alwaysWrite, or +// any files not listed in copies if write is onlyWriteIfChanged +func clearOutputDirectory(copies []*sbox_proto.Copy, outputDir string, write writeType) error { + if outputDir == "" { + return fmt.Errorf("output directory must be set") + } + + if write == alwaysWrite { + // When writing all the output files remove the whole output directory + return os.RemoveAll(outputDir) + } + + outputFiles := make(map[string]bool, len(copies)) + for _, copyPair := range copies { + outputFiles[copyPair.GetTo()] = true + } + + existingFiles := findAllFilesUnder(outputDir) + for _, existingFile := range existingFiles { + fullExistingFile := filepath.Join(outputDir, existingFile) + if !outputFiles[fullExistingFile] { + err := os.Remove(fullExistingFile) + if err != nil { + return fmt.Errorf("failed to remove obsolete output file %s: %w", fullExistingFile, err) + } + } + } + + return nil +} + // Rewrite one or more depfiles so that it doesn't include the (randomized) sandbox directory // to an output file. func rewriteDepFiles(ins []string, out string) error { @@ -621,6 +728,66 @@ func joinPath(dir, file string) string { return filepath.Join(dir, file) } +// filesHaveSameContents compares the contents if two files, returning true if they are the same +// and returning false if they are different or any errors occur. +func filesHaveSameContents(a, b string) bool { + // Compare the sizes of the two files + statA, err := os.Stat(a) + if err != nil { + return false + } + statB, err := os.Stat(b) + if err != nil { + return false + } + + if statA.Size() != statB.Size() { + return false + } + + // Open the two files + fileA, err := os.Open(a) + if err != nil { + return false + } + defer fileA.Close() + fileB, err := os.Open(a) + if err != nil { + return false + } + defer fileB.Close() + + // Compare the files 1MB at a time + const bufSize = 1 * 1024 * 1024 + bufA := make([]byte, bufSize) + bufB := make([]byte, bufSize) + + remain := statA.Size() + for remain > 0 { + toRead := int64(bufSize) + if toRead > remain { + toRead = remain + } + + _, err = io.ReadFull(fileA, bufA[:toRead]) + if err != nil { + return false + } + _, err = io.ReadFull(fileB, bufB[:toRead]) + if err != nil { + return false + } + + if bytes.Compare(bufA[:toRead], bufB[:toRead]) != 0 { + return false + } + + remain -= toRead + } + + return true +} + func makeAbsPathEnv(pathEnv string) (string, error) { pathEnvElements := filepath.SplitList(pathEnv) for i, p := range pathEnvElements { diff --git a/java/droidstubs.go b/java/droidstubs.go index 2921c3e82..c5b56f76d 100644 --- a/java/droidstubs.go +++ b/java/droidstubs.go @@ -433,6 +433,10 @@ func (d *Droidstubs) apiLevelsAnnotationsFlags(ctx android.ModuleContext, cmd *a } } +func metalavaUseRbe(ctx android.ModuleContext) bool { + return ctx.Config().UseRBE() && ctx.Config().IsEnvTrue("RBE_METALAVA") +} + func metalavaCmd(ctx android.ModuleContext, rule *android.RuleBuilder, javaVersion javaVersion, srcs android.Paths, srcJarList android.Path, bootclasspath, classpath classpath, homeDir android.WritablePath) *android.RuleBuilderCommand { rule.Command().Text("rm -rf").Flag(homeDir.String()) @@ -441,7 +445,7 @@ func metalavaCmd(ctx android.ModuleContext, rule *android.RuleBuilder, javaVersi cmd := rule.Command() cmd.FlagWithArg("ANDROID_PREFS_ROOT=", homeDir.String()) - if ctx.Config().UseRBE() && ctx.Config().IsEnvTrue("RBE_METALAVA") { + if metalavaUseRbe(ctx) { rule.Remoteable(android.RemoteRuleSupports{RBE: true}) execStrategy := ctx.Config().GetenvWithDefault("RBE_METALAVA_EXEC_STRATEGY", remoteexec.LocalExecStrategy) labels := map[string]string{"type": "tool", "name": "metalava"} @@ -665,7 +669,9 @@ func (d *Droidstubs) GenerateAndroidBuildActions(ctx android.ModuleContext) { } // TODO(b/183630617): rewrapper doesn't support restat rules - // rule.Restat() + if !metalavaUseRbe(ctx) { + rule.Restat() + } zipSyncCleanupCmd(rule, srcJarDir)