From 7123cc5370a38983ee6325b5f5f6df19f4e4f10b Mon Sep 17 00:00:00 2001 From: Chris Parsons Date: Wed, 4 Oct 2023 22:27:40 +0000 Subject: [PATCH] Dont write data attrs for cc lib rules The data attributes for cc library rules is dropped by their starlark macros, so serves no real purpose. Furthermore, this unused dependency proves problematic for allowlist v2, as there are many cases at HEAD where the corner-case "requires" dependency would otherwise mark a module as unconvertible. Fixes: 303307456 Test: New unit test Test: Manual verification to unblock allowlist v2 `bp2build.sh` runs in AOSP Change-Id: I6ca6104b958d2b428fc2ca5b3fa794106571acca --- android/module.go | 30 +++++++++----- bp2build/cc_library_conversion_test.go | 54 ++++++++++++++++++++++++++ cc/library.go | 18 +++++++-- 3 files changed, 89 insertions(+), 13 deletions(-) diff --git a/android/module.go b/android/module.go index 74b8cb8ff..ce6c78d35 100644 --- a/android/module.go +++ b/android/module.go @@ -1272,6 +1272,22 @@ func InitCommonOSAndroidMultiTargetsArchModule(m Module, hod HostOrDeviceSupport m.base().commonProperties.CreateCommonOSVariant = true } +func (attrs *CommonAttributes) getRequiredWithoutCycles(ctx *bottomUpMutatorContext, props *commonProperties) []string { + // Treat `required` as if it's empty if data should be skipped for this target, + // as `required` is only used for the `data` attribute at this time, and we want + // to avoid lookups of labels that won't actually be dependencies of this target. + // TODO: b/202299295 - Refactor this to use `required` dependencies, once they + // are handled other than passing to `data`. + if proptools.Bool(attrs.SkipData) { + return []string{} + } + // The required property can contain the module itself. This causes a cycle + // when generated as the 'data' label list attribute in Bazel. Remove it if + // it exists. See b/247985196. + _, requiredWithoutCycles := RemoveFromList(ctx.ModuleName(), props.Required) + return FirstUniqueStrings(requiredWithoutCycles) +} + func (attrs *CommonAttributes) fillCommonBp2BuildModuleAttrs(ctx *bottomUpMutatorContext, enabledPropertyOverrides bazel.BoolAttribute) constraintAttributes { @@ -1340,18 +1356,13 @@ func (attrs *CommonAttributes) fillCommonBp2BuildModuleAttrs(ctx *bottomUpMutato attrs.Applicable_licenses = bazel.MakeLabelListAttribute(BazelLabelForModuleDeps(ctx, mod.commonProperties.Licenses)) - // The required property can contain the module itself. This causes a cycle - // when generated as the 'data' label list attribute in Bazel. Remove it if - // it exists. See b/247985196. - _, requiredWithoutCycles := RemoveFromList(ctx.ModuleName(), mod.commonProperties.Required) - requiredWithoutCycles = FirstUniqueStrings(requiredWithoutCycles) + requiredWithoutCycles := attrs.getRequiredWithoutCycles(ctx, &mod.commonProperties) required := depsToLabelList(requiredWithoutCycles) archVariantProps := mod.GetArchVariantProperties(ctx, &commonProperties{}) for axis, configToProps := range archVariantProps { for config, _props := range configToProps { if archProps, ok := _props.(*commonProperties); ok { - _, requiredWithoutCycles := RemoveFromList(ctx.ModuleName(), archProps.Required) - requiredWithoutCycles = FirstUniqueStrings(requiredWithoutCycles) + requiredWithoutCycles := attrs.getRequiredWithoutCycles(ctx, archProps) required.SetSelectValue(axis, config, depsToLabelList(requiredWithoutCycles).Value) if !neitherHostNorDevice { if archProps.Enabled != nil { @@ -1408,9 +1419,8 @@ func (attrs *CommonAttributes) fillCommonBp2BuildModuleAttrs(ctx *bottomUpMutato platformEnabledAttribute.Add(&l) } - if !proptools.Bool(attrs.SkipData) { - attrs.Data.Append(required) - } + attrs.Data.Append(required) + // SkipData is not an attribute of any Bazel target // Set this to nil so that it does not appear in the generated build file attrs.SkipData = nil diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index bc88f86b7..0685de1db 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -5245,3 +5245,57 @@ versioned_ndk_headers { } runCcLibraryTestCase(t, tc) } + +// Regression test for b/303307456. +// TODO: b/202299295 - Remove this test when cc rules have proper support +// for the `required` property +func TestCcModules_requiredProperty(t *testing.T) { + runCcLibrarySharedTestCase(t, Bp2buildTestCase{ + Description: "cc modules do not use the required property", + Filesystem: map[string]string{ + "foo.c": "", + "bar.c": "", + }, + Blueprint: soongCcLibraryPreamble + ` +cc_library { + name: "foo_both", + srcs: ["foo.c"], + include_build_directory: false, + required: ["bar"], +} +cc_library_shared { + name: "foo_shared", + srcs: ["foo.c"], + include_build_directory: false, + required: ["bar"], +} +cc_library_static { + name: "foo_static", + srcs: ["foo.c"], + include_build_directory: false, + required: ["bar"], +} +cc_library_static { + name: "bar", + srcs: ["bar.c"], + include_build_directory: false, +}`, + ExpectedBazelTargets: []string{ + MakeBazelTarget("cc_library_static", "foo_both_bp2build_cc_library_static", AttrNameToString{ + "srcs_c": `["foo.c"]`, + }), + MakeBazelTarget("cc_library_shared", "foo_both", AttrNameToString{ + "srcs_c": `["foo.c"]`, + }), + MakeBazelTarget("cc_library_shared", "foo_shared", AttrNameToString{ + "srcs_c": `["foo.c"]`, + }), + MakeBazelTarget("cc_library_static", "foo_static", AttrNameToString{ + "srcs_c": `["foo.c"]`, + }), + MakeBazelTarget("cc_library_static", "bar", AttrNameToString{ + "srcs_c": `["bar.c"]`, + }), + }, + }) +} diff --git a/cc/library.go b/cc/library.go index 90d91ca0e..6acd7ae56 100644 --- a/cc/library.go +++ b/cc/library.go @@ -468,12 +468,16 @@ func libraryBp2Build(ctx android.Bp2buildMutatorContext, m *Module) { android.CommonAttributes{ Name: m.Name() + "_bp2build_cc_library_static", Tags: tagsForStaticVariant, + // TODO: b/303307456 - Remove this when data is properly supported in cc rules. + SkipData: proptools.BoolPtr(true), }, staticTargetAttrs, staticAttrs.Enabled) ctx.CreateBazelTargetModuleWithRestrictions(sharedProps, android.CommonAttributes{ Name: m.Name(), Tags: tagsForSharedVariant, + // TODO: b/303307456 - Remove this when data is properly supported in cc rules. + SkipData: proptools.BoolPtr(true), }, sharedTargetAttrs, sharedAttrs.Enabled) @@ -496,8 +500,11 @@ func createStubsBazelTargetIfNeeded(ctx android.Bp2buildMutatorContext, m *Modul Deps: baseAttributes.deps, Api_surface: proptools.StringPtr("module-libapi"), } - ctx.CreateBazelTargetModule(stubSuitesProps, - android.CommonAttributes{Name: m.Name() + "_stub_libs"}, + ctx.CreateBazelTargetModule(stubSuitesProps, android.CommonAttributes{ + Name: m.Name() + "_stub_libs", + // TODO: b/303307456 - Remove this when data is properly supported in cc rules. + SkipData: proptools.BoolPtr(true), + }, stubSuitesAttrs) // Add alias for the stub shared_library in @api_surfaces repository @@ -2935,7 +2942,12 @@ func sharedOrStaticLibraryBp2Build(ctx android.Bp2buildMutatorContext, module *M tags := android.ApexAvailableTagsWithoutTestApexes(ctx, module) - ctx.CreateBazelTargetModule(props, android.CommonAttributes{Name: module.Name(), Tags: tags}, attrs) + ctx.CreateBazelTargetModule(props, android.CommonAttributes{ + Name: module.Name(), + Tags: tags, + // TODO: b/303307456 - Remove this when data is properly supported in cc rules. + SkipData: proptools.BoolPtr(true), + }, attrs) } type includesAttributes struct {