From 58ff6801f4a7daff95a4db6bfacf00578dc730b5 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Wed, 17 Nov 2021 12:14:41 +0000 Subject: [PATCH] Fix bp2build select generation for inter-attribute soong config variable usage. There's bug a in the current soong_config_variable handling implementation where a soong_config_variable sets conditions_default value for an attr, and a non-conditions_default value for another attr. This results in the former attr not properly setting the zero value for the non-conditions_default select key, resulting in the pretty printer omitting the attribute totally. The current implementation in this CL ensures that the zero value is set whenever this happens at the module level. This is seen in library_linking_strategy_cc_defaults (see comments in code, and the new tests) Test: CI Bug: 198556411 Change-Id: Ibaeb94508c51a7429fb7a08df610cbb5470f76d2 --- android/variable.go | 127 ++++++++- bazel/properties.go | 6 + bp2build/build_conversion.go | 12 +- bp2build/configurability.go | 24 +- ...oong_config_module_type_conversion_test.go | 269 ++++++++++++++++++ cc/bp2build.go | 1 + 6 files changed, 418 insertions(+), 21 deletions(-) diff --git a/android/variable.go b/android/variable.go index 20b268eab..fe828d3e3 100644 --- a/android/variable.go +++ b/android/variable.go @@ -564,6 +564,10 @@ type ProductConfigProperty struct { FullConfig string } +func (p *ProductConfigProperty) AlwaysEmit() bool { + return p.Namespace != "" +} + func (p *ProductConfigProperty) ConfigurationAxis() bazel.ConfigurationAxis { if p.Namespace == "" { return bazel.ProductVariableConfigurationAxis(p.FullConfig) @@ -652,9 +656,123 @@ func ProductVariableProperties(ctx BazelConversionPathContext) ProductConfigProp } } + productConfigProperties.zeroValuesForNamespacedVariables() + return productConfigProperties } +// zeroValuesForNamespacedVariables ensures that selects that contain __only__ +// conditions default values have zero values set for the other non-default +// values for that select statement. +// +// If the ProductConfigProperties map contains these items, as parsed from the .bp file: +// +// library_linking_strategy: { +// prefer_static: { +// static_libs: [ +// "lib_a", +// "lib_b", +// ], +// }, +// conditions_default: { +// shared_libs: [ +// "lib_a", +// "lib_b", +// ], +// }, +// }, +// +// Static_libs {Library_linking_strategy ANDROID prefer_static} [lib_a lib_b] +// Shared_libs {Library_linking_strategy ANDROID conditions_default} [lib_a lib_b] +// +// We need to add this: +// +// Shared_libs {Library_linking_strategy ANDROID prefer_static} [] +// +// so that the following gets generated for the "dynamic_deps" attribute, +// instead of putting lib_a and lib_b directly into dynamic_deps without a +// select: +// +// dynamic_deps = select({ +// "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": [], +// "//conditions:default": [ +// "//foo/bar:lib_a", +// "//foo/bar:lib_b", +// ], +// }), +func (props *ProductConfigProperties) zeroValuesForNamespacedVariables() { + // A map of product config properties to the zero values of their respective + // property value. + zeroValues := make(map[ProductConfigProperty]interface{}) + + // A map of prop names (e.g. cflags) to product config properties where the + // (prop name, ProductConfigProperty) tuple contains a non-conditions_default key. + // + // e.g. + // + // prefer_static: { + // static_libs: [ + // "lib_a", + // "lib_b", + // ], + // }, + // conditions_default: { + // shared_libs: [ + // "lib_a", + // "lib_b", + // ], + // }, + // + // The tuple of ("static_libs", prefer_static) would be in this map. + hasNonDefaultValue := make(map[string]map[ProductConfigProperty]bool) + + // Iterate over all added soong config variables. + for propName, v := range *props { + for p, intf := range v { + if p.Namespace == "" { + // If there's no namespace, this isn't a soong config variable, + // i.e. this is a product variable. product variables have no + // conditions_defaults, so skip them. + continue + } + if p.FullConfig == bazel.ConditionsDefaultConfigKey { + // Skip conditions_defaults. + continue + } + if hasNonDefaultValue[propName] == nil { + hasNonDefaultValue[propName] = make(map[ProductConfigProperty]bool) + hasNonDefaultValue[propName][p] = false + } + // Create the zero value of the variable. + if _, exists := zeroValues[p]; !exists { + zeroValue := reflect.Zero(reflect.ValueOf(intf).Type()).Interface() + if zeroValue == nil { + panic(fmt.Errorf("Expected non-nil zero value for product/config variable %+v\n", intf)) + } + zeroValues[p] = zeroValue + } + hasNonDefaultValue[propName][p] = true + } + } + + for propName := range *props { + for p, zeroValue := range zeroValues { + // Ignore variables that already have a non-default value for that axis + if exists, _ := hasNonDefaultValue[propName][p]; !exists { + // fmt.Println(propName, p.Namespace, p.Name, p.FullConfig, zeroValue) + // Insert the zero value for this propname + product config value. + props.AddProductConfigProperty( + propName, + p.Namespace, + p.Name, + p.FullConfig, + zeroValue, + ) + } + } + } +} + func (p *ProductConfigProperties) AddProductConfigProperty( propertyName, namespace, productVariableName, config string, property interface{}) { if (*p)[propertyName] == nil { @@ -675,7 +793,7 @@ func (p *ProductConfigProperties) AddProductConfigProperty( (*p)[propertyName][productConfigProp] = dst } default: - // TODO(jingwen): Add support for more types. + panic(fmt.Errorf("TODO: handle merging value %s", existing)) } } else { (*p)[propertyName][productConfigProp] = property @@ -823,9 +941,10 @@ func (productConfigProperties *ProductConfigProperties) AddProductConfigProperti field.Field(k).Interface(), // e.g. ["-DDEFAULT"], ["foo", "bar"] ) } - } else { - // Not a conditions_default or a struct prop, i.e. regular - // product variables, or not a string-typed config var. + } else if property.Kind() != reflect.Interface { + // If not an interface, then this is not a conditions_default or + // a struct prop. That is, this is a regular product variable, + // or a bool/value config variable. config := productVariableName + suffix productConfigProperties.AddProductConfigProperty( propertyName, diff --git a/bazel/properties.go b/bazel/properties.go index a438481be..76e73c588 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -417,6 +417,12 @@ type LabelListAttribute struct { // This mode facilitates use of attribute defaults: an empty list should // override the default. ForceSpecifyEmptyList bool + + // If true, signal the intent to the code generator to emit all select keys, + // even if the Includes list for that key is empty. This mode facilitates + // specific select statements where an empty list for a non-default select + // key has a meaning. + EmitEmptyList bool } type configurableLabelLists map[ConfigurationAxis]labelListSelectValues diff --git a/bp2build/build_conversion.go b/bp2build/build_conversion.go index aa1cf7068..10ee58294 100644 --- a/bp2build/build_conversion.go +++ b/bp2build/build_conversion.go @@ -532,8 +532,8 @@ func isStructPtr(t reflect.Type) bool { // prettyPrint a property value into the equivalent Starlark representation // recursively. -func prettyPrint(propertyValue reflect.Value, indent int) (string, error) { - if isZero(propertyValue) { +func prettyPrint(propertyValue reflect.Value, indent int, emitZeroValues bool) (string, error) { + if !emitZeroValues && isZero(propertyValue) { // A property value being set or unset actually matters -- Soong does set default // values for unset properties, like system_shared_libs = ["libc", "libm", "libdl"] at // https://cs.android.com/android/platform/superproject/+/master:build/soong/cc/linker.go;l=281-287;drc=f70926eef0b9b57faf04c17a1062ce50d209e480 @@ -556,7 +556,7 @@ func prettyPrint(propertyValue reflect.Value, indent int) (string, error) { case reflect.Int, reflect.Uint, reflect.Int64: ret = fmt.Sprintf("%v", propertyValue.Interface()) case reflect.Ptr: - return prettyPrint(propertyValue.Elem(), indent) + return prettyPrint(propertyValue.Elem(), indent, emitZeroValues) case reflect.Slice: if propertyValue.Len() == 0 { return "[]", nil @@ -565,7 +565,7 @@ func prettyPrint(propertyValue reflect.Value, indent int) (string, error) { if propertyValue.Len() == 1 { // Single-line list for list with only 1 element ret += "[" - indexedValue, err := prettyPrint(propertyValue.Index(0), indent) + indexedValue, err := prettyPrint(propertyValue.Index(0), indent, emitZeroValues) if err != nil { return "", err } @@ -575,7 +575,7 @@ func prettyPrint(propertyValue reflect.Value, indent int) (string, error) { // otherwise, use a multiline list. ret += "[\n" for i := 0; i < propertyValue.Len(); i++ { - indexedValue, err := prettyPrint(propertyValue.Index(i), indent+1) + indexedValue, err := prettyPrint(propertyValue.Index(i), indent+1, emitZeroValues) if err != nil { return "", err } @@ -660,7 +660,7 @@ func extractStructProperties(structValue reflect.Value, indent int) map[string]s } propertyName := proptools.PropertyNameForField(field.Name) - prettyPrintedValue, err := prettyPrint(fieldValue, indent+1) + prettyPrintedValue, err := prettyPrint(fieldValue, indent+1, false) if err != nil { panic( fmt.Errorf( diff --git a/bp2build/configurability.go b/bp2build/configurability.go index 0bcf91d58..c953259f9 100644 --- a/bp2build/configurability.go +++ b/bp2build/configurability.go @@ -95,7 +95,7 @@ func getLabelListValues(list bazel.LabelListAttribute) (reflect.Value, []selects continue } selectKey := axis.SelectKey(config) - if use, value := labelListSelectValue(selectKey, labels); use { + if use, value := labelListSelectValue(selectKey, labels, list.EmitEmptyList); use { archSelects[selectKey] = value } } @@ -107,8 +107,8 @@ func getLabelListValues(list bazel.LabelListAttribute) (reflect.Value, []selects return value, ret } -func labelListSelectValue(selectKey string, list bazel.LabelList) (bool, reflect.Value) { - if selectKey == bazel.ConditionsDefaultSelectKey || len(list.Includes) > 0 { +func labelListSelectValue(selectKey string, list bazel.LabelList, emitEmptyList bool) (bool, reflect.Value) { + if selectKey == bazel.ConditionsDefaultSelectKey || emitEmptyList || len(list.Includes) > 0 { return true, reflect.ValueOf(list.Includes) } else if len(list.Excludes) > 0 { // if there is still an excludes -- we need to have an empty list for this select & use the @@ -129,6 +129,7 @@ func prettyPrintAttribute(v bazel.Attribute, indent int) (string, error) { var value reflect.Value var configurableAttrs []selects var defaultSelectValue *string + var emitZeroValues bool // If true, print the default attribute value, even if the attribute is zero. shouldPrintDefault := false switch list := v.(type) { @@ -137,6 +138,7 @@ func prettyPrintAttribute(v bazel.Attribute, indent int) (string, error) { defaultSelectValue = &emptyBazelList case bazel.LabelListAttribute: value, configurableAttrs = getLabelListValues(list) + emitZeroValues = list.EmitEmptyList defaultSelectValue = &emptyBazelList if list.ForceSpecifyEmptyList && (!value.IsNil() || list.HasConfigurableValues()) { shouldPrintDefault = true @@ -154,7 +156,7 @@ func prettyPrintAttribute(v bazel.Attribute, indent int) (string, error) { var err error ret := "" if value.Kind() != reflect.Invalid { - s, err := prettyPrint(value, indent) + s, err := prettyPrint(value, indent, false) // never emit zero values for the base value if err != nil { return ret, err } @@ -163,7 +165,7 @@ func prettyPrintAttribute(v bazel.Attribute, indent int) (string, error) { } // Convenience function to append selects components to an attribute value. appendSelects := func(selectsData selects, defaultValue *string, s string) (string, error) { - selectMap, err := prettyPrintSelectMap(selectsData, defaultValue, indent) + selectMap, err := prettyPrintSelectMap(selectsData, defaultValue, indent, emitZeroValues) if err != nil { return "", err } @@ -190,7 +192,7 @@ func prettyPrintAttribute(v bazel.Attribute, indent int) (string, error) { // prettyPrintSelectMap converts a map of select keys to reflected Values as a generic way // to construct a select map for any kind of attribute type. -func prettyPrintSelectMap(selectMap map[string]reflect.Value, defaultValue *string, indent int) (string, error) { +func prettyPrintSelectMap(selectMap map[string]reflect.Value, defaultValue *string, indent int, emitZeroValues bool) (string, error) { if selectMap == nil { return "", nil } @@ -202,11 +204,11 @@ func prettyPrintSelectMap(selectMap map[string]reflect.Value, defaultValue *stri continue } value := selectMap[selectKey] - if isZero(value) { + if isZero(value) && !emitZeroValues { // Ignore zero values to not generate empty lists. continue } - s, err := prettyPrintSelectEntry(value, selectKey, indent) + s, err := prettyPrintSelectEntry(value, selectKey, indent, emitZeroValues) if err != nil { return "", err } @@ -227,7 +229,7 @@ func prettyPrintSelectMap(selectMap map[string]reflect.Value, defaultValue *stri ret += selects // Handle the default condition - s, err := prettyPrintSelectEntry(selectMap[bazel.ConditionsDefaultSelectKey], bazel.ConditionsDefaultSelectKey, indent) + s, err := prettyPrintSelectEntry(selectMap[bazel.ConditionsDefaultSelectKey], bazel.ConditionsDefaultSelectKey, indent, emitZeroValues) if err != nil { return "", err } @@ -249,9 +251,9 @@ func prettyPrintSelectMap(selectMap map[string]reflect.Value, defaultValue *stri // prettyPrintSelectEntry converts a reflect.Value into an entry in a select map // with a provided key. -func prettyPrintSelectEntry(value reflect.Value, key string, indent int) (string, error) { +func prettyPrintSelectEntry(value reflect.Value, key string, indent int, emitZeroValues bool) (string, error) { s := makeIndent(indent + 1) - v, err := prettyPrint(value, indent+1) + v, err := prettyPrint(value, indent+1, emitZeroValues) if err != nil { return "", err } diff --git a/bp2build/soong_config_module_type_conversion_test.go b/bp2build/soong_config_module_type_conversion_test.go index 5669a8383..523f75031 100644 --- a/bp2build/soong_config_module_type_conversion_test.go +++ b/bp2build/soong_config_module_type_conversion_test.go @@ -32,6 +32,8 @@ func registerSoongConfigModuleTypes(ctx android.RegistrationContext) { ctx.RegisterModuleType("soong_config_module_type", android.SoongConfigModuleTypeFactory) ctx.RegisterModuleType("soong_config_string_variable", android.SoongConfigStringVariableDummyFactory) ctx.RegisterModuleType("soong_config_bool_variable", android.SoongConfigBoolVariableDummyFactory) + + ctx.RegisterModuleType("cc_library", cc.LibraryFactory) } func TestSoongConfigModuleType(t *testing.T) { @@ -590,3 +592,270 @@ cc_library_static { local_includes = ["."], )`}}) } + +func TestSoongConfigModuleType_Defaults(t *testing.T) { + bp := ` +soong_config_string_variable { + name: "library_linking_strategy", + values: [ + "prefer_static", + ], +} + +soong_config_module_type { + name: "library_linking_strategy_cc_defaults", + module_type: "cc_defaults", + config_namespace: "ANDROID", + variables: ["library_linking_strategy"], + properties: [ + "shared_libs", + "static_libs", + ], + bazel_module: { bp2build_available: true }, +} + +library_linking_strategy_cc_defaults { + name: "library_linking_strategy_lib_a_defaults", + soong_config_variables: { + library_linking_strategy: { + prefer_static: { + static_libs: [ + "lib_a", + ], + }, + conditions_default: { + shared_libs: [ + "lib_a", + ], + }, + }, + }, +} + +library_linking_strategy_cc_defaults { + name: "library_linking_strategy_merged_defaults", + defaults: ["library_linking_strategy_lib_a_defaults"], + soong_config_variables: { + library_linking_strategy: { + prefer_static: { + static_libs: [ + "lib_b", + ], + }, + conditions_default: { + shared_libs: [ + "lib_b", + ], + }, + }, + }, +} + +cc_binary { + name: "library_linking_strategy_sample_binary", + srcs: ["library_linking_strategy.cc"], + defaults: ["library_linking_strategy_merged_defaults"], +}` + + otherDeps := ` +cc_library { name: "lib_a", bazel_module: { bp2build_available: false } } +cc_library { name: "lib_b", bazel_module: { bp2build_available: false } } +cc_library { name: "lib_default", bazel_module: { bp2build_available: false } } +` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for library_linking_strategy", + moduleTypeUnderTest: "cc_binary", + moduleTypeUnderTestFactory: cc.BinaryFactory, + moduleTypeUnderTestBp2BuildMutator: cc.BinaryBp2build, + blueprint: bp, + filesystem: map[string]string{ + "foo/bar/Android.bp": otherDeps, + }, + expectedBazelTargets: []string{`cc_binary( + name = "library_linking_strategy_sample_binary", + deps = select({ + "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": [ + "//foo/bar:lib_b_bp2build_cc_library_static", + "//foo/bar:lib_a_bp2build_cc_library_static", + ], + "//conditions:default": [], + }), + dynamic_deps = select({ + "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": [], + "//conditions:default": [ + "//foo/bar:lib_b", + "//foo/bar:lib_a", + ], + }), + local_includes = ["."], + srcs = ["library_linking_strategy.cc"], +)`}}) +} + +func TestSoongConfigModuleType_Defaults_Another(t *testing.T) { + bp := ` +soong_config_string_variable { + name: "library_linking_strategy", + values: [ + "prefer_static", + ], +} + +soong_config_module_type { + name: "library_linking_strategy_cc_defaults", + module_type: "cc_defaults", + config_namespace: "ANDROID", + variables: ["library_linking_strategy"], + properties: [ + "shared_libs", + "static_libs", + ], + bazel_module: { bp2build_available: true }, +} + +library_linking_strategy_cc_defaults { + name: "library_linking_strategy_sample_defaults", + soong_config_variables: { + library_linking_strategy: { + prefer_static: { + static_libs: [ + "lib_a", + "lib_b", + ], + }, + conditions_default: { + shared_libs: [ + "lib_a", + "lib_b", + ], + }, + }, + }, +} + +cc_binary { + name: "library_linking_strategy_sample_binary", + srcs: ["library_linking_strategy.cc"], + defaults: ["library_linking_strategy_sample_defaults"], +}` + + otherDeps := ` +cc_library { name: "lib_a", bazel_module: { bp2build_available: false } } +cc_library { name: "lib_b", bazel_module: { bp2build_available: false } } +` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for library_linking_strategy", + moduleTypeUnderTest: "cc_binary", + moduleTypeUnderTestFactory: cc.BinaryFactory, + moduleTypeUnderTestBp2BuildMutator: cc.BinaryBp2build, + blueprint: bp, + filesystem: map[string]string{ + "foo/bar/Android.bp": otherDeps, + }, + expectedBazelTargets: []string{`cc_binary( + name = "library_linking_strategy_sample_binary", + deps = select({ + "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": [ + "//foo/bar:lib_a_bp2build_cc_library_static", + "//foo/bar:lib_b_bp2build_cc_library_static", + ], + "//conditions:default": [], + }), + dynamic_deps = select({ + "//build/bazel/product_variables:android__library_linking_strategy__prefer_static": [], + "//conditions:default": [ + "//foo/bar:lib_a", + "//foo/bar:lib_b", + ], + }), + local_includes = ["."], + srcs = ["library_linking_strategy.cc"], +)`}}) +} + +func TestSoongConfigModuleType_Defaults_UnusedProps(t *testing.T) { + bp := ` +soong_config_string_variable { + name: "alphabet", + values: [ + "a", + "b", + "c", // unused + ], +} + +soong_config_module_type { + name: "alphabet_cc_defaults", + module_type: "cc_defaults", + config_namespace: "ANDROID", + variables: ["alphabet"], + properties: [ + "cflags", // unused + "shared_libs", + "static_libs", + ], + bazel_module: { bp2build_available: true }, +} + +alphabet_cc_defaults { + name: "alphabet_sample_cc_defaults", + soong_config_variables: { + alphabet: { + a: { + shared_libs: [ + "lib_a", + ], + }, + b: { + shared_libs: [ + "lib_b", + ], + }, + conditions_default: { + static_libs: [ + "lib_default", + ], + }, + }, + }, +} + +cc_binary { + name: "alphabet_binary", + srcs: ["main.cc"], + defaults: ["alphabet_sample_cc_defaults"], +}` + + otherDeps := ` +cc_library { name: "lib_a", bazel_module: { bp2build_available: false } } +cc_library { name: "lib_b", bazel_module: { bp2build_available: false } } +cc_library { name: "lib_default", bazel_module: { bp2build_available: false } } +` + + runSoongConfigModuleTypeTest(t, bp2buildTestCase{ + description: "soong config variables - generates selects for library_linking_strategy", + moduleTypeUnderTest: "cc_binary", + moduleTypeUnderTestFactory: cc.BinaryFactory, + moduleTypeUnderTestBp2BuildMutator: cc.BinaryBp2build, + blueprint: bp, + filesystem: map[string]string{ + "foo/bar/Android.bp": otherDeps, + }, + expectedBazelTargets: []string{`cc_binary( + name = "alphabet_binary", + deps = select({ + "//build/bazel/product_variables:android__alphabet__a": [], + "//build/bazel/product_variables:android__alphabet__b": [], + "//conditions:default": ["//foo/bar:lib_default_bp2build_cc_library_static"], + }), + dynamic_deps = select({ + "//build/bazel/product_variables:android__alphabet__a": ["//foo/bar:lib_a"], + "//build/bazel/product_variables:android__alphabet__b": ["//foo/bar:lib_b"], + "//conditions:default": [], + }), + local_includes = ["."], + srcs = ["main.cc"], +)`}}) +} diff --git a/cc/bp2build.go b/cc/bp2build.go index dde8dd4c7..d9494362f 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -611,6 +611,7 @@ func (la *linkerAttributes) convertProductVariables(ctx android.BazelConversionP ctx.ModuleErrorf("Could not convert product variable %s property", dep.excludesField) } + dep.attribute.EmitEmptyList = productConfigProp.AlwaysEmit() dep.attribute.SetSelectValue( productConfigProp.ConfigurationAxis(), productConfigProp.SelectKey(),