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

@@ -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 {
+			} else if property.Kind() != reflect.Interface {
-				// Not a conditions_default or a struct prop, i.e. regular
+				// If not an interface, then this is not a conditions_default or
-				// product variables, or not a string-typed config var.
+				// a struct prop. That is, this is a regular product variable,
+				// or a bool/value config variable.
 				config := productVariableName + suffix
 				productConfigProperties.AddProductConfigProperty(
 					propertyName,

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

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) {
+func prettyPrint(propertyValue reflect.Value, indent int, emitZeroValues bool) (string, error) {
-	if isZero(propertyValue) {
+	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(

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) {
+func labelListSelectValue(selectKey string, list bazel.LabelList, emitEmptyList bool) (bool, reflect.Value) {
-	if selectKey == bazel.ConditionsDefaultSelectKey || len(list.Includes) > 0 {
+	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
 	}

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"],
+)`}})
+}

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(),