From c087be1e8bde30fcadfcd4d76d15fb9dee120db4 Mon Sep 17 00:00:00 2001 From: Bill Peckham Date: Thu, 13 Feb 2020 15:55:10 -0800 Subject: [PATCH] Automate dependency on inputs of genrule module type. This change automates one of the best practices for custom tools. The best practice says "Prefer to list input files on the command line, otherwise we may not know to re-run your command when a new input file is added." [1] Normally you'd reference these inputs with $(in) or one of the forms of the $(location*) substituions on the command line for the custom tool. However, if the custom tool does not accept the list of inputs on the command line, the build system could fail to re-run the custom tool if the list changes. This change adds a hash of the list of input names to the sbox command that wraps the custom tool. If the list of inputs change, the hash will change, and therefore the sbox command will change, causing ninja to re-run the custom tool. The hash is visible to (but ignored by) the sbox command, and hidden from your custom tool. [1] https://android.googlesource.com/platform/build/soong/+/refs/heads/master/docs/best_practices.md#custom-build-tools Test: TestGenruleHashInputs Bug: 149397658 Change-Id: I18b547ea3c4296ee15bd6150a4778a8f376d80b7 --- cmd/sbox/sbox.go | 5 ++- genrule/genrule.go | 23 ++++++++++- genrule/genrule_test.go | 87 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/cmd/sbox/sbox.go b/cmd/sbox/sbox.go index 7057b33f0..65a34fdf4 100644 --- a/cmd/sbox/sbox.go +++ b/cmd/sbox/sbox.go @@ -36,6 +36,7 @@ var ( outputRoot string keepOutDir bool depfileOut string + inputHash string ) func init() { @@ -51,6 +52,8 @@ func init() { flag.StringVar(&depfileOut, "depfile-out", "", "file path of the depfile to generate. This value will replace '__SBOX_DEPFILE__' in the command and will be treated as an output but won't be added to __SBOX_OUT_FILES__") + flag.StringVar(&inputHash, "input-hash", "", + "This option is ignored. Typical usage is to supply a hash of the list of input names so that the module will be rebuilt if the list (and thus the hash) changes.") } func usageViolation(violation string) { @@ -59,7 +62,7 @@ func usageViolation(violation string) { } fmt.Fprintf(os.Stderr, - "Usage: sbox -c --sandbox-path --output-root [--depfile-out depFile] [...]\n"+ + "Usage: sbox -c --sandbox-path --output-root [--depfile-out depFile] [--input-hash hash] [...]\n"+ "\n"+ "Deletes ,"+ "runs ,"+ diff --git a/genrule/genrule.go b/genrule/genrule.go index a0008d329..fe877fe8d 100644 --- a/genrule/genrule.go +++ b/genrule/genrule.go @@ -26,6 +26,7 @@ import ( "android/soong/android" "android/soong/shared" + "crypto/sha256" "path/filepath" ) @@ -309,6 +310,7 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) { addLocationLabel(out.Rel(), []string{filepath.Join("__SBOX_OUT_DIR__", out.Rel())}) } + referencedIn := false referencedDepfile := false rawCommand, err := android.ExpandNinjaEscaped(task.cmd, func(name string) (string, bool, error) { @@ -333,6 +335,7 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) { } return locationLabels[firstLabel][0], false, nil case "in": + referencedIn = true return "${in}", true, nil case "out": return "__SBOX_OUT_FILES__", false, nil @@ -398,8 +401,16 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) { // Escape the command for the shell rawCommand = "'" + strings.Replace(rawCommand, "'", `'\''`, -1) + "'" g.rawCommands = append(g.rawCommands, rawCommand) - sandboxCommand := fmt.Sprintf("rm -rf %s && $sboxCmd --sandbox-path %s --output-root %s -c %s %s $allouts", - task.genDir, sandboxPath, task.genDir, rawCommand, depfilePlaceholder) + + sandboxCommand := fmt.Sprintf("rm -rf %s && $sboxCmd --sandbox-path %s --output-root %s", + task.genDir, sandboxPath, task.genDir) + + if !referencedIn { + sandboxCommand = sandboxCommand + hashSrcFiles(srcFiles) + } + + sandboxCommand = sandboxCommand + fmt.Sprintf(" -c %s %s $allouts", + rawCommand, depfilePlaceholder) ruleParams := blueprint.RuleParams{ Command: sandboxCommand, @@ -463,6 +474,14 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) { } +func hashSrcFiles(srcFiles android.Paths) string { + h := sha256.New() + for _, src := range srcFiles { + h.Write([]byte(src.String())) + } + return fmt.Sprintf(" --input-hash %x", h.Sum(nil)) +} + func (g *Module) generateSourceFile(ctx android.ModuleContext, task generateTask, rule blueprint.Rule) { desc := "generate" if len(task.out) == 0 { diff --git a/genrule/genrule_test.go b/genrule/genrule_test.go index 7eb43ac9d..4b36600a6 100644 --- a/genrule/genrule_test.go +++ b/genrule/genrule_test.go @@ -502,6 +502,93 @@ func TestGenruleCmd(t *testing.T) { } } +func TestGenruleHashInputs(t *testing.T) { + + // The basic idea here is to verify that the sbox command (which is + // in the Command field of the generate rule) contains a hash of the + // inputs, but only if $(in) is not referenced in the genrule cmd + // property. + + // By including a hash of the inputs, we cause the rule to re-run if + // the list of inputs changes (because the sbox command changes). + + // However, if the genrule cmd property already contains $(in), then + // the dependency is already expressed, so we don't need to include the + // hash in that case. + + bp := ` + genrule { + name: "hash0", + srcs: ["in1.txt", "in2.txt"], + out: ["out"], + cmd: "echo foo > $(out)", + } + genrule { + name: "hash1", + srcs: ["*.txt"], + out: ["out"], + cmd: "echo bar > $(out)", + } + genrule { + name: "hash2", + srcs: ["*.txt"], + out: ["out"], + cmd: "echo $(in) > $(out)", + } + ` + testcases := []struct { + name string + expectedHash string + }{ + { + name: "hash0", + // sha256 value obtained from: echo -n 'in1.txtin2.txt' | sha256sum + expectedHash: "031097e11e0a8c822c960eb9742474f46336360a515744000d086d94335a9cb9", + }, + { + name: "hash1", + // sha256 value obtained from: echo -n 'in1.txtin2.txtin3.txt' | sha256sum + expectedHash: "de5d22a4a7ab50d250cc59fcdf7a7e0775790d270bfca3a7a9e1f18a70dd996c", + }, + { + name: "hash2", + // $(in) is present, option should not appear + expectedHash: "", + }, + } + + config := testConfig(bp, nil) + ctx := testContext(config) + _, errs := ctx.ParseFileList(".", []string{"Android.bp"}) + if errs == nil { + _, errs = ctx.PrepareBuildActions(config) + } + if errs != nil { + t.Fatal(errs) + } + + for _, test := range testcases { + t.Run(test.name, func(t *testing.T) { + gen := ctx.ModuleForTests(test.name, "") + command := gen.Rule("generator").RuleParams.Command + + if len(test.expectedHash) > 0 { + // We add spaces before and after to make sure that + // this option doesn't abutt another sbox option. + expectedInputHashOption := " --input-hash " + test.expectedHash + " " + + if !strings.Contains(command, expectedInputHashOption) { + t.Errorf("Expected command \"%s\" to contain \"%s\"", command, expectedInputHashOption) + } + } else { + if strings.Contains(command, "--input-hash") { + t.Errorf("Unexpected \"--input-hash\" found in command: \"%s\"", command) + } + } + }) + } +} + func TestGenSrcs(t *testing.T) { testcases := []struct { name string