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

android/soong_config_modules.go

@@ -396,7 +396,7 @@ func loadSoongConfigModuleTypeDefinition(ctx LoadHookContext, from string) map[s
 		}
 
 		if ctx.Config().BuildMode == Bp2build {
-			ctx.Config().Bp2buildSoongConfigDefinitions.AddVars(*mtDef)
+			ctx.Config().Bp2buildSoongConfigDefinitions.AddVars(mtDef)
 		}
 
 		globalModuleTypes := ctx.moduleFactories()

android/soongconfig/modules.go

@@ -240,12 +240,7 @@ type SoongConfigDefinition struct {
 // 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
+	StringVars map[string]map[string]bool
-	// 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
 	BoolVars   map[string]bool
 	ValueVars  map[string]bool
 }
@@ -255,7 +250,7 @@ var bp2buildSoongConfigVarsLock sync.Mutex
 // 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 @@ func (defs *Bp2BuildSoongConfigDefinitions) AddVars(mtDef SoongConfigDefinition)
 	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 @@ func (defs *Bp2BuildSoongConfigDefinitions) AddVars(mtDef SoongConfigDefinition)
 	if defs.ValueVars == nil {
 		defs.ValueVars = make(map[string]bool)
 	}
-	if defs.varCache == nil {
+	// varCache contains a cache of string variables namespace + property
-		defs.varCache = make(map[string]bool)
+	// 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 @@ func (defs *Bp2BuildSoongConfigDefinitions) AddVars(mtDef SoongConfigDefinition)
 	}
 }
 
+// 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 @@ func (defs Bp2BuildSoongConfigDefinitions) String() string {
 	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
 }

android/soongconfig/modules_test.go

@@ -414,6 +414,110 @@ func Test_PropertiesToApply_String_Error(t *testing.T) {
 	}
 }
 
+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_value_variables = {
 soong_config_string_variables = {}`}, {
 			desc: "only string vars",
 			defs: Bp2BuildSoongConfigDefinitions{
-				StringVars: map[string][]string{
+				StringVars: map[string]map[string]bool{
-					"string_var": []string{
+					"string_var": map[string]bool{
-						"choice1",
+						"choice1": true,
-						"choice2",
+						"choice2": true,
-						"choice3",
+						"choice3": true,
 					},
 				},
 			},
@@ -484,15 +588,15 @@ soong_config_string_variables = {
 					"value_var_one": true,
 					"value_var_two": true,
 				},
-				StringVars: map[string][]string{
+				StringVars: map[string]map[string]bool{
-					"string_var_one": []string{
+					"string_var_one": map[string]bool{
-						"choice1",
+						"choice1": true,
-						"choice2",
+						"choice2": true,
-						"choice3",
+						"choice3": true,
 					},
-					"string_var_two": []string{
+					"string_var_two": map[string]bool{
-						"foo",
+						"foo": true,
-						"bar",
+						"bar": true,
 					},
 				},
 			},
@@ -512,8 +616,8 @@ soong_config_string_variables = {
         "choice3",
     ],
     "string_var_two": [
-        "foo",
         "bar",
+        "foo",
     ],
 }`},
 	}