From 78f3c3a3ad7a57ed49d651983b7a71fa4e4fe16d Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Thu, 15 Aug 2024 17:19:34 -0700 Subject: [PATCH] Add $(build_number_file) support to genrules Genrules that set uses_order_only_build_number_file: true will now have the ability to reference a $(build_number_file) label that will point to the build number file. It will also caused the build number file to be added as an order-only dependency, which will make it show up in the genrule sandbox. This is needed for converting make code that references the build number to soong, and for sandboxing the remaining unsandboxed genrules that reference the build number. Bug: 341873065 Test: m nothing --no-skip-soong-tests Change-Id: I9092cbb0eb39c5449a79f0ee40a4202262cef206 --- android/rule_builder.go | 6 ++++ genrule/genrule.go | 35 +++++++++++++++++++++++ genrule/genrule_test.go | 63 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) diff --git a/android/rule_builder.go b/android/rule_builder.go index 8b03124bf..464aca4a0 100644 --- a/android/rule_builder.go +++ b/android/rule_builder.go @@ -554,6 +554,12 @@ func (r *RuleBuilder) build(name string, desc string, ninjaEscapeCommandString b To: proto.String(r.sboxPathForInputRel(input)), }) } + for _, input := range r.OrderOnlys() { + command.CopyBefore = append(command.CopyBefore, &sbox_proto.Copy{ + From: proto.String(input.String()), + To: proto.String(r.sboxPathForInputRel(input)), + }) + } // If using rsp files copy them and their contents into the sbox directory with // the appropriate path mappings. diff --git a/genrule/genrule.go b/genrule/genrule.go index 25579220d..919564399 100644 --- a/genrule/genrule.go +++ b/genrule/genrule.go @@ -147,6 +147,21 @@ type generatorProperties struct { // Enable restat to update the output only if the output is changed Write_if_changed *bool + + // When set to true, an additional $(build_number_file) label will be available + // to use in the cmd. This will be the location of a text file containing the + // build number. The dependency on this file will be "order-only", meaning that + // the genrule will not rerun when only this file changes, to avoid rerunning + // the genrule every build, because the build number changes every build. + // This also means that you should not attempt to consume the build number from + // the result of this genrule in another build rule. If you do, the build number + // in the second build rule will be stale when the second build rule rebuilds + // but this genrule does not. Only certain allowlisted modules are allowed to + // use this property, usages of the build number should be kept to the absolute + // minimum. Particularly no modules on the system image may include the build + // number. Prefer using libbuildversion via the use_version_lib property on + // cc modules. + Uses_order_only_build_number_file *bool } type Module struct { @@ -228,6 +243,15 @@ func toolDepsMutator(ctx android.BottomUpMutatorContext) { } } +// This allowlist should be kept to the bare minimum, it's +// intended for things that existed before the build number +// was tightly controlled. Prefer using libbuildversion +// via the use_version_lib property of cc modules. +var genrule_build_number_allowlist = map[string]bool{ + "build/soong/tests:gen": true, + "tools/tradefederation/core:tradefed_zip": true, +} + // generateCommonBuildActions contains build action generation logic // common to both the mixed build case and the legacy case of genrule processing. // To fully support genrule in mixed builds, the contents of this function should @@ -470,6 +494,11 @@ func (g *Module) generateCommonBuildActions(ctx android.ModuleContext) { return strings.Join(proptools.ShellEscapeList(sandboxOuts), " "), nil case "genDir": return proptools.ShellEscape(cmd.PathForOutput(task.genDir)), nil + case "build_number_file": + if !proptools.Bool(g.properties.Uses_order_only_build_number_file) { + return reportError("to use the $(build_number_file) label, you must set uses_order_only_build_number_file: true") + } + return proptools.ShellEscape(cmd.PathForInput(ctx.Config().BuildNumberFile(ctx))), nil default: if strings.HasPrefix(name, "location ") { label := strings.TrimSpace(strings.TrimPrefix(name, "location ")) @@ -516,6 +545,12 @@ func (g *Module) generateCommonBuildActions(ctx android.ModuleContext) { cmd.Implicits(task.in) cmd.ImplicitTools(tools) cmd.ImplicitPackagedTools(packagedTools) + if proptools.Bool(g.properties.Uses_order_only_build_number_file) { + if _, ok := genrule_build_number_allowlist[ctx.ModuleDir()+":"+ctx.ModuleName()]; !ok { + ctx.ModuleErrorf("Only allowlisted modules may use uses_order_only_build_number_file: true") + } + cmd.OrderOnly(ctx.Config().BuildNumberFile(ctx)) + } // Create the rule to run the genrule command inside sbox. rule.Build(name, desc) diff --git a/genrule/genrule_test.go b/genrule/genrule_test.go index 444aedb90..9278f1574 100644 --- a/genrule/genrule_test.go +++ b/genrule/genrule_test.go @@ -19,6 +19,7 @@ import ( "os" "regexp" "strconv" + "strings" "testing" "android/soong/android" @@ -1192,6 +1193,68 @@ func TestGenruleWithGlobPaths(t *testing.T) { } } +func TestGenruleUsesOrderOnlyBuildNumberFile(t *testing.T) { + testCases := []struct { + name string + bp string + fs android.MockFS + expectedError string + expectedCommand string + }{ + { + name: "not allowed when not in allowlist", + fs: android.MockFS{ + "foo/Android.bp": []byte(` +genrule { + name: "gen", + uses_order_only_build_number_file: true, + cmd: "cp $(build_number_file) $(out)", + out: ["out.txt"], +} +`), + }, + expectedError: `Only allowlisted modules may use uses_order_only_build_number_file: true`, + }, + { + name: "normal", + fs: android.MockFS{ + "build/soong/tests/Android.bp": []byte(` +genrule { + name: "gen", + uses_order_only_build_number_file: true, + cmd: "cp $(build_number_file) $(out)", + out: ["out.txt"], +} +`), + }, + expectedCommand: `cp BUILD_NUMBER_FILE __SBOX_SANDBOX_DIR__/out/out.txt`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fixtures := android.GroupFixturePreparers( + prepareForGenRuleTest, + android.PrepareForTestWithVisibility, + android.FixtureMergeMockFs(tc.fs), + android.FixtureModifyConfigAndContext(func(config android.Config, ctx *android.TestContext) { + config.TestProductVariables.BuildNumberFile = proptools.StringPtr("build_number.txt") + }), + ) + if tc.expectedError != "" { + fixtures = fixtures.ExtendWithErrorHandler(android.FixtureExpectsOneErrorPattern(tc.expectedError)) + } + result := fixtures.RunTest(t) + + if tc.expectedError == "" { + tc.expectedCommand = strings.ReplaceAll(tc.expectedCommand, "BUILD_NUMBER_FILE", result.Config.SoongOutDir()+"/build_number.txt") + gen := result.Module("gen", "").(*Module) + android.AssertStringEquals(t, "raw commands", tc.expectedCommand, gen.rawCommands[0]) + } + }) + } +} + type testTool struct { android.ModuleBase outputFile android.Path