From eabaedd520f8b25573c831693d064e5916245746 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 6 Feb 2020 17:01:55 -0800 Subject: [PATCH] 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 | 87 ++++++++++++++++++++++++------ android/defaults_test.go | 46 +++++++++++++++- android/module.go | 11 +--- android/variable.go | 18 ++++++- android/variable_test.go | 111 ++++++++++++++++++++++++++++++++++++++- cc/cc_test.go | 39 ++++++++++++++ 6 files changed, 282 insertions(+), 30 deletions(-) diff --git a/android/defaults.go b/android/defaults.go index 759744693..fd707a45f 100644 --- a/android/defaults.go +++ b/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 - defaultableProperties []interface{} + defaultsProperties defaultsProperties + 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 proptools.TypeEqual(prop, def) { - err := proptools.PrependProperties(prop, def, nil) - if err != nil { - if propertyErr, ok := err.(*proptools.ExtendPropertyError); ok { - ctx.PropertyErrorf(propertyErr.Property, "%s", propertyErr.Err.Error()) - } else { - panic(err) - } - } + if prop == defaultable.defaultableVariableProperties { + defaultable.applyDefaultVariableProperties(ctx, defaults, prop) + } else { + defaultable.applyDefaultProperties(ctx, defaults, prop) + } + } + } +} + +// 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) } } } diff --git a/android/defaults_test.go b/android/defaults_test.go index ba607ef53..d096b2f92 100644 --- a/android/defaults_test.go +++ b/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 { diff --git a/android/module.go b/android/module.go index 15f3b5614..0a8737bee 100644 --- a/android/module.go +++ b/android/module.go @@ -537,16 +537,7 @@ func InitAndroidModule(m Module) { &base.nameProperties, &base.commonProperties) - // 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) - } + initProductVariableModule(m) base.generalProperties = m.GetProperties() base.customizableProperties = m.GetProperties() diff --git a/android/variable.go b/android/variable.go index 165db1cf2..de6621b0d 100644 --- a/android/variable.go +++ b/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. diff --git a/android/variable_test.go b/android/variable_test.go index cde2b1ac9..9cafedd50 100644 --- a/android/variable_test.go +++ b/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{} diff --git a/cc/cc_test.go b/cc/cc_test.go index 332cc45e7..c105954e3 100644 --- a/cc/cc_test.go +++ b/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) + } +}