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
diff --git a/bp2build/build_conversion.go b/bp2build/build_conversion.go
index aa1cf70..10ee582 100644
--- a/bp2build/build_conversion.go
+++ b/bp2build/build_conversion.go
@@ -532,8 +532,8 @@
// 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 @@
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 @@
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 @@
// 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 @@
}
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 0bcf91d..c953259 100644
--- a/bp2build/configurability.go
+++ b/bp2build/configurability.go
@@ -95,7 +95,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
// 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 @@
// 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 @@
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 @@
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 @@
// 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 5669a83..523f750 100644
--- a/bp2build/soong_config_module_type_conversion_test.go
+++ b/bp2build/soong_config_module_type_conversion_test.go
@@ -32,6 +32,8 @@
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 @@
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"],
+)`}})
+}