Fix product variables in defaults modules

Product variables structs are generated at runtime to contain only
the properties that apply to the current module.  Defaults modules
always contained all product variable properties. Defaults modules
apply their properties to the target module using
proptools.PrependProperties, which prepends structs that have
matching types.  Filtered property structs had a different type
and were dropped.

Even after adding filtering to the defaults product variable
properties, defaults modules may contain more property structs
than the target module they are applied to, so the product
variables struct for the defaults module could contain more
fields than the product variables struct for the target module.
Use proptools.PrependMatchingProperties when applying defaults
of product variables instead, which will apply matching properties
across types.

Test: defaults_test.go
Test: variable_test.go
Change-Id: I281bdefef92053457a3b7b65383493a4e7d999df

android/defaults.go

@@ -15,6 +15,8 @@
 package android
 
 import (
+	"reflect"
+
 	"github.com/google/blueprint"
 	"github.com/google/blueprint/proptools"
 )
@@ -30,16 +32,18 @@ type defaultsProperties struct {
 }
 
 type DefaultableModuleBase struct {
-	defaultsProperties    defaultsProperties
+	defaultsProperties            defaultsProperties
-	defaultableProperties []interface{}
+	defaultableProperties         []interface{}
+	defaultableVariableProperties interface{}
 }
 
 func (d *DefaultableModuleBase) defaults() *defaultsProperties {
 	return &d.defaultsProperties
 }
 
