From 4fabaf52b0c457b154017d678d1fc56f476b4087 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Tue, 25 May 2021 02:40:29 +0000 Subject: [PATCH 1/2] bp2build/b: exit early in GENERATE_BAZEL_FILES=1. This CL fixes a typo in writeFakeNinjaFile to correctly write a fake out/soong/build.ninja and its depfile. It also modifies bootstrap phase to *not* run the main soong build phase (which takes more than a minute) if GENERATE_BAZEL_FILES=1. This change has the side effect that `GENERATE_BAZEL_FILES=1 m nothing` no longer generates the real build.ninja, which is fine because one shouldn't be using GENERATE_BAZEL_FILES=1 for that anyway (or, use USE_BAZEL_ANALYSIS=1). This change has no effect on mixed builds. Time on a change to Soong or any Android.bp files: Before: bp2build_workspace_marker (~20 seconds) + build.ninja (1 min) After: bp2build_workspace_marker (~20 seconds) Time on the second of two consecutive `b build //bionic/...`: 2.070s Test: TH Test: Soong integration tests Change-Id: I43720641815994caba97b8d165d7c3fc254cbd06 --- cmd/soong_build/main.go | 28 ++++++++++++++++------------ tests/bootstrap_test.sh | 16 ++++++++++++++++ ui/build/soong.go | 6 +++--- 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/cmd/soong_build/main.go b/cmd/soong_build/main.go index 70c88563f..7abb67f29 100644 --- a/cmd/soong_build/main.go +++ b/cmd/soong_build/main.go @@ -175,6 +175,9 @@ func writeJsonModuleGraph(configuration android.Config, ctx *android.Context, pa writeFakeNinjaFile(extraNinjaDeps, configuration.BuildDir()) } +// doChosenActivity runs Soong for a specific activity, like bp2build, queryview +// or the actual Soong build for the build.ninja file. Returns the top level +// output file of the specific activity. func doChosenActivity(configuration android.Config, extraNinjaDeps []string) string { bazelConversionRequested := bp2buildMarker != "" mixedModeBuild := configuration.BazelContext.BazelEnabled() @@ -187,11 +190,7 @@ func doChosenActivity(configuration android.Config, extraNinjaDeps []string) str // Run the alternate pipeline of bp2build mutators and singleton to convert // Blueprint to BUILD files before everything else. runBp2Build(configuration, extraNinjaDeps) - if bp2buildMarker != "" { - return bp2buildMarker - } else { - return bootstrap.CmdlineArgs.OutFile - } + return bp2buildMarker } ctx := newContext(configuration, prepareBuildActions) @@ -327,13 +326,13 @@ func writeFakeNinjaFile(extraNinjaDeps []string, buildDir string) { ninjaFileName := "build.ninja" ninjaFile := shared.JoinPath(topDir, buildDir, ninjaFileName) - ninjaFileD := shared.JoinPath(topDir, buildDir, ninjaFileName) + ninjaFileD := shared.JoinPath(topDir, buildDir, ninjaFileName+".d") // A workaround to create the 'nothing' ninja target so `m nothing` works, // since bp2build runs without Kati, and the 'nothing' target is declared in // a Makefile. ioutil.WriteFile(ninjaFile, []byte("build nothing: phony\n phony_output = true\n"), 0666) ioutil.WriteFile(ninjaFileD, - []byte(fmt.Sprintf("%s: \\\n %s\n", ninjaFileName, extraNinjaDepsString)), + []byte(fmt.Sprintf("%s: \\\n %s\n", ninjaFile, extraNinjaDepsString)), 0666) } @@ -520,9 +519,14 @@ func runBp2Build(configuration android.Config, extraNinjaDeps []string) { os.Exit(1) } - if bp2buildMarker != "" { - touch(shared.JoinPath(topDir, bp2buildMarker)) - } else { - writeFakeNinjaFile(extraNinjaDeps, codegenContext.Config().BuildDir()) - } + // Create an empty bp2build marker file. + touch(shared.JoinPath(topDir, bp2buildMarker)) + + // bp2build *always* writes a fake Ninja file containing just the nothing + // phony target if it ever re-runs. This allows bp2build to exit early with + // GENERATE_BAZEL_FILES=1 m nothing. + // + // If bp2build is invoked as part of an integrated mixed build, the fake + // build.ninja file will be rewritten later into the real file anyway. + writeFakeNinjaFile(extraNinjaDeps, codegenContext.Config().BuildDir()) } diff --git a/tests/bootstrap_test.sh b/tests/bootstrap_test.sh index 42363e9c0..8c8dc8208 100755 --- a/tests/bootstrap_test.sh +++ b/tests/bootstrap_test.sh @@ -493,6 +493,21 @@ function test_bp2build_smoke { [[ -e out/soong/workspace ]] || fail "Bazel workspace not created" } +function test_bp2build_generates_fake_ninja_file { + setup + create_mock_bazel + + run_bp2build + + if [[ ! -f "./out/soong/build.ninja" ]]; then + fail "./out/soong/build.ninja was not generated" + fi + + if ! grep "build nothing: phony" "./out/soong/build.ninja"; then + fail "missing phony nothing target in out/soong/build.ninja" + fi +} + function test_bp2build_add_android_bp { setup @@ -678,6 +693,7 @@ test_glob_during_bootstrapping test_soong_build_rerun_iff_environment_changes test_dump_json_module_graph test_bp2build_smoke +test_bp2build_generates_fake_ninja_file test_bp2build_null_build test_bp2build_add_android_bp test_bp2build_add_to_glob diff --git a/ui/build/soong.go b/ui/build/soong.go index a41dbe1b5..cd645eb41 100644 --- a/ui/build/soong.go +++ b/ui/build/soong.go @@ -155,9 +155,9 @@ func bootstrapBlueprint(ctx Context, config Config, integratedBp2Build bool) { Outputs: []string{bp2BuildMarkerFile}, Args: bp2buildArgs, } - args.PrimaryBuilderInvocations = []bootstrap.PrimaryBuilderInvocation{ - bp2buildInvocation, - mainSoongBuildInvocation, + args.PrimaryBuilderInvocations = []bootstrap.PrimaryBuilderInvocation{bp2buildInvocation} + if config.bazelBuildMode() == mixedBuild { + args.PrimaryBuilderInvocations = append(args.PrimaryBuilderInvocations, mainSoongBuildInvocation) } } else { args.PrimaryBuilderInvocations = []bootstrap.PrimaryBuilderInvocation{mainSoongBuildInvocation} From 4910976314d1467c6c5a2eb49d9876e54bde94c7 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Tue, 25 May 2021 05:16:48 +0000 Subject: [PATCH 2/2] Make BUILD file merging slightly smarter. This change enables checked-in BUILD files like prebuilts/clang/host/linux-x86/BUILD.bazel to be merged cleanly with the bp2build generated one into the synthetic workspace. The checked-in BUILD file contains a package() declaration that bp2build also generates. To avoid double declaration, the BUILD file writer now checks if the BazelTargets contain handcrafted targets. If so, it delegates the package declaration to the handcrafted BUILD file instead. This change also sorts the bp2build targets before the handcrafted ones, and adds a section header to demarcate the two sets of targets. Test: TH Change-Id: I3ecdeaab3226b895b623daf0791d24a657f7a7c6 --- android/bazel.go | 2 +- bp2build/build_conversion.go | 41 ++++++++++++++- bp2build/build_conversion_test.go | 86 +++++++++++++++++-------------- bp2build/conversion.go | 35 +++++++------ 4 files changed, 108 insertions(+), 56 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index f56c24e04..ef770bf9c 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -138,7 +138,6 @@ var ( // e.g. ERROR: Analysis of target '@soong_injection//mixed_builds:buildroot' failed "external/bazelbuild-rules_android":/* recursive = */ true, - "prebuilts/clang/host/linux-x86":/* recursive = */ false, "prebuilts/sdk":/* recursive = */ false, "prebuilts/sdk/tools":/* recursive = */ false, } @@ -155,6 +154,7 @@ var ( "external/fmtlib": Bp2BuildDefaultTrueRecursively, "external/arm-optimized-routines": Bp2BuildDefaultTrueRecursively, "external/scudo": Bp2BuildDefaultTrueRecursively, + "prebuilts/clang/host/linux-x86": Bp2BuildDefaultTrueRecursively, } // Per-module denylist to always opt modules out of both bp2build and mixed builds. diff --git a/bp2build/build_conversion.go b/bp2build/build_conversion.go index bddc524cd..7a73e18b8 100644 --- a/bp2build/build_conversion.go +++ b/bp2build/build_conversion.go @@ -19,6 +19,7 @@ import ( "android/soong/bazel" "fmt" "reflect" + "sort" "strings" "github.com/google/blueprint" @@ -34,6 +35,7 @@ type BazelTarget struct { content string ruleClass string bzlLoadLocation string + handcrafted bool } // IsLoadedFromStarlark determines if the BazelTarget's rule class is loaded from a .bzl file, @@ -45,12 +47,47 @@ func (t BazelTarget) IsLoadedFromStarlark() bool { // BazelTargets is a typedef for a slice of BazelTarget objects. type BazelTargets []BazelTarget +// HasHandcraftedTargetsreturns true if a set of bazel targets contain +// handcrafted ones. +func (targets BazelTargets) hasHandcraftedTargets() bool { + for _, target := range targets { + if target.handcrafted { + return true + } + } + return false +} + +// sort a list of BazelTargets in-place, by name, and by generated/handcrafted types. +func (targets BazelTargets) sort() { + sort.Slice(targets, func(i, j int) bool { + if targets[i].handcrafted != targets[j].handcrafted { + // Handcrafted targets will be generated after the bp2build generated targets. + return targets[j].handcrafted + } + // This will cover all bp2build generated targets. + return targets[i].name < targets[j].name + }) +} + // String returns the string representation of BazelTargets, without load // statements (use LoadStatements for that), since the targets are usually not // adjacent to the load statements at the top of the BUILD file. func (targets BazelTargets) String() string { var res string for i, target := range targets { + // There is only at most 1 handcrafted "target", because its contents + // represent the entire BUILD file content from the tree. See + // build_conversion.go#getHandcraftedBuildContent for more information. + // + // Add a header to make it easy to debug where the handcrafted targets + // are in a generated BUILD file. + if target.handcrafted { + res += "# -----------------------------\n" + res += "# Section: Handcrafted targets. \n" + res += "# -----------------------------\n\n" + } + res += target.content if i != len(targets)-1 { res += "\n\n" @@ -267,7 +304,8 @@ func getHandcraftedBuildContent(ctx *CodegenContext, b android.Bazelable, pathTo } // TODO(b/181575318): once this is more targeted, we need to include name, rule class, etc return BazelTarget{ - content: c, + content: c, + handcrafted: true, }, nil } @@ -294,6 +332,7 @@ func generateBazelTarget(ctx bpToBuildContext, m blueprint.Module, btm android.B targetName, attributes, ), + handcrafted: false, } } diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index 71660a8e1..b1c342c69 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -1452,53 +1452,61 @@ filegroup { dir := "." for _, testCase := range testCases { - fs := make(map[string][]byte) - toParse := []string{ - "Android.bp", - } - for f, content := range testCase.fs { - if strings.HasSuffix(f, "Android.bp") { - toParse = append(toParse, f) + t.Run(testCase.description, func(t *testing.T) { + fs := make(map[string][]byte) + toParse := []string{ + "Android.bp", } - fs[f] = []byte(content) - } - config := android.TestConfig(buildDir, nil, testCase.bp, fs) - ctx := android.NewTestContext(config) - ctx.RegisterModuleType(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestFactory) - for _, m := range testCase.depsMutators { - ctx.DepsBp2BuildMutators(m) - } - ctx.RegisterBp2BuildMutator(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestBp2BuildMutator) - ctx.RegisterForBazelConversion() + for f, content := range testCase.fs { + if strings.HasSuffix(f, "Android.bp") { + toParse = append(toParse, f) + } + fs[f] = []byte(content) + } + config := android.TestConfig(buildDir, nil, testCase.bp, fs) + ctx := android.NewTestContext(config) + ctx.RegisterModuleType(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestFactory) + for _, m := range testCase.depsMutators { + ctx.DepsBp2BuildMutators(m) + } + ctx.RegisterBp2BuildMutator(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestBp2BuildMutator) + ctx.RegisterForBazelConversion() - _, errs := ctx.ParseFileList(dir, toParse) - if errored(t, testCase.description, errs) { - continue - } - _, errs = ctx.ResolveDependencies(config) - if errored(t, testCase.description, errs) { - continue - } + _, errs := ctx.ParseFileList(dir, toParse) + if errored(t, testCase.description, errs) { + return + } + _, errs = ctx.ResolveDependencies(config) + if errored(t, testCase.description, errs) { + return + } - checkDir := dir - if testCase.dir != "" { - checkDir = testCase.dir - } - bazelTargets := generateBazelTargetsForDir(NewCodegenContext(config, *ctx.Context, Bp2Build), checkDir) - if actualCount, expectedCount := len(bazelTargets), len(testCase.expectedBazelTargets); actualCount != expectedCount { - t.Errorf("%s: Expected %d bazel target, got %d\n%s", testCase.description, expectedCount, actualCount, bazelTargets) - } else { + checkDir := dir + if testCase.dir != "" { + checkDir = testCase.dir + } + bazelTargets := generateBazelTargetsForDir(NewCodegenContext(config, *ctx.Context, Bp2Build), checkDir) + bazelTargets.sort() + actualCount := len(bazelTargets) + expectedCount := len(testCase.expectedBazelTargets) + if actualCount != expectedCount { + t.Errorf("Expected %d bazel target, got %d\n%s", expectedCount, actualCount, bazelTargets) + } + if !strings.Contains(bazelTargets.String(), "Section: Handcrafted targets. ") { + t.Errorf("Expected string representation of bazelTargets to contain handcrafted section header.") + } for i, target := range bazelTargets { - if w, g := testCase.expectedBazelTargets[i], target.content; w != g { + actualContent := target.content + expectedContent := testCase.expectedBazelTargets[i] + if expectedContent != actualContent { t.Errorf( - "%s: Expected generated Bazel target to be '%s', got '%s'", - testCase.description, - w, - g, + "Expected generated Bazel target to be '%s', got '%s'", + expectedContent, + actualContent, ) } } - } + }) } } diff --git a/bp2build/conversion.go b/bp2build/conversion.go index 101ad3d04..bced4c19f 100644 --- a/bp2build/conversion.go +++ b/bp2build/conversion.go @@ -5,7 +5,6 @@ import ( "android/soong/cc/config" "fmt" "reflect" - "sort" "strings" "github.com/google/blueprint/proptools" @@ -64,22 +63,28 @@ func createBuildFiles(buildToTargets map[string]BazelTargets, mode CodegenMode) continue } targets := buildToTargets[dir] - sort.Slice(targets, func(i, j int) bool { - // this will cover all bp2build generated targets - if targets[i].name < targets[j].name { - return true - } - // give a strict ordering to content from hand-crafted targets - return targets[i].content < targets[j].content - }) - content := soongModuleLoad + targets.sort() + + var content string if mode == Bp2Build { - content = `# This file was automatically generated by bp2build for the Bazel migration project. -# Feel free to edit or test it, but do *not* check it into your version control system.` - content += "\n\n" - content += "package(default_visibility = [\"//visibility:public\"])" - content += "\n\n" + content = `# READ THIS FIRST: +# This file was automatically generated by bp2build for the Bazel migration project. +# Feel free to edit or test it, but do *not* check it into your version control system. +` + if targets.hasHandcraftedTargets() { + // For BUILD files with both handcrafted and generated targets, + // don't hardcode actual content, like package() declarations. + // Leave that responsibility to the checked-in BUILD file + // instead. + content += `# This file contains generated targets and handcrafted targets that are manually managed in the source tree.` + } else { + // For fully-generated BUILD files, hardcode the default visibility. + content += "package(default_visibility = [\"//visibility:public\"])" + } + content += "\n" content += targets.LoadStatements() + } else if mode == QueryView { + content = soongModuleLoad } if content != "" { // If there are load statements, add a couple of newlines.