From 912bc8862e125e3d84bb2aa885ffe19da835ecef Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Wed, 8 Mar 2023 12:29:50 -0800 Subject: [PATCH] Use product variables from the overridden apex override_apex's bp2build converter had a bug where it was looking at the product variables for the override_apex module itself instead of for the base module it is overriding. Fixes: 271424349 Test: go test Change-Id: If1e2653d3751fa908faf0ab97dfa2e943ebe98ec --- android/module.go | 2 +- android/variable.go | 5 +- apex/apex.go | 11 ++-- bp2build/apex_conversion_test.go | 87 ++++++++++++++++++++++++++++++++ bp2build/testing.go | 2 +- cc/bp2build.go | 2 +- etc/prebuilt_etc.go | 2 +- 7 files changed, 96 insertions(+), 15 deletions(-) diff --git a/android/module.go b/android/module.go index 58c54642a..773d77be0 100644 --- a/android/module.go +++ b/android/module.go @@ -1334,7 +1334,7 @@ func (attrs *CommonAttributes) fillCommonBp2BuildModuleAttrs(ctx *topDownMutator // Check product variables for `enabled: true` flag override. // Returns a list of the constraint_value targets who enable this override. func productVariableConfigEnableLabels(ctx *topDownMutatorContext) []bazel.Label { - productVariableProps := ProductVariableProperties(ctx) + productVariableProps := ProductVariableProperties(ctx, ctx.Module()) productConfigEnablingTargets := []bazel.Label{} const propName = "Enabled" if productConfigProps, exists := productVariableProps[propName]; exists { diff --git a/android/variable.go b/android/variable.go index f7ac7d682..1b5d5586e 100644 --- a/android/variable.go +++ b/android/variable.go @@ -663,9 +663,8 @@ func (p *ProductConfigProperty) SelectKey() string { type ProductConfigProperties map[string]map[ProductConfigProperty]interface{} // ProductVariableProperties returns a ProductConfigProperties containing only the properties which -// have been set for the module in the given context. -func ProductVariableProperties(ctx BazelConversionPathContext) ProductConfigProperties { - module := ctx.Module() +// have been set for the given module. +func ProductVariableProperties(ctx ArchVariantContext, module Module) ProductConfigProperties { moduleBase := module.base() productConfigProperties := ProductConfigProperties{} diff --git a/apex/apex.go b/apex/apex.go index 88eb72fef..f50687613 100644 --- a/apex/apex.go +++ b/apex/apex.go @@ -50,7 +50,7 @@ func registerApexBuildComponents(ctx android.RegistrationContext) { ctx.RegisterModuleType("apex", BundleFactory) ctx.RegisterModuleType("apex_test", TestApexBundleFactory) ctx.RegisterModuleType("apex_vndk", vndkApexBundleFactory) - ctx.RegisterModuleType("apex_defaults", defaultsFactory) + ctx.RegisterModuleType("apex_defaults", DefaultsFactory) ctx.RegisterModuleType("prebuilt_apex", PrebuiltFactory) ctx.RegisterModuleType("override_apex", OverrideApexFactory) ctx.RegisterModuleType("apex_set", apexSetFactory) @@ -2728,14 +2728,9 @@ type Defaults struct { } // apex_defaults provides defaultable properties to other apex modules. -func defaultsFactory() android.Module { - return DefaultsFactory() -} - -func DefaultsFactory(props ...interface{}) android.Module { +func DefaultsFactory() android.Module { module := &Defaults{} - module.AddProperties(props...) module.AddProperties( &apexBundleProperties{}, &apexTargetBundleProperties{}, @@ -3538,7 +3533,7 @@ func convertWithBp2build(a *apexBundle, ctx android.TopDownMutatorContext) (baze fileContextsLabelAttribute.SetValue(android.BazelLabelForModuleSrcSingle(ctx, *a.properties.File_contexts)) } - productVariableProps := android.ProductVariableProperties(ctx) + productVariableProps := android.ProductVariableProperties(ctx, a) // TODO(b/219503907) this would need to be set to a.MinSdkVersionValue(ctx) but // given it's coming via config, we probably don't want to put it in here. var minSdkVersion bazel.StringAttribute diff --git a/bp2build/apex_conversion_test.go b/bp2build/apex_conversion_test.go index 1c0e56308..73c889f1c 100644 --- a/bp2build/apex_conversion_test.go +++ b/bp2build/apex_conversion_test.go @@ -61,7 +61,10 @@ func registerOverrideApexModuleTypes(ctx android.RegistrationContext) { ctx.RegisterModuleType("android_app_certificate", java.AndroidAppCertificateFactory) ctx.RegisterModuleType("filegroup", android.FileGroupFactory) ctx.RegisterModuleType("apex", apex.BundleFactory) + ctx.RegisterModuleType("apex_defaults", apex.DefaultsFactory) ctx.RegisterModuleType("prebuilt_etc", etc.PrebuiltEtcFactory) + ctx.RegisterModuleType("soong_config_module_type", android.SoongConfigModuleTypeFactory) + ctx.RegisterModuleType("soong_config_string_variable", android.SoongConfigStringVariableDummyFactory) } func TestApexBundleSimple(t *testing.T) { @@ -1359,3 +1362,87 @@ apex_test { }), }}) } + +func TestApexBundle_overridePlusProductVars(t *testing.T) { + // Reproduction of b/271424349 + // Tests that overriding an apex that uses product variables correctly copies the product var + // selects over to the override. + runOverrideApexTestCase(t, Bp2buildTestCase{ + Description: "apex - overriding a module that uses product vars", + ModuleTypeUnderTest: "override_apex", + ModuleTypeUnderTestFactory: apex.OverrideApexFactory, + Blueprint: ` +soong_config_string_variable { + name: "library_linking_strategy", + values: [ + "prefer_static", + ], +} + +soong_config_module_type { + name: "library_linking_strategy_apex_defaults", + module_type: "apex_defaults", + config_namespace: "ANDROID", + variables: ["library_linking_strategy"], + properties: [ + "manifest", + "min_sdk_version", + ], +} + +library_linking_strategy_apex_defaults { + name: "higher_min_sdk_when_prefer_static", + soong_config_variables: { + library_linking_strategy: { + // Use the R min_sdk_version + prefer_static: {}, + // Override the R min_sdk_version to min_sdk_version that supports dcla + conditions_default: { + min_sdk_version: "31", + }, + }, + }, +} + +filegroup { + name: "foo-file_contexts", + srcs: [ + "com.android.apogee-file_contexts", + ], + bazel_module: { bp2build_available: false }, +} + +apex { + name: "foo", + defaults: ["higher_min_sdk_when_prefer_static"], + min_sdk_version: "30", + package_name: "pkg_name", + file_contexts: ":foo-file_contexts", +} +override_apex { + name: "override_foo", + base: ":foo", + package_name: "override_pkg_name", +} +`, + ExpectedBazelTargets: []string{ + MakeBazelTarget("apex", "foo", AttrNameToString{ + "file_contexts": `":foo-file_contexts"`, + "manifest": `"apex_manifest.json"`, + "min_sdk_version": `select({ + "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": "30", + "//conditions:default": "31", + })`, + "package_name": `"pkg_name"`, + }), MakeBazelTarget("apex", "override_foo", AttrNameToString{ + "base_apex_name": `"foo"`, + "file_contexts": `":foo-file_contexts"`, + "manifest": `"apex_manifest.json"`, + "min_sdk_version": `select({ + "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": "30", + "//conditions:default": "31", + })`, + "package_name": `"override_pkg_name"`, + }), + }}) +} diff --git a/bp2build/testing.go b/bp2build/testing.go index 43baf98db..bac53836a 100644 --- a/bp2build/testing.go +++ b/bp2build/testing.go @@ -454,7 +454,7 @@ func (m *customModule) ConvertWithBp2build(ctx android.TopDownMutatorContext) { } } } - productVariableProps := android.ProductVariableProperties(ctx) + productVariableProps := android.ProductVariableProperties(ctx, ctx.Module()) if props, ok := productVariableProps["String_literal_prop"]; ok { for c, p := range props { if val, ok := p.(*string); ok { diff --git a/cc/bp2build.go b/cc/bp2build.go index 1ea8bdad3..8644bf680 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -766,7 +766,7 @@ func bp2BuildParseBaseProps(ctx android.Bp2buildMutatorContext, module *Module) nativeCoverage = BoolPtr(false) } - productVariableProps := android.ProductVariableProperties(ctx) + productVariableProps := android.ProductVariableProperties(ctx, ctx.Module()) (&compilerAttrs).convertProductVariables(ctx, productVariableProps) (&linkerAttrs).convertProductVariables(ctx, productVariableProps) diff --git a/etc/prebuilt_etc.go b/etc/prebuilt_etc.go index b0660df0c..dcd7fdcb9 100644 --- a/etc/prebuilt_etc.go +++ b/etc/prebuilt_etc.go @@ -717,7 +717,7 @@ func (module *PrebuiltEtc) Bp2buildHelper(ctx android.TopDownMutatorContext) *ba } } - for propName, productConfigProps := range android.ProductVariableProperties(ctx) { + for propName, productConfigProps := range android.ProductVariableProperties(ctx, ctx.Module()) { for configProp, propVal := range productConfigProps { if propName == "Src" { props, ok := propVal.(*string)