From 4910976314d1467c6c5a2eb49d9876e54bde94c7 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Tue, 25 May 2021 05:16:48 +0000 Subject: [PATCH] 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.