From 5011e61c7130433db51d48870deb88343d3422c1 Mon Sep 17 00:00:00 2001 From: Chris Parsons Date: Wed, 13 Sep 2023 23:33:20 +0000 Subject: [PATCH] Add unit test for parsing build files in bp2build This involves some minor changes to testing infrastructure. This is a rollforward of aosp/2628496 with a couple of minor changes: - In ParseBuildFiles, filter out all build files that are kept due to ShouldKeepExistingBuildFileForDir - Add some minor test infrastructure for StubbedBuildDefinitions, with a couple of proof of concept tests used to demonstrate its usage. This pattern will become immensely more common as we implement allowlist v2 (as we will need to update all tests which today simulate build definitions that have missing deps) Bug: 285631638 Fixes: 286545783 Test: bp2build.sh Test: m nothing Change-Id: I7c3a03b02098e39dd8e51d327482b440f294478f --- android/fixture.go | 9 +++ android/register.go | 54 ++++++++++++++++ bp2build/build_conversion_test.go | 63 +++++++++++++++++++ bp2build/cc_library_shared_conversion_test.go | 14 ++--- bp2build/testing.go | 46 +++++++++++--- cmd/soong_build/main.go | 44 ++----------- 6 files changed, 175 insertions(+), 55 deletions(-) diff --git a/android/fixture.go b/android/fixture.go index 6660afd65..5ad47e8c9 100644 --- a/android/fixture.go +++ b/android/fixture.go @@ -275,6 +275,15 @@ func FixtureModifyContext(mutator func(ctx *TestContext)) FixturePreparer { }) } +// Sync the mock filesystem with the current config, then modify the context, +// This allows context modification that requires filesystem access. +func FixtureModifyContextWithMockFs(mutator func(ctx *TestContext)) FixturePreparer { + return newSimpleFixturePreparer(func(f *fixture) { + f.config.mockFileSystem("", f.mockFS) + mutator(f.ctx) + }) +} + func FixtureRegisterWithContext(registeringFunc func(ctx RegistrationContext)) FixturePreparer { return FixtureModifyContext(func(ctx *TestContext) { registeringFunc(ctx) }) } diff --git a/android/register.go b/android/register.go index df97c75e4..f1c298691 100644 --- a/android/register.go +++ b/android/register.go @@ -15,9 +15,13 @@ package android import ( + "bufio" "fmt" + "path/filepath" "reflect" + "regexp" + "android/soong/shared" "github.com/google/blueprint" ) @@ -197,6 +201,56 @@ func (ctx *Context) RegisterForBazelConversion() { RegisterMutatorsForBazelConversion(ctx, bp2buildPreArchMutators) } +// RegisterExistingBazelTargets reads Bazel BUILD.bazel and BUILD files under +// the workspace, and returns a map containing names of Bazel targets defined in +// these BUILD files. +// For example, maps "//foo/bar" to ["baz", "qux"] if `//foo/bar:{baz,qux}` exist. +func (c *Context) RegisterExistingBazelTargets(topDir string, existingBazelFiles []string) error { + result := map[string][]string{} + + // Search for instances of `name = "$NAME"` (with arbitrary spacing). + targetNameRegex := regexp.MustCompile(`(?m)^\s*name\s*=\s*\"([^\"]+)\"`) + + parseBuildFile := func(path string) error { + fullPath := shared.JoinPath(topDir, path) + sourceDir := filepath.Dir(path) + + fileInfo, err := c.Config().fs.Stat(fullPath) + if err != nil { + return fmt.Errorf("Error accessing Bazel file '%s': %s", path, err) + } + if !fileInfo.IsDir() && + (fileInfo.Name() == "BUILD" || fileInfo.Name() == "BUILD.bazel") { + f, err := c.Config().fs.Open(fullPath) + if err != nil { + return fmt.Errorf("Error reading Bazel file '%s': %s", path, err) + } + defer f.Close() + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := scanner.Text() + matches := targetNameRegex.FindAllStringSubmatch(line, -1) + for _, match := range matches { + result[sourceDir] = append(result[sourceDir], match[1]) + } + } + } + return nil + } + + for _, path := range existingBazelFiles { + if !c.Config().Bp2buildPackageConfig.ShouldKeepExistingBuildFileForDir(filepath.Dir(path)) { + continue + } + err := parseBuildFile(path) + if err != nil { + return err + } + } + c.Config().SetBazelBuildFileTargets(result) + return nil +} + // Register the pipeline of singletons, module types, and mutators for // generating build.ninja and other files for Kati, from Android.bp files. func (ctx *Context) Register() { diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index 3887c5da9..3c2673425 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -1926,6 +1926,69 @@ func TestPrettyPrintSelectMapEqualValues(t *testing.T) { android.AssertStringEquals(t, "Print the common value if all keys in an axis have the same value", `[":libfoo.impl"]`, actual) } +func TestAlreadyPresentBuildTarget(t *testing.T) { + bp := ` + custom { + name: "foo", + } + custom { + name: "bar", + } + ` + alreadyPresentBuildFile := + MakeBazelTarget( + "custom", + "foo", + AttrNameToString{}, + ) + expectedBazelTargets := []string{ + MakeBazelTarget( + "custom", + "bar", + AttrNameToString{}, + ), + } + registerCustomModule := func(ctx android.RegistrationContext) { + ctx.RegisterModuleType("custom", customModuleFactoryHostAndDevice) + } + RunBp2BuildTestCase(t, registerCustomModule, Bp2buildTestCase{ + AlreadyExistingBuildContents: alreadyPresentBuildFile, + Blueprint: bp, + ExpectedBazelTargets: expectedBazelTargets, + Description: "Not duplicating work for an already-present BUILD target.", + }) +} + +// Verifies that if a module is defined in pkg1/Android.bp, that a target present +// in pkg2/BUILD.bazel does not result in the module being labeled "already defined +// in a BUILD file". +func TestBuildTargetPresentOtherDirectory(t *testing.T) { + bp := ` + custom { + name: "foo", + } + ` + expectedBazelTargets := []string{ + MakeBazelTarget( + "custom", + "foo", + AttrNameToString{}, + ), + } + registerCustomModule := func(ctx android.RegistrationContext) { + ctx.RegisterModuleType("custom", customModuleFactoryHostAndDevice) + } + RunBp2BuildTestCase(t, registerCustomModule, Bp2buildTestCase{ + KeepBuildFileForDirs: []string{"other_pkg"}, + Filesystem: map[string]string{ + "other_pkg/BUILD.bazel": MakeBazelTarget("custom", "foo", AttrNameToString{}), + }, + Blueprint: bp, + ExpectedBazelTargets: expectedBazelTargets, + Description: "Not treating a BUILD target as the bazel definition for a module in another package", + }) +} + // If CommonAttributes.Dir is set, the bazel target should be created in that dir func TestCreateBazelTargetInDifferentDir(t *testing.T) { t.Parallel() diff --git a/bp2build/cc_library_shared_conversion_test.go b/bp2build/cc_library_shared_conversion_test.go index 44b97227e..9b533f397 100644 --- a/bp2build/cc_library_shared_conversion_test.go +++ b/bp2build/cc_library_shared_conversion_test.go @@ -218,12 +218,12 @@ cc_library_shared { func TestCcLibrarySharedOsSpecificSharedLib(t *testing.T) { runCcLibrarySharedTestCase(t, Bp2buildTestCase{ - Description: "cc_library_shared os-specific shared_libs", - Filesystem: map[string]string{}, + StubbedBuildDefinitions: []string{"shared_dep"}, + Description: "cc_library_shared os-specific shared_libs", + Filesystem: map[string]string{}, Blueprint: soongCcLibrarySharedPreamble + ` cc_library_shared { name: "shared_dep", - bazel_module: { bp2build_available: false }, } cc_library_shared { name: "foo_shared", @@ -243,20 +243,18 @@ cc_library_shared { func TestCcLibrarySharedBaseArchOsSpecificSharedLib(t *testing.T) { runCcLibrarySharedTestCase(t, Bp2buildTestCase{ - Description: "cc_library_shared base, arch, and os-specific shared_libs", - Filesystem: map[string]string{}, + StubbedBuildDefinitions: []string{"shared_dep", "shared_dep2", "shared_dep3"}, + Description: "cc_library_shared base, arch, and os-specific shared_libs", + Filesystem: map[string]string{}, Blueprint: soongCcLibrarySharedPreamble + ` cc_library_shared { name: "shared_dep", - bazel_module: { bp2build_available: false }, } cc_library_shared { name: "shared_dep2", - bazel_module: { bp2build_available: false }, } cc_library_shared { name: "shared_dep3", - bazel_module: { bp2build_available: false }, } cc_library_shared { name: "foo_shared", diff --git a/bp2build/testing.go b/bp2build/testing.go index 997df64a4..7333c0b50 100644 --- a/bp2build/testing.go +++ b/bp2build/testing.go @@ -21,6 +21,7 @@ specific-but-shared functionality among tests in package import ( "fmt" + "path/filepath" "sort" "strings" "testing" @@ -82,7 +83,16 @@ type Bp2buildTestCase struct { // ExpectedBazelTargets compares the BazelTargets generated in `Dir` (if not empty). // Otherwise, it checks the BazelTargets generated by `Blueprint` in the root directory. ExpectedBazelTargets []string - Filesystem map[string]string + // AlreadyExistingBuildContents, if non-empty, simulates an already-present source BUILD file + // in the directory under test. The BUILD file has the given contents. This BUILD file + // will also be treated as "BUILD file to keep" by the simulated bp2build environment. + AlreadyExistingBuildContents string + // StubbedBuildDefinitions, if non-empty, adds stub definitions to an already-present source + // BUILD file in the directory under test, for each target name given. These stub definitions + // are added to the BUILD file given in AlreadyExistingBuildContents, if it is set. + StubbedBuildDefinitions []string + + Filesystem map[string]string // Dir sets the directory which will be compared against the targets in ExpectedBazelTargets. // This should used in conjunction with the Filesystem property to check for targets // generated from a directory that is not the root. @@ -110,11 +120,31 @@ func RunBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.Regi func runBp2BuildTestCaseWithSetup(t *testing.T, extraPreparer android.FixturePreparer, tc Bp2buildTestCase) { t.Helper() - dir := "." + checkDir := "." + if tc.Dir != "" { + checkDir = tc.Dir + } + keepExistingBuildDirs := tc.KeepBuildFileForDirs + buildFilesToParse := []string{} filesystem := make(map[string][]byte) for f, content := range tc.Filesystem { filesystem[f] = []byte(content) } + alreadyExistingBuildContents := tc.AlreadyExistingBuildContents + if len(tc.StubbedBuildDefinitions) > 0 { + for _, targetName := range tc.StubbedBuildDefinitions { + alreadyExistingBuildContents += MakeBazelTarget( + "bp2build_test_stub", + targetName, + AttrNameToString{}) + } + } + if len(alreadyExistingBuildContents) > 0 { + buildFilePath := filepath.Join(checkDir, "BUILD") + filesystem[buildFilePath] = []byte(alreadyExistingBuildContents) + keepExistingBuildDirs = append(keepExistingBuildDirs, checkDir) + buildFilesToParse = append(buildFilesToParse, buildFilePath) + } preparers := []android.FixturePreparer{ extraPreparer, @@ -123,7 +153,7 @@ func runBp2BuildTestCaseWithSetup(t *testing.T, extraPreparer android.FixturePre android.FixtureRegisterWithContext(func(ctx android.RegistrationContext) { ctx.RegisterModuleType(tc.ModuleTypeUnderTest, tc.ModuleTypeUnderTestFactory) }), - android.FixtureModifyContext(func(ctx *android.TestContext) { + android.FixtureModifyContextWithMockFs(func(ctx *android.TestContext) { // A default configuration for tests to not have to specify bp2build_available on top level // targets. bp2buildConfig := android.NewBp2BuildAllowlist().SetDefaultConfig( @@ -131,7 +161,7 @@ func runBp2BuildTestCaseWithSetup(t *testing.T, extraPreparer android.FixturePre android.Bp2BuildTopLevel: allowlists.Bp2BuildDefaultTrueRecursively, }, ) - for _, f := range tc.KeepBuildFileForDirs { + for _, f := range keepExistingBuildDirs { bp2buildConfig.SetKeepExistingBuildFile(map[string]bool{ f: /*recursive=*/ false, }) @@ -141,6 +171,10 @@ func runBp2BuildTestCaseWithSetup(t *testing.T, extraPreparer android.FixturePre // from cloning modules to their original state after mutators run. This // would lose some data intentionally set by these mutators. ctx.SkipCloneModulesAfterMutators = true + err := ctx.RegisterExistingBazelTargets(".", buildFilesToParse) + if err != nil { + t.Errorf("error parsing build files in test setup: %s", err) + } }), android.FixtureModifyEnv(func(env map[string]string) { if tc.UnconvertedDepsMode == errorModulesUnconvertedDeps { @@ -159,10 +193,6 @@ func runBp2BuildTestCaseWithSetup(t *testing.T, extraPreparer android.FixturePre return } - checkDir := dir - if tc.Dir != "" { - checkDir = tc.Dir - } expectedTargets := map[string][]string{ checkDir: tc.ExpectedBazelTargets, } diff --git a/cmd/soong_build/main.go b/cmd/soong_build/main.go index 626f076a3..d20847bb2 100644 --- a/cmd/soong_build/main.go +++ b/cmd/soong_build/main.go @@ -21,7 +21,6 @@ import ( "fmt" "os" "path/filepath" - "regexp" "strings" "time" @@ -617,43 +616,6 @@ func excludedFromSymlinkForest(ctx *android.Context, verbose bool) []string { return excluded } -// buildTargetsByPackage parses Bazel BUILD.bazel and BUILD files under -// the workspace, and returns a map containing names of Bazel targets defined in -// these BUILD files. -// For example, maps "//foo/bar" to ["baz", "qux"] if `//foo/bar:{baz,qux}` exist. -func buildTargetsByPackage(ctx *android.Context) map[string][]string { - existingBazelFiles, err := getExistingBazelRelatedFiles(topDir) - maybeQuit(err, "Error determining existing Bazel-related files") - - result := map[string][]string{} - - // Search for instances of `name = "$NAME"` (with arbitrary spacing). - targetNameRegex := regexp.MustCompile(`(?m)^\s*name\s*=\s*\"([^\"]+)\"`) - - for _, path := range existingBazelFiles { - if !ctx.Config().Bp2buildPackageConfig.ShouldKeepExistingBuildFileForDir(filepath.Dir(path)) { - continue - } - fullPath := shared.JoinPath(topDir, path) - sourceDir := filepath.Dir(path) - fileInfo, err := os.Stat(fullPath) - maybeQuit(err, "Error accessing Bazel file '%s'", fullPath) - - if !fileInfo.IsDir() && - (fileInfo.Name() == "BUILD" || fileInfo.Name() == "BUILD.bazel") { - // Process this BUILD file. - buildFileContent, err := os.ReadFile(fullPath) - maybeQuit(err, "Error reading Bazel file '%s'", fullPath) - - matches := targetNameRegex.FindAllStringSubmatch(string(buildFileContent), -1) - for _, match := range matches { - result[sourceDir] = append(result[sourceDir], match[1]) - } - } - } - return result -} - // Run Soong in the bp2build mode. This creates a standalone context that registers // an alternate pipeline of mutators and singletons specifically for generating // Bazel BUILD files instead of Ninja files. @@ -662,7 +624,11 @@ func runBp2Build(ctx *android.Context, extraNinjaDeps []string, metricsDir strin ctx.EventHandler.Do("bp2build", func() { ctx.EventHandler.Do("read_build", func() { - ctx.Config().SetBazelBuildFileTargets(buildTargetsByPackage(ctx)) + existingBazelFiles, err := getExistingBazelRelatedFiles(topDir) + maybeQuit(err, "Error determining existing Bazel-related files") + + err = ctx.RegisterExistingBazelTargets(topDir, existingBazelFiles) + maybeQuit(err, "Error parsing existing Bazel-related files") }) // Propagate "allow misssing dependencies" bit. This is normally set in