Migrate overlapping config var defs in all bp file
Unlike most module types, config variable handling is always namespaced
to the Android.bp file, which limits reuse of the variable definitions.
Additionally multiple of these module types can define a string variable
in the same config namespace, but specify different valid values for the
string.
Previously, we cached the first instance we see of variable + namespace;
however, this caused non-determinism in which defintion would be used
and not migrating all values. Instead, we now only cache within a single
Android.bp file where the variable definitions are re-used.
Test: go tests
Bug: 271481817
Change-Id: Ic327657c508e47a705bacd24712a1916e105c7cd
diff --git a/android/soong_config_modules.go b/android/soong_config_modules.go
index 9f5440d..5fa6012 100644
--- a/android/soong_config_modules.go
+++ b/android/soong_config_modules.go
@@ -396,7 +396,7 @@
}
if ctx.Config().BuildMode == Bp2build {
- ctx.Config().Bp2buildSoongConfigDefinitions.AddVars(*mtDef)
+ ctx.Config().Bp2buildSoongConfigDefinitions.AddVars(mtDef)
}
globalModuleTypes := ctx.moduleFactories()
diff --git a/android/soongconfig/modules.go b/android/soongconfig/modules.go
index 9239ca9..23c8afa 100644
--- a/android/soongconfig/modules.go
+++ b/android/soongconfig/modules.go
@@ -240,12 +240,7 @@
// string vars, bool vars and value vars created by every
// soong_config_module_type in this build.
type Bp2BuildSoongConfigDefinitions struct {
- // varCache contains a cache of string variables namespace + property
- // The same variable may be used in multiple module types (for example, if need support
- // for cc_default and java_default), only need to process once
- varCache map[string]bool
-
- StringVars map[string][]string
+ StringVars map[string]map[string]bool
BoolVars map[string]bool
ValueVars map[string]bool
}
@@ -255,7 +250,7 @@
// SoongConfigVariablesForBp2build extracts information from a
// SoongConfigDefinition that bp2build needs to generate constraint settings and
// values for, in order to migrate soong_config_module_type usages to Bazel.
-func (defs *Bp2BuildSoongConfigDefinitions) AddVars(mtDef SoongConfigDefinition) {
+func (defs *Bp2BuildSoongConfigDefinitions) AddVars(mtDef *SoongConfigDefinition) {
// In bp2build mode, this method is called concurrently in goroutines from
// loadhooks while parsing soong_config_module_type, so add a mutex to
// prevent concurrent map writes. See b/207572723
@@ -263,7 +258,7 @@
defer bp2buildSoongConfigVarsLock.Unlock()
if defs.StringVars == nil {
- defs.StringVars = make(map[string][]string)
+ defs.StringVars = make(map[string]map[string]bool)
}
if defs.BoolVars == nil {
defs.BoolVars = make(map[string]bool)
@@ -271,24 +266,29 @@
if defs.ValueVars == nil {
defs.ValueVars = make(map[string]bool)
}
- if defs.varCache == nil {
- defs.varCache = make(map[string]bool)
- }
+ // varCache contains a cache of string variables namespace + property
+ // The same variable may be used in multiple module types (for example, if need support
+ // for cc_default and java_default), only need to process once
+ varCache := map[string]bool{}
+
for _, moduleType := range mtDef.ModuleTypes {
for _, v := range moduleType.Variables {
key := strings.Join([]string{moduleType.ConfigNamespace, v.variableProperty()}, "__")
// The same variable may be used in multiple module types (for example, if need support
// for cc_default and java_default), only need to process once
- if _, keyInCache := defs.varCache[key]; keyInCache {
+ if _, keyInCache := varCache[key]; keyInCache {
continue
} else {
- defs.varCache[key] = true
+ varCache[key] = true
}
if strVar, ok := v.(*stringVariable); ok {
+ if _, ok := defs.StringVars[key]; !ok {
+ defs.StringVars[key] = make(map[string]bool, len(strVar.values))
+ }
for _, value := range strVar.values {
- defs.StringVars[key] = append(defs.StringVars[key], value)
+ defs.StringVars[key][value] = true
}
} else if _, ok := v.(*boolVariable); ok {
defs.BoolVars[key] = true
@@ -301,6 +301,23 @@
}
}
+// This is a copy of the one available in soong/android/util.go, but depending
+// on the android package causes a cyclic dependency. A refactoring here is to
+// extract common utils out from android/utils.go for other packages like this.
+func sortedStringKeys(m interface{}) []string {
+ v := reflect.ValueOf(m)
+ if v.Kind() != reflect.Map {
+ panic(fmt.Sprintf("%#v is not a map", m))
+ }
+ keys := v.MapKeys()
+ s := make([]string, 0, len(keys))
+ for _, key := range keys {
+ s = append(s, key.String())
+ }
+ sort.Strings(s)
+ return s
+}
+
// String emits the Soong config variable definitions as Starlark dictionaries.
func (defs Bp2BuildSoongConfigDefinitions) String() string {
ret := ""
@@ -312,8 +329,13 @@
ret += starlark_fmt.PrintBoolDict(defs.ValueVars, 0)
ret += "\n\n"
+ stringVars := make(map[string][]string, len(defs.StringVars))
+ for k, v := range defs.StringVars {
+ stringVars[k] = sortedStringKeys(v)
+ }
+
ret += "soong_config_string_variables = "
- ret += starlark_fmt.PrintStringListDict(defs.StringVars, 0)
+ ret += starlark_fmt.PrintStringListDict(stringVars, 0)
return ret
}
diff --git a/android/soongconfig/modules_test.go b/android/soongconfig/modules_test.go
index d5d87ef..a5fa349 100644
--- a/android/soongconfig/modules_test.go
+++ b/android/soongconfig/modules_test.go
@@ -414,6 +414,110 @@
}
}
+func Test_Bp2BuildSoongConfigDefinitionsAddVars(t *testing.T) {
+ testCases := []struct {
+ desc string
+ defs []*SoongConfigDefinition
+ expected Bp2BuildSoongConfigDefinitions
+ }{
+ {
+ desc: "non-overlapping",
+ defs: []*SoongConfigDefinition{
+ &SoongConfigDefinition{
+ ModuleTypes: map[string]*ModuleType{
+ "a": &ModuleType{
+ ConfigNamespace: "foo",
+ Variables: []soongConfigVariable{
+ &stringVariable{
+ baseVariable: baseVariable{"string_var"},
+ values: []string{"a", "b", "c"},
+ },
+ },
+ },
+ },
+ },
+ &SoongConfigDefinition{
+ ModuleTypes: map[string]*ModuleType{
+ "b": &ModuleType{
+ ConfigNamespace: "foo",
+ Variables: []soongConfigVariable{
+ &stringVariable{
+ baseVariable: baseVariable{"string_var"},
+ values: []string{"a", "b", "c"},
+ },
+ &boolVariable{baseVariable: baseVariable{"bool_var"}},
+ &valueVariable{baseVariable: baseVariable{"variable_var"}},
+ },
+ },
+ },
+ },
+ },
+ expected: Bp2BuildSoongConfigDefinitions{
+ StringVars: map[string]map[string]bool{
+ "foo__string_var": map[string]bool{"a": true, "b": true, "c": true},
+ },
+ BoolVars: map[string]bool{"foo__bool_var": true},
+ ValueVars: map[string]bool{"foo__variable_var": true},
+ },
+ },
+ {
+ desc: "overlapping",
+ defs: []*SoongConfigDefinition{
+ &SoongConfigDefinition{
+ ModuleTypes: map[string]*ModuleType{
+ "a": &ModuleType{
+ ConfigNamespace: "foo",
+ Variables: []soongConfigVariable{
+ &stringVariable{
+ baseVariable: baseVariable{"string_var"},
+ values: []string{"a", "b", "c"},
+ },
+ },
+ },
+ },
+ },
+ &SoongConfigDefinition{
+ ModuleTypes: map[string]*ModuleType{
+ "b": &ModuleType{
+ ConfigNamespace: "foo",
+ Variables: []soongConfigVariable{
+ &stringVariable{
+ baseVariable: baseVariable{"string_var"},
+ values: []string{"b", "c", "d"},
+ },
+ &boolVariable{baseVariable: baseVariable{"bool_var"}},
+ &valueVariable{baseVariable: baseVariable{"variable_var"}},
+ },
+ },
+ },
+ },
+ },
+ expected: Bp2BuildSoongConfigDefinitions{
+ StringVars: map[string]map[string]bool{
+ "foo__string_var": map[string]bool{"a": true, "b": true, "c": true, "d": true},
+ },
+ BoolVars: map[string]bool{"foo__bool_var": true},
+ ValueVars: map[string]bool{"foo__variable_var": true},
+ },
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.desc, func(t *testing.T) {
+ actual := &Bp2BuildSoongConfigDefinitions{}
+ for _, d := range tc.defs {
+ func(def *SoongConfigDefinition) {
+ actual.AddVars(def)
+ }(d)
+ }
+ if !reflect.DeepEqual(*actual, tc.expected) {
+ t.Errorf("Expected %#v, got %#v", tc.expected, *actual)
+ }
+ })
+ }
+
+}
+
func Test_Bp2BuildSoongConfigDefinitions(t *testing.T) {
testCases := []struct {
desc string
@@ -456,11 +560,11 @@
soong_config_string_variables = {}`}, {
desc: "only string vars",
defs: Bp2BuildSoongConfigDefinitions{
- StringVars: map[string][]string{
- "string_var": []string{
- "choice1",
- "choice2",
- "choice3",
+ StringVars: map[string]map[string]bool{
+ "string_var": map[string]bool{
+ "choice1": true,
+ "choice2": true,
+ "choice3": true,
},
},
},
@@ -484,15 +588,15 @@
"value_var_one": true,
"value_var_two": true,
},
- StringVars: map[string][]string{
- "string_var_one": []string{
- "choice1",
- "choice2",
- "choice3",
+ StringVars: map[string]map[string]bool{
+ "string_var_one": map[string]bool{
+ "choice1": true,
+ "choice2": true,
+ "choice3": true,
},
- "string_var_two": []string{
- "foo",
- "bar",
+ "string_var_two": map[string]bool{
+ "foo": true,
+ "bar": true,
},
},
},
@@ -512,8 +616,8 @@
"choice3",
],
"string_var_two": [
- "foo",
"bar",
+ "foo",
],
}`},
}