Parcourir la source

Enable restat for sbox rules

Add support to sbox for only writing output files if they are changed
to support enabling restat.  Move the clearing of the output directory
into sbox, and when write if changed is enabled delete obsolete output
files without deleting output files that will be replaced.

Enable restat for local metalava rules.  Restat for metalava rules
run remotely is still blocked on b/183630617.

Bug: 222095735
Test: TestRuleBuilder_Build
Test: m SystemUI
Change-Id: If8fc47e711f4a6f43947ab69f17cccc8ba6ae61a
Colin Cross il y a 2 ans
Parent
commit
e52c2ac7fb
4 fichiers modifiés avec 239 ajouts et 73 suppressions
  1. 7 14
      android/rule_builder.go
  2. 8 8
      android/rule_builder_test.go
  3. 216 49
      cmd/sbox/sbox.go
  4. 8 2
      java/droidstubs.go

+ 7 - 14
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.

+ 8 - 8
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),

+ 216 - 49
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))
+		}
+	}
+
+	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)
 	}
-	return missingOutputErrors
+
+	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 {

+ 8 - 2
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)