StrixOS/build_soong
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bp2build: don't generate filegroups (or error) if it contains a file

Browse Source 
with the same name.

Also add capability to test for errors raised in bp2build mutators /
contexts.

This CL does two things to filegroups:

1) If the filegroup has only 1 source file with the same name as itself,
don't generate a filegroup target. Instead, dependents will depend
directly on the Bazel file target instead.

2) If the filegroup has more than 1 source file and 1 of them has the
same name as itself, the bp2build mutator will error out. If bp2build
on CI passes, it means that the source tree / product we're testing
against does not have such a case (which seems to be true for most
source trees).

Either way, this will allow us to be unblocked for most of the errant
filegroups (case 1) in the tree.

Test: CI
Test: New test cases in filegroup_conversion_test.go

Fixes: 194762573
Change-Id: I830c53efc8808569afe3c5f9f08436855bcdafed
This commit is contained in:
Jingwen Chen
2021-09-02 11:44:42 +00:00
parent 4b221b3cf9
commit 5146ac02a1
5 changed files with 174 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
android/filegroup.go
View File

@@ -42,6 +42,27 @@ func FilegroupBp2Build(ctx TopDownMutatorContext) {
srcs := bazel.MakeLabelListAttribute(
BazelLabelForModuleSrcExcludes(ctx, fg.properties.Srcs, fg.properties.Exclude_srcs))
// For Bazel compatibility, don't generate the filegroup if there is only 1
// source file, and that the source file is named the same as the module
// itself. In Bazel, eponymous filegroups like this would be an error.
//
// Instead, dependents on this single-file filegroup can just depend
// on the file target, instead of rule target, directly.
//
// You may ask: what if a filegroup has multiple files, and one of them
// shares the name? The answer: we haven't seen that in the wild, and
// should lock Soong itself down to prevent the behavior. For now,
// we raise an error if bp2build sees this problem.
for _, f := range srcs.Value.Includes {
if f.Label == fg.Name() {
if len(srcs.Value.Includes) > 1 {
ctx.ModuleErrorf("filegroup '%s' cannot contain a file with the same name", fg.Name())
}
return
}
}
attrs := &bazelFilegroupAttributes{
Srcs: srcs,
}

1
bp2build/Android.bp
View File

@@ -39,6 +39,7 @@ bootstrap_go_package {
"cc_library_static_conversion_test.go",
"cc_object_conversion_test.go",
"conversion_test.go",
"filegroup_conversion_test.go",
"performance_test.go",
"prebuilt_etc_conversion_test.go",
"python_binary_conversion_test.go",

147
bp2build/build_conversion_test.go
View File

@@ -218,13 +218,9 @@ func TestGenerateSoongModuleTargets(t *testing.T) {
}
func TestGenerateBazelTargetModules(t *testing.T) {
testCases := []struct {
name string
bp string
expectedBazelTargets []string
}{
testCases := []bp2buildTestCase{
{
bp: `custom {
blueprint: `custom {
name: "foo",
string_list_prop: ["a", "b"],
string_prop: "a",
@@ -241,7 +237,7 @@ func TestGenerateBazelTargetModules(t *testing.T) {
},
},
{
bp: `custom {
blueprint: `custom {
name: "control_characters",
string_list_prop: ["\t", "\n"],
string_prop: "a\t\n\r",
@@ -258,7 +254,7 @@ func TestGenerateBazelTargetModules(t *testing.T) {
},
},
{
bp: `custom {
blueprint: `custom {
name: "has_dep",
arch_paths: [":dep"],
bazel_module: { bp2build_available: true },
@@ -280,7 +276,7 @@ custom {
},
},
{
bp: `custom {
blueprint: `custom {
name: "arch_paths",
arch: {
x86: {
@@ -299,7 +295,7 @@ custom {
},
},
{
bp: `custom {
blueprint: `custom {
name: "has_dep",
arch: {
x86: {
@@ -331,17 +327,17 @@ custom {
dir := "."
for _, testCase := range testCases {
config := android.TestConfig(buildDir, nil, testCase.bp, nil)
config := android.TestConfig(buildDir, nil, testCase.blueprint, nil)
ctx := android.NewTestContext(config)
registerCustomModuleForBp2buildConversion(ctx)
_, errs := ctx.ParseFileList(dir, []string{"Android.bp"})
if errored(t, "", errs) {
if errored(t, testCase, errs) {
continue
}
_, errs = ctx.ResolveDependencies(config)
if errored(t, "", errs) {
if errored(t, testCase, errs) {
continue
}
@@ -548,23 +544,13 @@ genrule {
}`,
}
testCases := []struct {
description string
moduleTypeUnderTest string
moduleTypeUnderTestFactory android.ModuleFactory
moduleTypeUnderTestBp2BuildMutator func(android.TopDownMutatorContext)
preArchMutators []android.RegisterMutatorFunc
bp string
expectedBazelTargets []string
fs map[string]string
dir string
}{
testCases := []bp2buildTestCase{
{
description: "filegroup with does not specify srcs",
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
bazel_module: { bp2build_available: true },
}`,
@@ -579,7 +565,7 @@ genrule {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
srcs: [],
bazel_module: { bp2build_available: true },
@@ -595,7 +581,7 @@ genrule {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
srcs: ["a", "b"],
bazel_module: { bp2build_available: true },
@@ -614,7 +600,7 @@ genrule {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
srcs: ["a", "b"],
exclude_srcs: ["a"],
@@ -631,7 +617,7 @@ genrule {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "foo",
srcs: ["**/*.txt"],
bazel_module: { bp2build_available: true },
@@ -645,7 +631,7 @@ genrule {
],
)`,
},
fs: map[string]string{
filesystem: map[string]string{
"other/a.txt": "",
"other/b.txt": "",
"other/subdir/a.txt": "",
@@ -657,7 +643,7 @@ genrule {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "foo",
srcs: ["a.txt"],
bazel_module: { bp2build_available: true },
@@ -672,7 +658,7 @@ genrule {
],
)`,
},
fs: map[string]string{
filesystem: map[string]string{
"other/Android.bp": `filegroup {
name: "fg_foo",
srcs: ["**/*.txt"],
@@ -689,7 +675,7 @@ genrule {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "foobar",
srcs: [
":foo",
@@ -705,7 +691,7 @@ genrule {
],
)`,
},
fs: map[string]string{
filesystem: map[string]string{
"other/Android.bp": `filegroup {
name: "foo",
srcs: ["a", "b"],
@@ -717,7 +703,7 @@ genrule {
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo.tool",
out: ["foo_tool.out"],
srcs: ["foo_tool.in"],
@@ -754,7 +740,7 @@ genrule {
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo.tools",
out: ["foo_tool.out", "foo_tool2.out"],
srcs: ["foo_tool.in"],
@@ -793,7 +779,7 @@ genrule {
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo",
out: ["foo.out"],
srcs: ["foo.in"],
@@ -809,14 +795,14 @@ genrule {
tools = ["//other:foo.tool"],
)`,
},
fs: otherGenruleBp,
filesystem: otherGenruleBp,
},
{
description: "genrule srcs using $(locations //absolute:label)",
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo",
out: ["foo.out"],
srcs: [":other.tool"],
@@ -832,14 +818,14 @@ genrule {
tools = ["//other:foo.tool"],
)`,
},
fs: otherGenruleBp,
filesystem: otherGenruleBp,
},
{
description: "genrule using $(location) label should substitute first tool label automatically",
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo",
out: ["foo.out"],
srcs: ["foo.in"],
@@ -858,14 +844,14 @@ genrule {
],
)`,
},
fs: otherGenruleBp,
filesystem: otherGenruleBp,
},
{
description: "genrule using $(locations) label should substitute first tool label automatically",
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo",
out: ["foo.out"],
srcs: ["foo.in"],
@@ -884,14 +870,14 @@ genrule {
],
)`,
},
fs: otherGenruleBp,
filesystem: otherGenruleBp,
},
{
description: "genrule without tools or tool_files can convert successfully",
moduleTypeUnderTest: "genrule",
moduleTypeUnderTestFactory: genrule.GenRuleFactory,
moduleTypeUnderTestBp2BuildMutator: genrule.GenruleBp2Build,
bp: `genrule {
blueprint: `genrule {
name: "foo",
out: ["foo.out"],
srcs: ["foo.in"],
@@ -914,24 +900,24 @@ genrule {
toParse := []string{
"Android.bp",
}
for f, content := range testCase.fs {
for f, content := range testCase.filesystem {
if strings.HasSuffix(f, "Android.bp") {
toParse = append(toParse, f)
}
fs[f] = []byte(content)
}
config := android.TestConfig(buildDir, nil, testCase.bp, fs)
config := android.TestConfig(buildDir, nil, testCase.blueprint, fs)
ctx := android.NewTestContext(config)
ctx.RegisterModuleType(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestFactory)
ctx.RegisterBp2BuildMutator(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestBp2BuildMutator)
ctx.RegisterForBazelConversion()
_, errs := ctx.ParseFileList(dir, toParse)
if errored(t, testCase.description, errs) {
if errored(t, testCase, errs) {
continue
}
_, errs = ctx.ResolveDependencies(config)
if errored(t, testCase.description, errs) {
if errored(t, testCase, errs) {
continue
}
@@ -1353,30 +1339,20 @@ filegroup { name: "opt-out-h", bazel_module: { bp2build_available: false } }
}
func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
testCases := []struct {
description string
moduleTypeUnderTest string
moduleTypeUnderTestFactory android.ModuleFactory
moduleTypeUnderTestBp2BuildMutator func(android.TopDownMutatorContext)
preArchMutators []android.RegisterMutatorFunc
bp string
expectedBazelTargets []string
fs map[string]string
dir string
}{
testCases := []bp2buildTestCase{
{
description: "filegroup bazel_module.label",
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
bazel_module: { label: "//other:fg_foo" },
}`,
expectedBazelTargets: []string{
`// BUILD file`,
},
fs: map[string]string{
filesystem: map[string]string{
"other/BUILD.bazel": `// BUILD file`,
},
},
@@ -1385,7 +1361,7 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
bazel_module: { label: "//other:fg_foo" },
}
@@ -1397,7 +1373,7 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
expectedBazelTargets: []string{
`// BUILD file`,
},
fs: map[string]string{
filesystem: map[string]string{
"other/BUILD.bazel": `// BUILD file`,
},
},
@@ -1407,8 +1383,8 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
dir: "other",
bp: ``,
fs: map[string]string{
blueprint: ``,
filesystem: map[string]string{
"other/Android.bp": `filegroup {
name: "fg_foo",
bazel_module: {
@@ -1434,7 +1410,7 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
bazel_module: {
label: "//other:fg_foo",
@@ -1453,7 +1429,7 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
)`,
`// BUILD file`,
},
fs: map[string]string{
filesystem: map[string]string{
"other/BUILD.bazel": `// BUILD file`,
},
},
@@ -1466,24 +1442,24 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
toParse := []string{
"Android.bp",
}
for f, content := range testCase.fs {
for f, content := range testCase.filesystem {
if strings.HasSuffix(f, "Android.bp") {
toParse = append(toParse, f)
}
fs[f] = []byte(content)
}
config := android.TestConfig(buildDir, nil, testCase.bp, fs)
config := android.TestConfig(buildDir, nil, testCase.blueprint, fs)
ctx := android.NewTestContext(config)
ctx.RegisterModuleType(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestFactory)
ctx.RegisterBp2BuildMutator(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestBp2BuildMutator)
ctx.RegisterForBazelConversion()
_, errs := ctx.ParseFileList(dir, toParse)
if errored(t, testCase.description, errs) {
if errored(t, testCase, errs) {
return
}
_, errs = ctx.ResolveDependencies(config)
if errored(t, testCase.description, errs) {
if errored(t, testCase, errs) {
return
}
@@ -1517,22 +1493,13 @@ func TestCombineBuildFilesBp2buildTargets(t *testing.T) {
}
func TestGlobExcludeSrcs(t *testing.T) {
testCases := []struct {
description string
moduleTypeUnderTest string
moduleTypeUnderTestFactory android.ModuleFactory
moduleTypeUnderTestBp2BuildMutator func(android.TopDownMutatorContext)
bp string
expectedBazelTargets []string
fs map[string]string
dir string
}{
testCases := []bp2buildTestCase{
{
description: "filegroup top level exclude_srcs",
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: `filegroup {
blueprint: `filegroup {
name: "fg_foo",
srcs: ["**/*.txt"],
exclude_srcs: ["c.txt"],
@@ -1548,7 +1515,7 @@ func TestGlobExcludeSrcs(t *testing.T) {
],
)`,
},
fs: map[string]string{
filesystem: map[string]string{
"a.txt": "",
"b.txt": "",
"c.txt": "",
@@ -1562,9 +1529,9 @@ func TestGlobExcludeSrcs(t *testing.T) {
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
bp: "",
blueprint: "",
dir: "dir",
fs: map[string]string{
filesystem: map[string]string{
"dir/Android.bp": `filegroup {
name: "fg_foo",
srcs: ["**/*.txt"],
@@ -1596,24 +1563,24 @@ func TestGlobExcludeSrcs(t *testing.T) {
toParse := []string{
"Android.bp",
}
for f, content := range testCase.fs {
for f, content := range testCase.filesystem {
if strings.HasSuffix(f, "Android.bp") {
toParse = append(toParse, f)
}
fs[f] = []byte(content)
}
config := android.TestConfig(buildDir, nil, testCase.bp, fs)
config := android.TestConfig(buildDir, nil, testCase.blueprint, fs)
ctx := android.NewTestContext(config)
ctx.RegisterModuleType(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestFactory)
ctx.RegisterBp2BuildMutator(testCase.moduleTypeUnderTest, testCase.moduleTypeUnderTestBp2BuildMutator)
ctx.RegisterForBazelConversion()
_, errs := ctx.ParseFileList(dir, toParse)
if errored(t, testCase.description, errs) {
if errored(t, testCase, errs) {
continue
}
_, errs = ctx.ResolveDependencies(config)
if errored(t, testCase.description, errs) {
if errored(t, testCase, errs) {
continue
}

62
bp2build/filegroup_conversion_test.go Normal file
View File

@@ -0,0 +1,62 @@
// Copyright 2021 Google Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package bp2build
import (
"android/soong/android"
"fmt"
"testing"
)
func runFilegroupTestCase(t *testing.T, tc bp2buildTestCase) {
t.Helper()
runBp2BuildTestCase(t, registerFilegroupModuleTypes, tc)
}
func registerFilegroupModuleTypes(ctx android.RegistrationContext) {}
func TestFilegroupSameNameAsFile_OneFile(t *testing.T) {
runFilegroupTestCase(t, bp2buildTestCase{
description: "filegroup - same name as file, with one file",
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
filesystem: map[string]string{},
blueprint: `
filegroup {
name: "foo",
srcs: ["foo"],
}
`,
expectedBazelTargets: []string{}})
}
func TestFilegroupSameNameAsFile_MultipleFiles(t *testing.T) {
runFilegroupTestCase(t, bp2buildTestCase{
description: "filegroup - same name as file, with multiple files",
moduleTypeUnderTest: "filegroup",
moduleTypeUnderTestFactory: android.FileGroupFactory,
moduleTypeUnderTestBp2BuildMutator: android.FilegroupBp2Build,
filesystem: map[string]string{},
blueprint: `
filegroup {
name: "foo",
srcs: ["foo", "bar"],
}
`,
expectedErr: fmt.Errorf("filegroup 'foo' cannot contain a file with the same name"),
})
}

39
bp2build/testing.go
View File

@@ -36,14 +36,35 @@ var (
buildDir string
)
func errored(t *testing.T, desc string, errs []error) bool {
func checkError(t *testing.T, errs []error, expectedErr error) bool {
t.Helper()
// expectedErr is not nil, find it in the list of errors
if len(errs) != 1 {
t.Errorf("Expected only 1 error, got %d: %q", len(errs), errs)
}
if errs[0].Error() == expectedErr.Error() {
return true
}
return false
}
func errored(t *testing.T, tc bp2buildTestCase, errs []error) bool {
t.Helper()
if tc.expectedErr != nil {
// Rely on checkErrors, as this test case is expected to have an error.
return false
}
if len(errs) > 0 {
for _, err := range errs {
t.Errorf("%s: %s", desc, err)
t.Errorf("%s: %s", tc.description, err)
}
return true
}
// All good, continue execution.
return false
}
@@ -61,6 +82,7 @@ type bp2buildTestCase struct {
expectedBazelTargets []string
filesystem map[string]string
dir string
expectedErr error
}
func runBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.RegistrationContext), tc bp2buildTestCase) {
@@ -85,12 +107,17 @@ func runBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.Regi
ctx.RegisterBp2BuildMutator(tc.moduleTypeUnderTest, tc.moduleTypeUnderTestBp2BuildMutator)
ctx.RegisterForBazelConversion()
_, errs := ctx.ParseFileList(dir, toParse)
if errored(t, tc.description, errs) {
_, parseErrs := ctx.ParseFileList(dir, toParse)
if errored(t, tc, parseErrs) {
return
}
_, errs = ctx.ResolveDependencies(config)
if errored(t, tc.description, errs) {
_, resolveDepsErrs := ctx.ResolveDependencies(config)
if errored(t, tc, resolveDepsErrs) {
return
}
errs := append(parseErrs, resolveDepsErrs...)
if tc.expectedErr != nil && checkError(t, errs, tc.expectedErr) {
return
}