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

@@ -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) })
 }

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() {

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()

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",

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,
 	}

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