From 0e33c9d772a5652ee30edda5cb642bb62fe8f574 Mon Sep 17 00:00:00 2001 From: Sam Delmerico Date: Fri, 7 Jan 2022 20:39:21 +0000 Subject: [PATCH] Support `enabled` flag in product variable config Some Android.bp modules have `enabled: false` but only use a product variable such as `source_build` to enable the module. Currently b2build does not handle this case at all. This commit adds the functionality to support this use case. Also, remove `__enabled` suffix in ProductVariable SelectKey. Bug: 210546943 Test: go test ./bp2build Topic: use_enabled_flag_product_variable_config Change-Id: I459c17a84c172df010666391066bf4d11d19253e --- android/module.go | 63 ++++++- android/variable.go | 12 +- ...oong_config_module_type_conversion_test.go | 173 ++++++++++++++++-- 3 files changed, 227 insertions(+), 21 deletions(-) diff --git a/android/module.go b/android/module.go index c2fa84847..50537209c 100644 --- a/android/module.go +++ b/android/module.go @@ -1139,20 +1139,71 @@ func (attrs *CommonAttributes) fillCommonBp2BuildModuleAttrs(ctx *topDownMutator } } - data.Append(required) + productConfigEnabledLabels := []bazel.Label{} + if !proptools.BoolDefault(enabledProperty.Value, true) { + // If the module is not enabled by default, then we can check if a + // product variable enables it + productConfigEnabledLabels = productVariableConfigEnableLabels(ctx) - var err error - constraints := constraintAttributes{} - constraints.Target_compatible_with, err = enabledProperty.ToLabelListAttribute( + if len(productConfigEnabledLabels) > 0 { + // In this case, an existing product variable configuration overrides any + // module-level `enable: false` definition + newValue := true + enabledProperty.Value = &newValue + } + } + + productConfigEnabledAttribute := bazel.MakeLabelListAttribute(bazel.LabelList{ + productConfigEnabledLabels, nil, + }) + + platformEnabledAttribute, err := enabledProperty.ToLabelListAttribute( bazel.LabelList{[]bazel.Label{bazel.Label{Label: "@platforms//:incompatible"}}, nil}, bazel.LabelList{[]bazel.Label{}, nil}) - if err != nil { - ctx.ModuleErrorf("Error processing enabled attribute: %s", err) + ctx.ModuleErrorf("Error processing platform enabled attribute: %s", err) } + + data.Append(required) + + constraints := constraintAttributes{} + moduleEnableConstraints := bazel.LabelListAttribute{} + moduleEnableConstraints.Append(platformEnabledAttribute) + moduleEnableConstraints.Append(productConfigEnabledAttribute) + constraints.Target_compatible_with = moduleEnableConstraints + return constraints } +// 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) + productConfigEnablingTargets := []bazel.Label{} + const propName = "Enabled" + if productConfigProps, exists := productVariableProps[propName]; exists { + for productConfigProp, prop := range productConfigProps { + flag, ok := prop.(*bool) + if !ok { + ctx.ModuleErrorf("Could not convert product variable %s property", proptools.PropertyNameForField(propName)) + } + + if *flag { + axis := productConfigProp.ConfigurationAxis() + targetLabel := axis.SelectKey(productConfigProp.SelectKey()) + productConfigEnablingTargets = append(productConfigEnablingTargets, bazel.Label{ + Label: targetLabel, + }) + } else { + // TODO(b/210546943): handle negative case where `enabled: false` + ctx.ModuleErrorf("`enabled: false` is not currently supported for configuration variables. See b/210546943", proptools.PropertyNameForField(propName)) + } + } + } + + return productConfigEnablingTargets +} + // A ModuleBase object contains the properties that are common to all Android // modules. It should be included as an anonymous field in every module // struct definition. InitAndroidModule should then be called from the module's diff --git a/android/variable.go b/android/variable.go index b300267ba..40dd2d82c 100644 --- a/android/variable.go +++ b/android/variable.go @@ -601,10 +601,16 @@ func (p *ProductConfigProperty) SelectKey() string { value := p.FullConfig if value == p.Name { - value = "enabled" + value = "" } - // e.g. acme__feature1__enabled, android__board__soc_a - return strings.ToLower(strings.Join([]string{p.Namespace, p.Name, value}, "__")) + + // e.g. acme__feature1, android__board__soc_a + selectKey := strings.ToLower(strings.Join([]string{p.Namespace, p.Name}, "__")) + if value != "" { + selectKey = strings.ToLower(strings.Join([]string{selectKey, value}, "__")) + } + + return selectKey } // ProductConfigProperties is a map of maps to group property values according diff --git a/bp2build/soong_config_module_type_conversion_test.go b/bp2build/soong_config_module_type_conversion_test.go index f1489aab9..b1e1fb2ee 100644 --- a/bp2build/soong_config_module_type_conversion_test.go +++ b/bp2build/soong_config_module_type_conversion_test.go @@ -68,7 +68,7 @@ custom_cc_library_static { expectedBazelTargets: []string{`cc_library_static( name = "foo", copts = select({ - "//build/bazel/product_variables:acme__feature1__enabled": ["-DFEATURE1"], + "//build/bazel/product_variables:acme__feature1": ["-DFEATURE1"], "//conditions:default": ["-DDEFAULT1"], }), local_includes = ["."], @@ -116,7 +116,7 @@ custom_cc_library_static { expectedBazelTargets: []string{`cc_library_static( name = "foo", copts = select({ - "//build/bazel/product_variables:acme__feature1__enabled": ["-DFEATURE1"], + "//build/bazel/product_variables:acme__feature1": ["-DFEATURE1"], "//conditions:default": ["-DDEFAULT1"], }), local_includes = ["."], @@ -240,10 +240,10 @@ custom_cc_library_static { "//build/bazel/product_variables:acme__board__soc_b": ["-DSOC_B"], "//conditions:default": ["-DSOC_DEFAULT"], }) + select({ - "//build/bazel/product_variables:acme__feature1__enabled": ["-DFEATURE1"], + "//build/bazel/product_variables:acme__feature1": ["-DFEATURE1"], "//conditions:default": ["-DDEFAULT1"], }) + select({ - "//build/bazel/product_variables:acme__feature2__enabled": ["-DFEATURE2"], + "//build/bazel/product_variables:acme__feature2": ["-DFEATURE2"], "//conditions:default": ["-DDEFAULT2"], }), local_includes = ["."], @@ -367,7 +367,7 @@ cc_library_static { expectedBazelTargets: []string{`cc_library_static( name = "lib", copts = select({ - "//build/bazel/product_variables:vendor_foo__feature__enabled": [ + "//build/bazel/product_variables:vendor_foo__feature": [ "-cflag_feature_2", "-cflag_feature_1", ], @@ -446,11 +446,11 @@ cc_library_static { expectedBazelTargets: []string{`cc_library_static( name = "lib", asflags = select({ - "//build/bazel/product_variables:acme__feature__enabled": ["-asflag_bar"], + "//build/bazel/product_variables:acme__feature": ["-asflag_bar"], "//conditions:default": ["-asflag_default_bar"], }), copts = select({ - "//build/bazel/product_variables:acme__feature__enabled": [ + "//build/bazel/product_variables:acme__feature": [ "-cflag_foo", "-cflag_bar", ], @@ -465,11 +465,11 @@ cc_library_static { `cc_library_static( name = "lib2", asflags = select({ - "//build/bazel/product_variables:acme__feature__enabled": ["-asflag_bar"], + "//build/bazel/product_variables:acme__feature": ["-asflag_bar"], "//conditions:default": ["-asflag_default_bar"], }), copts = select({ - "//build/bazel/product_variables:acme__feature__enabled": [ + "//build/bazel/product_variables:acme__feature": [ "-cflag_bar", "-cflag_foo", ], @@ -561,13 +561,13 @@ cc_library_static { expectedBazelTargets: []string{`cc_library_static( name = "lib", copts = select({ - "//build/bazel/product_variables:vendor_bar__feature__enabled": ["-DVENDOR_BAR_FEATURE"], + "//build/bazel/product_variables:vendor_bar__feature": ["-DVENDOR_BAR_FEATURE"], "//conditions:default": ["-DVENDOR_BAR_DEFAULT"], }) + select({ - "//build/bazel/product_variables:vendor_foo__feature__enabled": ["-DVENDOR_FOO_FEATURE"], + "//build/bazel/product_variables:vendor_foo__feature": ["-DVENDOR_FOO_FEATURE"], "//conditions:default": ["-DVENDOR_FOO_DEFAULT"], }) + select({ - "//build/bazel/product_variables:vendor_qux__feature__enabled": ["-DVENDOR_QUX_FEATURE"], + "//build/bazel/product_variables:vendor_qux__feature": ["-DVENDOR_QUX_FEATURE"], "//conditions:default": ["-DVENDOR_QUX_DEFAULT"], }), local_includes = ["."], @@ -834,3 +834,152 @@ cc_library { name: "lib_default", bazel_module: { bp2build_available: false } } srcs = ["main.cc"], )`}}) } + +func TestSoongConfigModuleType_ProductVariableConfigWithPlatformConfig(t *testing.T) { + bp := ` +soong_config_bool_variable { + name: "special_build", +} + +soong_config_module_type { + name: "alphabet_cc_defaults", + module_type: "cc_defaults", + config_namespace: "alphabet_module", + bool_variables: ["special_build"], + properties: ["enabled"], +} + +alphabet_cc_defaults { + name: "alphabet_sample_cc_defaults", + soong_config_variables: { + special_build: { + enabled: true, + }, + }, +} + +cc_binary { + name: "alphabet_binary", + srcs: ["main.cc"], + defaults: ["alphabet_sample_cc_defaults"], + enabled: false, + arch: { + x86_64: { + enabled: false, + }, + }, + target: { + darwin: { + enabled: false, + }, + }, +}` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for library_linking_strategy", + moduleTypeUnderTest: "cc_binary", + moduleTypeUnderTestFactory: cc.BinaryFactory, + blueprint: bp, + filesystem: map[string]string{}, + expectedBazelTargets: []string{`cc_binary( + name = "alphabet_binary", + local_includes = ["."], + srcs = ["main.cc"], + target_compatible_with = ["//build/bazel/product_variables:alphabet_module__special_build"] + select({ + "//build/bazel/platforms/os_arch:android_x86_64": ["@platforms//:incompatible"], + "//build/bazel/platforms/os_arch:darwin_arm64": ["@platforms//:incompatible"], + "//build/bazel/platforms/os_arch:darwin_x86_64": ["@platforms//:incompatible"], + "//build/bazel/platforms/os_arch:linux_bionic_x86_64": ["@platforms//:incompatible"], + "//build/bazel/platforms/os_arch:linux_glibc_x86_64": ["@platforms//:incompatible"], + "//build/bazel/platforms/os_arch:linux_musl_x86_64": ["@platforms//:incompatible"], + "//build/bazel/platforms/os_arch:windows_x86_64": ["@platforms//:incompatible"], + "//conditions:default": [], + }), +)`}}) +} + +func TestSoongConfigModuleType_ProductVariableConfigOverridesEnable(t *testing.T) { + bp := ` +soong_config_bool_variable { + name: "special_build", +} + +soong_config_module_type { + name: "alphabet_cc_defaults", + module_type: "cc_defaults", + config_namespace: "alphabet_module", + bool_variables: ["special_build"], + properties: ["enabled"], +} + +alphabet_cc_defaults { + name: "alphabet_sample_cc_defaults", + soong_config_variables: { + special_build: { + enabled: true, + }, + }, +} + +cc_binary { + name: "alphabet_binary", + srcs: ["main.cc"], + defaults: ["alphabet_sample_cc_defaults"], + enabled: false, +}` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for library_linking_strategy", + moduleTypeUnderTest: "cc_binary", + moduleTypeUnderTestFactory: cc.BinaryFactory, + blueprint: bp, + filesystem: map[string]string{}, + expectedBazelTargets: []string{`cc_binary( + name = "alphabet_binary", + local_includes = ["."], + srcs = ["main.cc"], + target_compatible_with = ["//build/bazel/product_variables:alphabet_module__special_build"], +)`}}) +} + +func TestSoongConfigModuleType_ProductVariableIgnoredIfEnabledByDefault(t *testing.T) { + bp := ` +soong_config_bool_variable { + name: "special_build", +} + +soong_config_module_type { + name: "alphabet_cc_defaults", + module_type: "cc_defaults", + config_namespace: "alphabet_module", + bool_variables: ["special_build"], + properties: ["enabled"], +} + +alphabet_cc_defaults { + name: "alphabet_sample_cc_defaults", + soong_config_variables: { + special_build: { + enabled: true, + }, + }, +} + +cc_binary { + name: "alphabet_binary", + srcs: ["main.cc"], + defaults: ["alphabet_sample_cc_defaults"], +}` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for library_linking_strategy", + moduleTypeUnderTest: "cc_binary", + moduleTypeUnderTestFactory: cc.BinaryFactory, + blueprint: bp, + filesystem: map[string]string{}, + expectedBazelTargets: []string{`cc_binary( + name = "alphabet_binary", + local_includes = ["."], + srcs = ["main.cc"], +)`}}) +}