Merge "Migrate overlapping config var defs in all bp file"

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