From 0c4de1f2345d2d986ea2a5a590c23f189bef0290 Mon Sep 17 00:00:00 2001 From: Chris Parsons Date: Thu, 21 Sep 2023 20:36:35 +0000 Subject: [PATCH] Handle already existing targets of different name In other words, if, in bp2build, module "foo" would generate "foo", and "foo_two", and "foo_two" already exists in a build file, bp2build should label "foo" as being unconvertible. Fixes: 301321658 Fixes: 301312582 Bug: 285631638 Test: Unit tests Test: Verified that `m bp2build` results in bit-for-bit identical contents for out/soong/bp2build before and after this change. Change-Id: Icbbdd69fce83579ec9b172d04b2bf1f294698f70 --- android/bazel.go | 22 ++++--- android/config.go | 7 +-- android/module.go | 23 +------ bp2build/aar_conversion_test.go | 3 +- bp2build/apex_conversion_test.go | 6 +- bp2build/build_conversion.go | 61 ++++++++----------- bp2build/build_conversion_test.go | 35 +++++++++++ bp2build/cc_library_shared_conversion_test.go | 6 +- bp2build/java_binary_host_conversion_test.go | 2 +- 9 files changed, 86 insertions(+), 79 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index 8634daba1..732419b2e 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -604,13 +604,6 @@ func registerBp2buildConversionMutator(ctx RegisterMutatorsContext) { } func bp2buildConversionMutator(ctx TopDownMutatorContext) { - if ctx.Config().HasBazelBuildTargetInSource(ctx) { - // Defer to the BUILD target. Generating an additional target would - // cause a BUILD file conflict. - ctx.MarkBp2buildUnconvertible(bp2build_metrics_proto.UnconvertedReasonType_DEFINED_IN_BUILD_FILE, "") - return - } - bModule, ok := ctx.Module().(Bazelable) if !ok { ctx.MarkBp2buildUnconvertible(bp2build_metrics_proto.UnconvertedReasonType_TYPE_UNSUPPORTED, "") @@ -634,11 +627,24 @@ func bp2buildConversionMutator(ctx TopDownMutatorContext) { ctx.MarkBp2buildUnconvertible(bp2build_metrics_proto.UnconvertedReasonType_UNSUPPORTED, "") return } + if ctx.Module().base().GetUnconvertedReason() != nil { + return + } + bModule.ConvertWithBp2build(ctx) - if !ctx.Module().base().IsConvertedByBp2build() && ctx.Module().base().GetUnconvertedReason() == nil { + if len(ctx.Module().base().Bp2buildTargets()) == 0 && ctx.Module().base().GetUnconvertedReason() == nil { panic(fmt.Errorf("illegal bp2build invariant: module '%s' was neither converted nor marked unconvertible", ctx.ModuleName())) } + + for _, targetInfo := range ctx.Module().base().Bp2buildTargets() { + if ctx.Config().HasBazelBuildTargetInSource(targetInfo.TargetPackage(), targetInfo.TargetName()) { + // Defer to the BUILD target. Generating an additional target would + // cause a BUILD file conflict. + ctx.MarkBp2buildUnconvertible(bp2build_metrics_proto.UnconvertedReasonType_DEFINED_IN_BUILD_FILE, targetInfo.TargetName()) + return + } + } } func registerApiBp2buildConversionMutator(ctx RegisterMutatorsContext) { diff --git a/android/config.go b/android/config.go index df282ec20..f9d616d9a 100644 --- a/android/config.go +++ b/android/config.go @@ -2022,10 +2022,9 @@ func (c *config) LogMixedBuild(ctx BaseModuleContext, useBazel bool) { } } -func (c *config) HasBazelBuildTargetInSource(ctx BaseModuleContext) bool { - moduleName := ctx.Module().Name() - for _, buildTarget := range c.bazelTargetsByDir[ctx.ModuleDir()] { - if moduleName == buildTarget { +func (c *config) HasBazelBuildTargetInSource(dir string, target string) bool { + for _, existingTarget := range c.bazelTargetsByDir[dir] { + if target == existingTarget { return true } } diff --git a/android/module.go b/android/module.go index f48af4a03..ceb54de5a 100644 --- a/android/module.go +++ b/android/module.go @@ -565,8 +565,8 @@ type Module interface { AddProperties(props ...interface{}) GetProperties() []interface{} - // IsConvertedByBp2build returns whether this module was converted via bp2build - IsConvertedByBp2build() bool + // If this module should not have bazel BUILD definitions generated by bp2build, + // GetUnconvertedReason returns a reason this is the case. GetUnconvertedReason() *UnconvertedReason // Bp2buildTargets returns the target(s) generated for Bazel via bp2build for this module @@ -1639,35 +1639,16 @@ func (b bp2buildInfo) BazelAttributes() []interface{} { } func (m *ModuleBase) addBp2buildInfo(info bp2buildInfo) { - reason := m.commonProperties.BazelConversionStatus.UnconvertedReason - if reason != nil { - panic(fmt.Errorf("bp2build: internal error trying to convert module '%s' marked unconvertible. Reason type %d: %s", - m.Name(), - reason.ReasonType, - reason.Detail)) - } m.commonProperties.BazelConversionStatus.Bp2buildInfo = append(m.commonProperties.BazelConversionStatus.Bp2buildInfo, info) } func (m *ModuleBase) setBp2buildUnconvertible(reasonType bp2build_metrics_proto.UnconvertedReasonType, detail string) { - if len(m.commonProperties.BazelConversionStatus.Bp2buildInfo) > 0 { - fmt.Println(m.commonProperties.BazelConversionStatus.Bp2buildInfo) - panic(fmt.Errorf("bp2build: internal error trying to mark converted module '%s' as unconvertible. Reason type %d: %s", - m.Name(), - reasonType, - detail)) - } m.commonProperties.BazelConversionStatus.UnconvertedReason = &UnconvertedReason{ ReasonType: int(reasonType), Detail: detail, } } -// IsConvertedByBp2build returns whether this module was converted via bp2build. -func (m *ModuleBase) IsConvertedByBp2build() bool { - return len(m.commonProperties.BazelConversionStatus.Bp2buildInfo) > 0 -} - func (m *ModuleBase) GetUnconvertedReason() *UnconvertedReason { return m.commonProperties.BazelConversionStatus.UnconvertedReason } diff --git a/bp2build/aar_conversion_test.go b/bp2build/aar_conversion_test.go index 59aacbb15..40356a196 100644 --- a/bp2build/aar_conversion_test.go +++ b/bp2build/aar_conversion_test.go @@ -110,7 +110,7 @@ func TestConvertAndroidLibraryImport(t *testing.T) { "import.aar": "", "dep.aar": "", }, - StubbedBuildDefinitions: []string{"static_lib_dep", "prebuilt_static_import_dep"}, + StubbedBuildDefinitions: []string{"static_lib_dep", "static_import_dep", "static_import_dep-neverlink"}, // Bazel's aar_import can only export *_import targets, so we expect // only "static_import_dep" in exports, but both "static_lib_dep" and // "static_import_dep" in deps @@ -125,6 +125,7 @@ android_library_import { // TODO: b/301007952 - This dep is needed because android_library_import must have aars set. android_library_import { name: "static_import_dep", + aars: ["import.aar"], } `, ExpectedBazelTargets: []string{ diff --git a/bp2build/apex_conversion_test.go b/bp2build/apex_conversion_test.go index 60de28cc5..5871d5921 100644 --- a/bp2build/apex_conversion_test.go +++ b/bp2build/apex_conversion_test.go @@ -112,7 +112,7 @@ filegroup { } cc_binary { name: "cc_binary_1"} -sh_binary { name: "sh_binary_2"} +sh_binary { name: "sh_binary_2", src: "foo.sh"} apex { name: "com.android.apogee", @@ -609,7 +609,7 @@ filegroup { } cc_binary { name: "cc_binary_1" } -sh_binary { name: "sh_binary_2" } +sh_binary { name: "sh_binary_2", src: "foo.sh"} apex { name: "com.android.apogee", @@ -736,7 +736,7 @@ filegroup { } cc_binary { name: "cc_binary_1"} -sh_binary { name: "sh_binary_2"} +sh_binary { name: "sh_binary_2", src: "foo.sh"} apex_test { name: "com.android.apogee", diff --git a/bp2build/build_conversion.go b/bp2build/build_conversion.go index 15b77669c..e6941df30 100644 --- a/bp2build/build_conversion.go +++ b/bp2build/build_conversion.go @@ -713,27 +713,32 @@ func GenerateBazelTargets(ctx *CodegenContext, generateFilegroups bool) (convers switch ctx.Mode() { case Bp2Build: - // There are two main ways of converting a Soong module to Bazel: - // 1) Manually handcrafting a Bazel target and associating the module with its label - // 2) Automatically generating with bp2build converters - // - // bp2build converters are used for the majority of modules. - if b, ok := m.(android.Bazelable); ok && b.HasHandcraftedLabel() { - if aModule, ok := m.(android.Module); ok && aModule.IsConvertedByBp2build() { - panic(fmt.Errorf("module %q [%s] [%s] was both converted with bp2build and has a handcrafted label", bpCtx.ModuleName(m), moduleType, dir)) - } - // Handle modules converted to handcrafted targets. - // - // Since these modules are associated with some handcrafted - // target in a BUILD file, we don't autoconvert them. + if aModule, ok := m.(android.Module); ok { + reason := aModule.GetUnconvertedReason() + if reason != nil { + // If this module was force-enabled, cause an error. + if _, ok := ctx.Config().BazelModulesForceEnabledByFlag()[m.Name()]; ok && m.Name() != "" { + err := fmt.Errorf("Force Enabled Module %s not converted", m.Name()) + errs = append(errs, err) + } + + // Log the module isn't to be converted by bp2build. + // TODO: b/291598248 - Log handcrafted modules differently than other unconverted modules. + metrics.AddUnconvertedModule(m, moduleType, dir, *reason) + return + } + if len(aModule.Bp2buildTargets()) == 0 { + panic(fmt.Errorf("illegal bp2build invariant: module '%s' was neither converted nor marked unconvertible", aModule.Name())) + } - // Log the module. - metrics.AddUnconvertedModule(m, moduleType, dir, - android.UnconvertedReason{ - ReasonType: int(bp2build_metrics_proto.UnconvertedReasonType_DEFINED_IN_BUILD_FILE), - }) - } else if aModule, ok := m.(android.Module); ok && aModule.IsConvertedByBp2build() { // Handle modules converted to generated targets. + targets, targetErrs = generateBazelTargets(bpCtx, aModule) + errs = append(errs, targetErrs...) + for _, t := range targets { + // A module can potentially generate more than 1 Bazel + // target, each of a different rule class. + metrics.IncrementRuleClassCount(t.ruleClass) + } // Log the module. metrics.AddConvertedModule(aModule, moduleType, dir) @@ -761,24 +766,6 @@ func GenerateBazelTargets(ctx *CodegenContext, generateFilegroups bool) (convers return } } - targets, targetErrs = generateBazelTargets(bpCtx, aModule) - errs = append(errs, targetErrs...) - for _, t := range targets { - // A module can potentially generate more than 1 Bazel - // target, each of a different rule class. - metrics.IncrementRuleClassCount(t.ruleClass) - } - } else if _, ok := ctx.Config().BazelModulesForceEnabledByFlag()[m.Name()]; ok && m.Name() != "" { - err := fmt.Errorf("Force Enabled Module %s not converted", m.Name()) - errs = append(errs, err) - } else if aModule, ok := m.(android.Module); ok { - reason := aModule.GetUnconvertedReason() - if reason == nil { - panic(fmt.Errorf("module '%s' was neither converted nor marked unconvertible with bp2build", aModule.Name())) - } else { - metrics.AddUnconvertedModule(m, moduleType, dir, *reason) - } - return } else if glib, ok := m.(*bootstrap.GoPackage); ok { targets, targetErrs = generateBazelTargetsGoPackage(bpCtx, glib, nameToGoLibMap) errs = append(errs, targetErrs...) diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index 329c907f8..afbfffacc 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -1994,6 +1994,41 @@ func TestAlreadyPresentBuildTarget(t *testing.T) { }) } +func TestAlreadyPresentOneToManyBuildTarget(t *testing.T) { + bp := ` + custom { + name: "foo", + one_to_many_prop: true, + } + custom { + name: "bar", + } + ` + alreadyPresentBuildFile := + MakeBazelTarget( + "custom", + // one_to_many_prop ensures that foo generates "foo_proto_library_deps". + "foo_proto_library_deps", + 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 (different generated name)", + }) +} + // 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". diff --git a/bp2build/cc_library_shared_conversion_test.go b/bp2build/cc_library_shared_conversion_test.go index bdde996eb..6fa14e345 100644 --- a/bp2build/cc_library_shared_conversion_test.go +++ b/bp2build/cc_library_shared_conversion_test.go @@ -1595,7 +1595,7 @@ func TestCcLibrarySdkVariantUsesStubs(t *testing.T) { Description: "cc_library_shared stubs", ModuleTypeUnderTest: "cc_library_shared", ModuleTypeUnderTestFactory: cc.LibrarySharedFactory, - StubbedBuildDefinitions: []string{"libNoStubs", "libHasApexStubs", "libHasApexAndNdkStubs"}, + StubbedBuildDefinitions: []string{"libNoStubs", "libHasApexStubs", "libHasApexAndNdkStubs", "libHasApexAndNdkStubs.ndk_stub_libs"}, Blueprint: soongCcLibrarySharedPreamble + ` cc_library_shared { name: "libUsesSdk", @@ -1621,9 +1621,7 @@ cc_library_shared { } ndk_library { name: "libHasApexAndNdkStubs", - // TODO: b/301321658 - Stub this once existing-build-file handling can deal with - // modules that generate targets of a different name. - bazel_module: { bp2build_available: false }, + first_version: "28", } `, ExpectedBazelTargets: []string{ diff --git a/bp2build/java_binary_host_conversion_test.go b/bp2build/java_binary_host_conversion_test.go index 7d8ab635a..4271f76a5 100644 --- a/bp2build/java_binary_host_conversion_test.go +++ b/bp2build/java_binary_host_conversion_test.go @@ -114,7 +114,7 @@ func TestJavaBinaryHostLibs(t *testing.T) { runJavaBinaryHostTestCase(t, Bp2buildTestCase{ Description: "java_binary_host with srcs, libs.", Filesystem: testFs, - StubbedBuildDefinitions: []string{"prebuilt_java-lib-dep-1"}, + StubbedBuildDefinitions: []string{"java-lib-dep-1", "java-lib-dep-1-neverlink"}, Blueprint: `java_binary_host { name: "java-binary-host-libs", libs: ["java-lib-dep-1"],