From 0eeaeb8ef7665c83e82b1fdfa213fcfd3dadd8e8 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Wed, 21 Sep 2022 10:27:42 +0000 Subject: [PATCH] Fix package boundary in glob expansion with checked in BUILD files. For directories without an Android.bp file, if they contain a merged checked in BUILD file, it becomes a package boundary in the symlink forest. However, the current glob expansion is only aware of Android.bp files, but not merged BUILD files, so it generates glob expansions incorrectly for files that cross the package boundary. This CL fixes that by making the package boundary function aware of the keepExistingBuildFile allowlist. See the new test cases for example. Also stop using the same global bp2buildConfig for every test case to allow each test to define their own bp2build allowlists. Bug: 185358476 Test: new unit tests Change-Id: Ifcd1283480a09a642ef4343e5bbc599583524577 --- android/bazel_paths.go | 38 +++-- bp2build/build_conversion_test.go | 271 ++++++++++++++++++++++++------ bp2build/testing.go | 24 ++- 3 files changed, 259 insertions(+), 74 deletions(-) diff --git a/android/bazel_paths.go b/android/bazel_paths.go index bbdae96e7..0d38bdad2 100644 --- a/android/bazel_paths.go +++ b/android/bazel_paths.go @@ -202,20 +202,26 @@ func BazelLabelForModuleSrcExcludes(ctx BazelConversionPathContext, paths, exclu return labels } -// Returns true if a prefix + components[:i] + /Android.bp exists -// TODO(b/185358476) Could check for BUILD file instead of checking for Android.bp file, or ensure BUILD is always generated? -func directoryHasBlueprint(fs pathtools.FileSystem, prefix string, components []string, componentIndex int) bool { - blueprintPath := prefix - if blueprintPath != "" { - blueprintPath = blueprintPath + "/" - } - blueprintPath = blueprintPath + strings.Join(components[:componentIndex+1], "/") - blueprintPath = blueprintPath + "/Android.bp" - if exists, _, _ := fs.Exists(blueprintPath); exists { +// Returns true if a prefix + components[:i] is a package boundary. +// +// A package boundary is determined by a BUILD file in the directory. This can happen in 2 cases: +// +// 1. An Android.bp exists, which bp2build will always convert to a sibling BUILD file. +// 2. An Android.bp doesn't exist, but a checked-in BUILD/BUILD.bazel file exists, and that file +// is allowlisted by the bp2build configuration to be merged into the symlink forest workspace. +func isPackageBoundary(config Config, prefix string, components []string, componentIndex int) bool { + prefix = filepath.Join(prefix, filepath.Join(components[:componentIndex+1]...)) + if exists, _, _ := config.fs.Exists(filepath.Join(prefix, "Android.bp")); exists { return true - } else { - return false + } else if config.Bp2buildPackageConfig.ShouldKeepExistingBuildFileForDir(prefix) { + if exists, _, _ := config.fs.Exists(filepath.Join(prefix, "BUILD")); exists { + return true + } else if exists, _, _ := config.fs.Exists(filepath.Join(prefix, "BUILD.bazel")); exists { + return true + } } + + return false } // Transform a path (if necessary) to acknowledge package boundaries @@ -245,14 +251,14 @@ func transformSubpackagePath(ctx BazelConversionPathContext, path bazel.Label) b newLabel := "" pathComponents := strings.Split(path.Label, "/") - foundBlueprint := false + foundPackageBoundary := false // Check the deepest subdirectory first and work upwards for i := len(pathComponents) - 1; i >= 0; i-- { pathComponent := pathComponents[i] var sep string - if !foundBlueprint && directoryHasBlueprint(ctx.Config().fs, ctx.ModuleDir(), pathComponents, i) { + if !foundPackageBoundary && isPackageBoundary(ctx.Config(), ctx.ModuleDir(), pathComponents, i) { sep = ":" - foundBlueprint = true + foundPackageBoundary = true } else { sep = "/" } @@ -262,7 +268,7 @@ func transformSubpackagePath(ctx BazelConversionPathContext, path bazel.Label) b newLabel = pathComponent + sep + newLabel } } - if foundBlueprint { + if foundPackageBoundary { // Ensure paths end up looking like //bionic/... instead of //./bionic/... moduleDir := ctx.ModuleDir() if strings.HasPrefix(moduleDir, ".") { diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index dd28c3c94..d677c0f5b 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -986,57 +986,6 @@ func TestModuleTypeBp2Build(t *testing.T) { }), }, }, - { - Description: "filegroup with glob", - ModuleTypeUnderTest: "filegroup", - ModuleTypeUnderTestFactory: android.FileGroupFactory, - Blueprint: `filegroup { - name: "fg_foo", - srcs: ["**/*.txt"], - bazel_module: { bp2build_available: true }, -}`, - ExpectedBazelTargets: []string{ - MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ - "srcs": `[ - "other/a.txt", - "other/b.txt", - "other/subdir/a.txt", - ]`, - }), - }, - Filesystem: map[string]string{ - "other/a.txt": "", - "other/b.txt": "", - "other/subdir/a.txt": "", - "other/file": "", - }, - }, - { - Description: "filegroup with glob in subdir", - ModuleTypeUnderTest: "filegroup", - ModuleTypeUnderTestFactory: android.FileGroupFactory, - Dir: "other", - Filesystem: map[string]string{ - "other/Android.bp": `filegroup { - name: "fg_foo", - srcs: ["**/*.txt"], - bazel_module: { bp2build_available: true }, -}`, - "other/a.txt": "", - "other/b.txt": "", - "other/subdir/a.txt": "", - "other/file": "", - }, - ExpectedBazelTargets: []string{ - MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ - "srcs": `[ - "a.txt", - "b.txt", - "subdir/a.txt", - ]`, - }), - }, - }, { Description: "depends_on_other_dir_module", ModuleTypeUnderTest: "filegroup", @@ -1429,6 +1378,226 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) { } } +func TestGlob(t *testing.T) { + testCases := []Bp2buildTestCase{ + { + Description: "filegroup with glob", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + Blueprint: `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "other/a.txt", + "other/b.txt", + "other/subdir/a.txt", + ]`, + }), + }, + Filesystem: map[string]string{ + "other/a.txt": "", + "other/b.txt": "", + "other/subdir/a.txt": "", + "other/file": "", + }, + }, + { + Description: "filegroup with glob in subdir", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + Dir: "other", + Filesystem: map[string]string{ + "other/Android.bp": `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + "other/a.txt": "", + "other/b.txt": "", + "other/subdir/a.txt": "", + "other/file": "", + }, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "a.txt", + "b.txt", + "subdir/a.txt", + ]`, + }), + }, + }, + { + Description: "filegroup with glob with no kept BUILD files", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + KeepBuildFileForDirs: []string{ + // empty + }, + Blueprint: `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + Filesystem: map[string]string{ + "a.txt": "", + "b.txt": "", + "foo/BUILD": "", + "foo/a.txt": "", + "foo/bar/BUILD": "", + "foo/bar/b.txt": "", + }, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "a.txt", + "b.txt", + "foo/a.txt", + "foo/bar/b.txt", + ]`, + }), + }, + }, + { + Description: "filegroup with glob with kept BUILD file", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + KeepBuildFileForDirs: []string{ + "foo", + }, + Blueprint: `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + Filesystem: map[string]string{ + "a.txt": "", + "b.txt": "", + "foo/BUILD": "", + "foo/a.txt": "", + "foo/bar/BUILD": "", + "foo/bar/b.txt": "", + }, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "a.txt", + "b.txt", + "//foo:a.txt", + "//foo:bar/b.txt", + ]`, + }), + }, + }, + { + Description: "filegroup with glob with kept BUILD.bazel file", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + KeepBuildFileForDirs: []string{ + "foo", + }, + Blueprint: `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + Filesystem: map[string]string{ + "a.txt": "", + "b.txt": "", + "foo/BUILD.bazel": "", + "foo/a.txt": "", + "foo/bar/BUILD.bazel": "", + "foo/bar/b.txt": "", + }, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "a.txt", + "b.txt", + "//foo:a.txt", + "//foo:bar/b.txt", + ]`, + }), + }, + }, + { + Description: "filegroup with glob with Android.bp file as boundary", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + Blueprint: `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + Filesystem: map[string]string{ + "a.txt": "", + "b.txt": "", + "foo/Android.bp": "", + "foo/a.txt": "", + "foo/bar/Android.bp": "", + "foo/bar/b.txt": "", + }, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "a.txt", + "b.txt", + "//foo:a.txt", + "//foo/bar:b.txt", + ]`, + }), + }, + }, + { + Description: "filegroup with glob in subdir with kept BUILD and BUILD.bazel file", + ModuleTypeUnderTest: "filegroup", + ModuleTypeUnderTestFactory: android.FileGroupFactory, + Dir: "other", + KeepBuildFileForDirs: []string{ + "other/foo", + "other/foo/bar", + // deliberately not other/foo/baz/BUILD. + }, + Filesystem: map[string]string{ + "other/Android.bp": `filegroup { + name: "fg_foo", + srcs: ["**/*.txt"], + bazel_module: { bp2build_available: true }, +}`, + "other/a.txt": "", + "other/b.txt": "", + "other/foo/BUILD": "", + "other/foo/a.txt": "", + "other/foo/bar/BUILD.bazel": "", + "other/foo/bar/b.txt": "", + "other/foo/baz/BUILD": "", + "other/foo/baz/c.txt": "", + }, + ExpectedBazelTargets: []string{ + MakeBazelTargetNoRestrictions("filegroup", "fg_foo", map[string]string{ + "srcs": `[ + "a.txt", + "b.txt", + "//other/foo:a.txt", + "//other/foo/bar:b.txt", + "//other/foo:baz/c.txt", + ]`, + }), + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Description, func(t *testing.T) { + RunBp2BuildTestCaseSimple(t, testCase) + }) + } +} + func TestGlobExcludeSrcs(t *testing.T) { testCases := []Bp2buildTestCase{ { diff --git a/bp2build/testing.go b/bp2build/testing.go index 8ce8bb28e..c2c1b197d 100644 --- a/bp2build/testing.go +++ b/bp2build/testing.go @@ -30,13 +30,6 @@ import ( ) var ( - // A default configuration for tests to not have to specify bp2build_available on top level targets. - bp2buildConfig = android.NewBp2BuildAllowlist().SetDefaultConfig( - allowlists.Bp2BuildConfig{ - android.Bp2BuildTopLevel: allowlists.Bp2BuildDefaultTrueRecursively, - }, - ) - buildDir string ) @@ -87,6 +80,11 @@ type Bp2buildTestCase struct { // An error with a string contained within the string of the expected error ExpectedErr error UnconvertedDepsMode unconvertedDepsMode + + // For every directory listed here, the BUILD file for that directory will + // be merged with the generated BUILD file. This allows custom BUILD targets + // to be used in tests, or use BUILD files to draw package boundaries. + KeepBuildFileForDirs []string } func RunBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.RegistrationContext), tc Bp2buildTestCase) { @@ -107,6 +105,18 @@ func RunBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.Regi registerModuleTypes(ctx) ctx.RegisterModuleType(tc.ModuleTypeUnderTest, tc.ModuleTypeUnderTestFactory) + + // A default configuration for tests to not have to specify bp2build_available on top level targets. + bp2buildConfig := android.NewBp2BuildAllowlist().SetDefaultConfig( + allowlists.Bp2BuildConfig{ + android.Bp2BuildTopLevel: allowlists.Bp2BuildDefaultTrueRecursively, + }, + ) + for _, f := range tc.KeepBuildFileForDirs { + bp2buildConfig.SetKeepExistingBuildFile(map[string]bool{ + f: /*recursive=*/ false, + }) + } ctx.RegisterBp2BuildConfig(bp2buildConfig) ctx.RegisterForBazelConversion()