-func (d *DefaultableModuleBase) setProperties(props []interface{}) {
+func (d *DefaultableModuleBase) setProperties(props []interface{}, variableProperties interface{}) {
 	d.defaultableProperties = props
+	d.defaultableVariableProperties = variableProperties
 }
 
 // Interface that must be supported by any module to which defaults can be applied.
@@ -48,7 +52,7 @@ type Defaultable interface {
 	defaults() *defaultsProperties
 
 	// Set the property structures into which defaults will be added.
-	setProperties([]interface{})
+	setProperties(props []interface{}, variableProperties interface{})
 
 	// Apply defaults from the supplied Defaults to the property structures supplied to
 	// setProperties(...).
@@ -63,7 +67,10 @@ type DefaultableModule interface {
 var _ Defaultable = (*DefaultableModuleBase)(nil)
 
 func InitDefaultableModule(module DefaultableModule) {
-	module.setProperties(module.(Module).GetProperties())
+	if module.(Module).base().module == nil {
+		panic("InitAndroidModule must be called before InitDefaultableModule")
+	}
+	module.setProperties(module.(Module).GetProperties(), module.(Module).base().variableProperties)
 
 	module.AddProperties(module.defaults())
 }
@@ -114,6 +121,8 @@ type Defaults interface {
 	// Get the structures containing the properties for which defaults can be provided.
 	properties() []interface{}
 
+	productVariableProperties() interface{}
+
 	// Return the defaults common properties.
 	common() *commonProperties
 
@@ -134,6 +143,10 @@ func (d *DefaultsModuleBase) properties() []interface{} {
 	return d.defaultableProperties
 }
 
+func (d *DefaultsModuleBase) productVariableProperties() interface{} {
+	return d.defaultableVariableProperties
+}
+
 func (d *DefaultsModuleBase) common() *commonProperties {
 	return &d.commonProperties
 }
@@ -151,9 +164,10 @@ func InitDefaultsModule(module DefaultsModule) {
 	module.AddProperties(
 		&hostAndDeviceProperties{},
 		commonProperties,
-		&variableProperties{},
 		&ApexProperties{})
 
+	initAndroidModuleBase(module)
+	initProductVariableModule(module)
 	InitArchModule(module)
 	InitDefaultableModule(module)
 
@@ -185,16 +199,57 @@ func (defaultable *DefaultableModuleBase) applyDefaults(ctx TopDownMutatorContex
 
 	for _, defaults := range defaultsList {
 		for _, prop := range defaultable.defaultableProperties {
-			for _, def := range defaults.properties() {
+			if prop == defaultable.defaultableVariableProperties {
-				if proptools.TypeEqual(prop, def) {
+				defaultable.applyDefaultVariableProperties(ctx, defaults, prop)
-					err := proptools.PrependProperties(prop, def, nil)
+			} else {
-					if err != nil {
+				defaultable.applyDefaultProperties(ctx, defaults, prop)
-						if propertyErr, ok := err.(*proptools.ExtendPropertyError); ok {
+			}
-							ctx.PropertyErrorf(propertyErr.Property, "%s", propertyErr.Err.Error())
+		}
-						} else {
+	}
-							panic(err)
+}
-						}
+
-					}
+// Product variable properties need special handling, the type of the filtered product variable
+// property struct may not be identical between the defaults module and the defaultable module.
+// Use PrependMatchingProperties to apply whichever properties match.
+func (defaultable *DefaultableModuleBase) applyDefaultVariableProperties(ctx TopDownMutatorContext,
+	defaults Defaults, defaultableProp interface{}) {
+	if defaultableProp == nil {
+		return
+	}
+
+	defaultsProp := defaults.productVariableProperties()
+	if defaultsProp == nil {
+		return
+	}
+
+	dst := []interface{}{
+		defaultableProp,
+		// Put an empty copy of the src properties into dst so that properties in src that are not in dst
+		// don't cause a "failed to find property to extend" error.
+		proptools.CloneEmptyProperties(reflect.ValueOf(defaultsProp)).Interface(),
+	}
+
+	err := proptools.PrependMatchingProperties(dst, defaultsProp, nil)
+	if err != nil {
+		if propertyErr, ok := err.(*proptools.ExtendPropertyError); ok {
+			ctx.PropertyErrorf(propertyErr.Property, "%s", propertyErr.Err.Error())
+		} else {
+			panic(err)
+		}
+	}
+}
+
+func (defaultable *DefaultableModuleBase) applyDefaultProperties(ctx TopDownMutatorContext,
+	defaults Defaults, defaultableProp interface{}) {
+
+	for _, def := range defaults.properties() {
+		if proptools.TypeEqual(defaultableProp, def) {
+			err := proptools.PrependProperties(defaultableProp, def, nil)
+			if err != nil {
+				if propertyErr, ok := err.(*proptools.ExtendPropertyError); ok {
+					ctx.PropertyErrorf(propertyErr.Property, "%s", propertyErr.Err.Error())
+				} else {
+					panic(err)
 				}
 			}
 		}

android/defaults_test.go

@@ -15,6 +15,7 @@
 package android
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/google/blueprint/proptools"
@@ -40,8 +41,8 @@ func (d *defaultsTestModule) GenerateAndroidBuildActions(ctx ModuleContext) {
 func defaultsTestModuleFactory() Module {
 	module := &defaultsTestModule{}
 	module.AddProperties(&module.properties)
-	InitDefaultableModule(module)
 	InitAndroidModule(module)
+	InitDefaultableModule(module)
 	return module
 }
 
@@ -57,6 +58,49 @@ func defaultsTestDefaultsFactory() Module {
 	return defaults
 }
 
+func TestDefaults(t *testing.T) {
+	bp := `
+		defaults {
+			name: "transitive",
+			foo: ["transitive"],
+		}
+
+		defaults {
+			name: "defaults",
+			defaults: ["transitive"],
+			foo: ["defaults"],
+		}
+
+		test {
+			name: "foo",
+			defaults: ["defaults"],
+			foo: ["module"],
+		}
+	`
+
+	config := TestConfig(buildDir, nil, bp, nil)
+
+	ctx := NewTestContext()
+
+	ctx.RegisterModuleType("test", defaultsTestModuleFactory)
+	ctx.RegisterModuleType("defaults", defaultsTestDefaultsFactory)
+
+	ctx.PreArchMutators(RegisterDefaultsPreArchMutators)
+
+	ctx.Register(config)
+
+	_, errs := ctx.ParseFileList(".", []string{"Android.bp"})
+	FailIfErrored(t, errs)
+	_, errs = ctx.PrepareBuildActions(config)
+	FailIfErrored(t, errs)
+
+	foo := ctx.ModuleForTests("foo", "").Module().(*defaultsTestModule)
+
+	if g, w := foo.properties.Foo, []string{"transitive", "defaults", "module"}; !reflect.DeepEqual(g, w) {
+		t.Errorf("expected foo %q, got %q", w, g)
+	}
+}
+
 func TestDefaultsAllowMissingDependencies(t *testing.T) {
 	bp := `
 		defaults {

android/module.go

@@ -537,16 +537,7 @@ func InitAndroidModule(m Module) {
 		&base.nameProperties,
 		&base.commonProperties)
 
-	// Allow tests to override the default product variables
+	initProductVariableModule(m)
-	if base.variableProperties == nil {
-		base.variableProperties = defaultProductVariables
-	}
-
-	// Filter the product variables properties to the ones that exist on this module
-	base.variableProperties = createVariableProperties(m.GetProperties(), base.variableProperties)
-	if base.variableProperties != nil {
-		m.AddProperties(base.variableProperties)
-	}
 
 	base.generalProperties = m.GetProperties()
 	base.customizableProperties = m.GetProperties()

android/variable.go

@@ -25,7 +25,7 @@ import (
 
 func init() {
 	PreDepsMutators(func(ctx RegisterMutatorsContext) {
-		ctx.BottomUp("variable", variableMutator).Parallel()
+		ctx.BottomUp("variable", VariableMutator).Parallel()
 	})
 }
 
@@ -384,7 +384,7 @@ func (v *productVariables) SetDefaultConfig() {
 	}
 }
 
-func variableMutator(mctx BottomUpMutatorContext) {
+func VariableMutator(mctx BottomUpMutatorContext) {
 	var module Module
 	var ok bool
 	if module, ok = mctx.Module().(Module); !ok {
@@ -539,6 +539,20 @@ func sliceToTypeArray(s []interface{}) interface{} {
 	return ret.Interface()
 }
 
+func initProductVariableModule(m Module) {
+	base := m.base()
+
+	// Allow tests to override the default product variables
+	if base.variableProperties == nil {
+		base.variableProperties = defaultProductVariables
+	}
+	// Filter the product variables properties to the ones that exist on this module
+	base.variableProperties = createVariableProperties(m.GetProperties(), base.variableProperties)
+	if base.variableProperties != nil {
+		m.AddProperties(base.variableProperties)
+	}
+}
+
 // createVariableProperties takes the list of property structs for a module and returns a property struct that
 // contains the product variable properties that exist in the property structs, or nil if there are none.  It
 // caches the result.

android/variable_test.go

@@ -171,7 +171,7 @@ func TestProductVariables(t *testing.T) {
 		Foo []string
 	}{}))
 	ctx.PreDepsMutators(func(ctx RegisterMutatorsContext) {
-		ctx.BottomUp("variable", variableMutator).Parallel()
+		ctx.BottomUp("variable", VariableMutator).Parallel()
 	})
 
 	// Test that a module can use one product variable even if it doesn't have all the properties
@@ -209,6 +209,115 @@ func TestProductVariables(t *testing.T) {
 	FailIfErrored(t, errs)
 }
 
+var testProductVariableDefaultsProperties = struct {
+	Product_variables struct {
+		Eng struct {
+			Foo []string
+			Bar []string
+		}
+	}
+}{}
+
+type productVariablesDefaultsTestProperties struct {
+	Foo []string
+}
+
+type productVariablesDefaultsTestProperties2 struct {
+	Foo []string
+	Bar []string
+}
+
+type productVariablesDefaultsTestModule struct {
+	ModuleBase
+	DefaultableModuleBase
+	properties productVariablesDefaultsTestProperties
+}
+
+func (d *productVariablesDefaultsTestModule) GenerateAndroidBuildActions(ctx ModuleContext) {
+	ctx.Build(pctx, BuildParams{
+		Rule:   Touch,
+		Output: PathForModuleOut(ctx, "out"),
+	})
+}
+
+func productVariablesDefaultsTestModuleFactory() Module {
+	module := &productVariablesDefaultsTestModule{}
+	module.AddProperties(&module.properties)
+	module.variableProperties = testProductVariableDefaultsProperties
+	InitAndroidModule(module)
+	InitDefaultableModule(module)
+	return module
+}
+
+type productVariablesDefaultsTestDefaults struct {
+	ModuleBase
+	DefaultsModuleBase
+}
+
+func productVariablesDefaultsTestDefaultsFactory() Module {
+	defaults := &productVariablesDefaultsTestDefaults{}
+	defaults.AddProperties(&productVariablesDefaultsTestProperties{})
+	defaults.AddProperties(&productVariablesDefaultsTestProperties2{})
+	defaults.variableProperties = testProductVariableDefaultsProperties
+	InitDefaultsModule(defaults)
+	return defaults
+}
+
+// Test a defaults module that supports more product variable properties than the target module.
+func TestProductVariablesDefaults(t *testing.T) {
+	bp := `
+		defaults {
+			name: "defaults",
+			product_variables: {
+				eng: {
+					foo: ["product_variable_defaults"],
+					bar: ["product_variable_defaults"],
+				},
+			},
+			foo: ["defaults"],
+			bar: ["defaults"],
+		}
+
+		test {
+			name: "foo",
+			defaults: ["defaults"],
+			foo: ["module"],
+			product_variables: {
+				eng: {
+					foo: ["product_variable_module"],
+				},
+			},
+		}
+	`
+
+	config := TestConfig(buildDir, nil, bp, nil)
+	config.TestProductVariables.Eng = boolPtr(true)
+
+	ctx := NewTestContext()
+
+	ctx.RegisterModuleType("test", productVariablesDefaultsTestModuleFactory)
+	ctx.RegisterModuleType("defaults", productVariablesDefaultsTestDefaultsFactory)
+
+	ctx.PreArchMutators(RegisterDefaultsPreArchMutators)
+	ctx.PreDepsMutators(func(ctx RegisterMutatorsContext) {
+		ctx.BottomUp("variable", VariableMutator).Parallel()
+	})
+
+	ctx.Register(config)
+
+	_, errs := ctx.ParseFileList(".", []string{"Android.bp"})
+	FailIfErrored(t, errs)
+	_, errs = ctx.PrepareBuildActions(config)
+	FailIfErrored(t, errs)
+
+	foo := ctx.ModuleForTests("foo", "").Module().(*productVariablesDefaultsTestModule)
+
+	want := []string{"defaults", "module", "product_variable_defaults", "product_variable_module"}
+	if g, w := foo.properties.Foo, want; !reflect.DeepEqual(g, w) {
+		t.Errorf("expected foo %q, got %q", w, g)
+	}
+}
+
 func BenchmarkSliceToTypeArray(b *testing.B) {
 	for _, n := range []int{1, 2, 4, 8, 100} {
 		var propStructs []interface{}

cc/cc_test.go

@@ -2751,3 +2751,42 @@ func TestDefaults(t *testing.T) {
 		t.Errorf("libboth ar rule wanted %q, got %q", w, g)
 	}
 }
+
+func TestProductVariableDefaults(t *testing.T) {
+	bp := `
+		cc_defaults {
+			name: "libfoo_defaults",
+			srcs: ["foo.c"],
+			cppflags: ["-DFOO"],
+			product_variables: {
+				debuggable: {
+					cppflags: ["-DBAR"],
+				},
+			},
+		}
+
+		cc_library {
+			name: "libfoo",
+			defaults: ["libfoo_defaults"],
+		}
+	`
+
+	config := TestConfig(buildDir, android.Android, nil, bp, nil)
+	config.TestProductVariables.Debuggable = BoolPtr(true)
+
+	ctx := CreateTestContext()
+	ctx.PreDepsMutators(func(ctx android.RegisterMutatorsContext) {
+		ctx.BottomUp("variable", android.VariableMutator).Parallel()
+	})
+	ctx.Register(config)
+
+	_, errs := ctx.ParseFileList(".", []string{"Android.bp"})
+	android.FailIfErrored(t, errs)
+	_, errs = ctx.PrepareBuildActions(config)
+	android.FailIfErrored(t, errs)
+
+	libfoo := ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_static").Module().(*Module)
+	if !android.InList("-DBAR", libfoo.flags.Local.CppFlags) {
+		t.Errorf("expected -DBAR in cppflags, got %q", libfoo.flags.Local.CppFlags)
+	}
+}