From 25825ca08df9c3dab7657ce553c33f95198cff36 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Mon, 15 Nov 2021 12:28:43 +0000 Subject: [PATCH] Refactor ProductConfigProperties to use a struct key instead of an string key with hardcoded patterns. This fixes a bug with label list conditions_default attrs where the attribute values get clobbered in a map with the keys "conditions_default" (with a default empty list) and "acme__feature__conditions_default" (with a non-empty list) when generating the LabelListAttribute. Test: CI Change-Id: I5429e40f747b7a0ed559f8a468a4831cd32df2c0 --- android/bazel.go | 4 + android/variable.go | 72 ++++++++++------- ...oong_config_module_type_conversion_test.go | 77 ++++++++++++++++++- cc/bp2build.go | 42 +++++----- 4 files changed, 146 insertions(+), 49 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index 26d70e48f..8864db010 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -32,6 +32,10 @@ type properties struct { Bazel_module bazel.BazelModuleProperties } +// namespacedVariableProperties is a map from a string representing a Soong +// config variable namespace, like "android" or "vendor_name" to a struct +// pointer representing the soong_config_variables property of a module created +// by a soong_config_module_type or soong_config_module_type_import. type namespacedVariableProperties map[string]interface{} // BazelModuleBase contains the property structs with metadata for modules which can be converted to diff --git a/android/variable.go b/android/variable.go index 6ad58c3f2..89cd59ef2 100644 --- a/android/variable.go +++ b/android/variable.go @@ -491,11 +491,11 @@ type ProductConfigContext interface { type ProductConfigProperty struct { // The name of the product variable, e.g. "safestack", "malloc_not_svelte", // "board" - ProductConfigVariable string + Name string // Namespace of the variable, if this is a soong_config_module_type variable - // e.g. "acme", "ANDROID", "vendor_nae" - Namespace string // for soong config variables + // e.g. "acme", "ANDROID", "vendor_name" + Namespace string // Unique configuration to identify this product config property (i.e. a // primary key), as just using the product variable name is not sufficient. @@ -562,9 +562,6 @@ type ProductConfigProperty struct { // "acme__board__soc_a", "acme__board__soc_b", and // "acme__board__conditions_default" FullConfig string - - // The actual property value: list, bool, string.. - Property interface{} } func (p *ProductConfigProperty) ConfigurationAxis() bazel.ConfigurationAxis { @@ -573,14 +570,43 @@ func (p *ProductConfigProperty) ConfigurationAxis() bazel.ConfigurationAxis { } else { // Soong config variables can be uniquely identified by the namespace // (e.g. acme, android) and the product variable name (e.g. board, size) - return bazel.ProductVariableConfigurationAxis(p.Namespace + "__" + p.ProductConfigVariable) + return bazel.ProductVariableConfigurationAxis(p.Namespace + "__" + p.Name) } } -// ProductConfigProperties is a map of property name to a slice of -// ProductConfigProperty such that all product variable-specific versions of a -// property are easily accessed together -type ProductConfigProperties map[string]map[string]ProductConfigProperty +// SelectKey returns the literal string that represents this variable in a BUILD +// select statement. +func (p *ProductConfigProperty) SelectKey() string { + if p.Namespace == "" { + return strings.ToLower(p.FullConfig) + } + + if p.FullConfig == bazel.ConditionsDefaultConfigKey { + return bazel.ConditionsDefaultConfigKey + } + + value := p.FullConfig + if value == p.Name { + value = "enabled" + } + // e.g. acme__feature1__enabled, android__board__soc_a + return strings.ToLower(strings.Join([]string{p.Namespace, p.Name, value}, "__")) +} + +// ProductConfigProperties is a map of maps to group property values according +// their property name and the product config variable they're set under. +// +// The outer map key is the name of the property, like "cflags". +// +// The inner map key is a ProductConfigProperty, which is a struct of product +// variable name, namespace, and the "full configuration" of the product +// variable. +// +// e.g. product variable name: board, namespace: acme, full config: vendor_chip_foo +// +// The value of the map is the interface{} representing the value of the +// property, like ["-DDEFINES"] for cflags. +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. @@ -630,19 +656,16 @@ func ProductVariableProperties(ctx BazelConversionPathContext) ProductConfigProp func (p *ProductConfigProperties) AddProductConfigProperty( propertyName, namespace, productVariableName, config string, property interface{}) { if (*p)[propertyName] == nil { - (*p)[propertyName] = make(map[string]ProductConfigProperty) + (*p)[propertyName] = make(map[ProductConfigProperty]interface{}) } - // Normalize config to be all lowercase. It's the "primary key" of this - // unique property value. This can be the conditions_default value of the - // product variable as well. - config = strings.ToLower(config) - (*p)[propertyName][config] = ProductConfigProperty{ - Namespace: namespace, // e.g. acme, android - ProductConfigVariable: productVariableName, // e.g. size, feature1, feature2, FEATURE3, board - FullConfig: config, // e.g. size, feature1-x86, size__conditions_default - Property: property, // e.g. ["-O3"] + productConfigProp := ProductConfigProperty{ + Namespace: namespace, // e.g. acme, android + Name: productVariableName, // e.g. size, feature1, feature2, FEATURE3, board + FullConfig: config, // e.g. size, feature1-x86, size__conditions_default } + + (*p)[propertyName][productConfigProp] = property } var ( @@ -783,8 +806,8 @@ func productVariableValues( if field.Field(k).IsZero() { continue } - productVariableValue := proptools.PropertyNameForField(propertyName) - config := strings.Join([]string{namespace, productVariableName, productVariableValue}, "__") + // config can also be "conditions_default". + config := proptools.PropertyNameForField(propertyName) actualPropertyName := field.Type().Field(k).Name productConfigProperties.AddProductConfigProperty( @@ -799,9 +822,6 @@ func productVariableValues( // Not a conditions_default or a struct prop, i.e. regular // product variables, or not a string-typed config var. config := productVariableName + suffix - if namespace != "" { - config = namespace + "__" + config - } productConfigProperties.AddProductConfigProperty( propertyName, namespace, diff --git a/bp2build/soong_config_module_type_conversion_test.go b/bp2build/soong_config_module_type_conversion_test.go index 41184794f..2e6f54c8e 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": ["-DFEATURE1"], + "//build/bazel/product_variables:acme__feature1__enabled": ["-DFEATURE1"], "//conditions:default": ["-DDEFAULT1"], }), local_includes = ["."], @@ -118,7 +118,7 @@ custom_cc_library_static { expectedBazelTargets: []string{`cc_library_static( name = "foo", copts = select({ - "//build/bazel/product_variables:acme__feature1": ["-DFEATURE1"], + "//build/bazel/product_variables:acme__feature1__enabled": ["-DFEATURE1"], "//conditions:default": ["-DDEFAULT1"], }), local_includes = ["."], @@ -246,12 +246,81 @@ 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": ["-DFEATURE1"], + "//build/bazel/product_variables:acme__feature1__enabled": ["-DFEATURE1"], "//conditions:default": ["-DDEFAULT1"], }) + select({ - "//build/bazel/product_variables:acme__feature2": ["-DFEATURE2"], + "//build/bazel/product_variables:acme__feature2__enabled": ["-DFEATURE2"], "//conditions:default": ["-DDEFAULT2"], }), local_includes = ["."], )`}}) } + +func TestSoongConfigModuleType_StringVar_LabelListDeps(t *testing.T) { + bp := ` +soong_config_string_variable { + name: "board", + values: ["soc_a", "soc_b", "soc_c"], +} + +soong_config_module_type { + name: "custom_cc_library_static", + module_type: "cc_library_static", + config_namespace: "acme", + variables: ["board"], + properties: ["cflags", "static_libs"], + bazel_module: { bp2build_available: true }, +} + +custom_cc_library_static { + name: "foo", + bazel_module: { bp2build_available: true }, + soong_config_variables: { + board: { + soc_a: { + cflags: ["-DSOC_A"], + static_libs: ["soc_a_dep"], + }, + soc_b: { + cflags: ["-DSOC_B"], + static_libs: ["soc_b_dep"], + }, + soc_c: {}, + conditions_default: { + cflags: ["-DSOC_DEFAULT"], + static_libs: ["soc_default_static_dep"], + }, + }, + }, +}` + + otherDeps := ` +cc_library_static { name: "soc_a_dep", bazel_module: { bp2build_available: false } } +cc_library_static { name: "soc_b_dep", bazel_module: { bp2build_available: false } } +cc_library_static { name: "soc_default_static_dep", bazel_module: { bp2build_available: false } } +` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for label list attributes", + moduleTypeUnderTest: "cc_library_static", + moduleTypeUnderTestFactory: cc.LibraryStaticFactory, + moduleTypeUnderTestBp2BuildMutator: cc.CcLibraryStaticBp2Build, + blueprint: bp, + filesystem: map[string]string{ + "foo/bar/Android.bp": otherDeps, + }, + expectedBazelTargets: []string{`cc_library_static( + name = "foo", + copts = select({ + "//build/bazel/product_variables:acme__board__soc_a": ["-DSOC_A"], + "//build/bazel/product_variables:acme__board__soc_b": ["-DSOC_B"], + "//conditions:default": ["-DSOC_DEFAULT"], + }), + implementation_deps = select({ + "//build/bazel/product_variables:acme__board__soc_a": ["//foo/bar:soc_a_dep"], + "//build/bazel/product_variables:acme__board__soc_b": ["//foo/bar:soc_b_dep"], + "//conditions:default": ["//foo/bar:soc_default_static_dep"], + }), + local_includes = ["."], +)`}}) +} diff --git a/cc/bp2build.go b/cc/bp2build.go index 2059f5e53..dde8dd4c7 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -320,14 +320,14 @@ func (ca *compilerAttributes) convertProductVariables(ctx android.BazelConversio "CppFlags": &ca.cppFlags, } for propName, attr := range productVarPropNameToAttribute { - if props, exists := productVariableProps[propName]; exists { - for _, prop := range props { - flags, ok := prop.Property.([]string) + if productConfigProps, exists := productVariableProps[propName]; exists { + for productConfigProp, prop := range productConfigProps { + flags, ok := prop.([]string) if !ok { ctx.ModuleErrorf("Could not convert product variable %s property", proptools.PropertyNameForField(propName)) } - newFlags, _ := bazel.TryVariableSubstitutions(flags, prop.ProductConfigVariable) - attr.SetSelectValue(prop.ConfigurationAxis(), prop.FullConfig, newFlags) + newFlags, _ := bazel.TryVariableSubstitutions(flags, productConfigProp.Name) + attr.SetSelectValue(productConfigProp.ConfigurationAxis(), productConfigProp.SelectKey(), newFlags) } } } @@ -587,31 +587,35 @@ func (la *linkerAttributes) convertProductVariables(ctx android.BazelConversionP if !exists && !excludesExists { continue } - // collect all the configurations that an include or exclude property exists for. - // we want to iterate all configurations rather than either the include or exclude because for a - // particular configuration we may have only and include or only an exclude to handle - configs := make(map[string]bool, len(props)+len(excludeProps)) - for config := range props { - configs[config] = true + // Collect all the configurations that an include or exclude property exists for. + // We want to iterate all configurations rather than either the include or exclude because, for a + // particular configuration, we may have either only an include or an exclude to handle. + productConfigProps := make(map[android.ProductConfigProperty]bool, len(props)+len(excludeProps)) + for p := range props { + productConfigProps[p] = true } - for config := range excludeProps { - configs[config] = true + for p := range excludeProps { + productConfigProps[p] = true } - for config := range configs { - prop, includesExists := props[config] - excludesProp, excludesExists := excludeProps[config] + for productConfigProp := range productConfigProps { + prop, includesExists := props[productConfigProp] + excludesProp, excludesExists := excludeProps[productConfigProp] var includes, excludes []string var ok bool // if there was no includes/excludes property, casting fails and that's expected - if includes, ok = prop.Property.([]string); includesExists && !ok { + if includes, ok = prop.([]string); includesExists && !ok { ctx.ModuleErrorf("Could not convert product variable %s property", name) } - if excludes, ok = excludesProp.Property.([]string); excludesExists && !ok { + if excludes, ok = excludesProp.([]string); excludesExists && !ok { ctx.ModuleErrorf("Could not convert product variable %s property", dep.excludesField) } - dep.attribute.SetSelectValue(prop.ConfigurationAxis(), config, dep.depResolutionFunc(ctx, android.FirstUniqueStrings(includes), excludes)) + dep.attribute.SetSelectValue( + productConfigProp.ConfigurationAxis(), + productConfigProp.SelectKey(), + dep.depResolutionFunc(ctx, android.FirstUniqueStrings(includes), excludes), + ) } } }