From 18c46807c934f8e68b00e364cc75a5d346327521 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 24 Sep 2019 22:19:02 -0700 Subject: [PATCH] Filter product variable property structs For each module, take the default product variable properties and create a new type that contains only the properties that exist in the current module. This will provide better errors when attemping to set product variable properties that will never get used for a module, and fixes errors when a module attempts to use a product variable that also contains properties that don't apply to the current module. Fixes: 79249983 Test: TestProductVariables Test: m checkbuild Change-Id: I4dc24f7781b8ce798651cfc8bf3563005a92c13d --- android/module.go | 17 ++++-- android/mutator.go | 19 ++++++- android/variable.go | 114 +++++++++++++++++++++++++++++++++++++-- android/variable_test.go | 111 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 254 insertions(+), 7 deletions(-) diff --git a/android/module.go b/android/module.go index 8076a99ff..bb0aada9f 100644 --- a/android/module.go +++ b/android/module.go @@ -510,8 +510,19 @@ func InitAndroidModule(m Module) { m.AddProperties( &base.nameProperties, - &base.commonProperties, - &base.variableProperties) + &base.commonProperties) + + // Allow tests to override the default product variables + if base.variableProperties == nil { + base.variableProperties = zeroProductVariables + } + + // 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() @@ -593,7 +604,7 @@ type ModuleBase struct { nameProperties nameProperties commonProperties commonProperties - variableProperties variableProperties + variableProperties interface{} hostAndDeviceProperties hostAndDeviceProperties generalProperties []interface{} archProperties [][]interface{} diff --git a/android/mutator.go b/android/mutator.go index 510e63c0c..88ac52117 100644 --- a/android/mutator.go +++ b/android/mutator.go @@ -15,6 +15,8 @@ package android import ( + "reflect" + "github.com/google/blueprint" "github.com/google/blueprint/proptools" ) @@ -244,8 +246,23 @@ func (t *topDownMutatorContext) Rename(name string) { } func (t *topDownMutatorContext) CreateModule(factory ModuleFactory, props ...interface{}) Module { - inherited := []interface{}{&t.Module().base().commonProperties, &t.Module().base().variableProperties} + inherited := []interface{}{&t.Module().base().commonProperties} module := t.bp.CreateModule(ModuleFactoryAdaptor(factory), append(inherited, props...)...).(Module) + + if t.Module().base().variableProperties != nil && module.base().variableProperties != nil { + src := t.Module().base().variableProperties + dst := []interface{}{ + module.base().variableProperties, + // 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(src).Elem()).Interface(), + } + err := proptools.AppendMatchingProperties(dst, src, nil) + if err != nil { + panic(err) + } + } + return module } diff --git a/android/variable.go b/android/variable.go index 3f2b9e9ff..41943b0f9 100644 --- a/android/variable.go +++ b/android/variable.go @@ -122,7 +122,7 @@ type variableProperties struct { } `android:"arch_variant"` } -var zeroProductVariables variableProperties +var zeroProductVariables interface{} = variableProperties{} type productVariables struct { // Suffix to add to generated Makefiles @@ -366,8 +366,13 @@ func variableMutator(mctx BottomUpMutatorContext) { // TODO: depend on config variable, create variants, propagate variants up tree a := module.base() - variableValues := reflect.ValueOf(&a.variableProperties.Product_variables).Elem() - zeroValues := reflect.ValueOf(zeroProductVariables.Product_variables) + + if a.variableProperties == nil { + return + } + + variableValues := reflect.ValueOf(a.variableProperties).Elem().FieldByName("Product_variables") + zeroValues := reflect.ValueOf(zeroProductVariables).FieldByName("Product_variables") for i := 0; i < variableValues.NumField(); i++ { variableValue := variableValues.Field(i) @@ -496,3 +501,106 @@ func printfIntoProperty(propertyValue reflect.Value, variableValue interface{}) return nil } + +var variablePropTypeMap OncePer + +// sliceToTypeArray takes a slice of property structs and returns a reflection created array containing the +// reflect.Types of each property struct. The result can be used as a key in a map. +func sliceToTypeArray(s []interface{}) interface{} { + // Create an array using reflection whose length is the length of the input slice + ret := reflect.New(reflect.ArrayOf(len(s), reflect.TypeOf(reflect.TypeOf(0)))).Elem() + for i, e := range s { + ret.Index(i).Set(reflect.ValueOf(reflect.TypeOf(e))) + } + return ret.Interface() +} + +// 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. +func createVariableProperties(moduleTypeProps []interface{}, productVariables interface{}) interface{} { + // Convert the moduleTypeProps to an array of reflect.Types that can be used as a key in the OncePer. + key := sliceToTypeArray(moduleTypeProps) + + // Use the variablePropTypeMap OncePer to cache the result for each set of property struct types. + typ, _ := variablePropTypeMap.Once(NewCustomOnceKey(key), func() interface{} { + // Compute the filtered property struct type. + return createVariablePropertiesType(moduleTypeProps, productVariables) + }).(reflect.Type) + + if typ == nil { + return nil + } + + // Create a new pointer to a filtered property struct. + return reflect.New(typ).Interface() +} + +// createVariablePropertiesType creates a new type that contains only the product variable properties that exist in +// a list of property structs. +func createVariablePropertiesType(moduleTypeProps []interface{}, productVariables interface{}) reflect.Type { + typ, _ := proptools.FilterPropertyStruct(reflect.TypeOf(productVariables), + func(field reflect.StructField, prefix string) (bool, reflect.StructField) { + // Filter function, returns true if the field should be in the resulting struct + if prefix == "" { + // Keep the top level Product_variables field + return true, field + } + _, rest := splitPrefix(prefix) + if rest == "" { + // Keep the 2nd level field (i.e. Product_variables.Eng) + return true, field + } + + // Strip off the first 2 levels of the prefix + _, prefix = splitPrefix(rest) + + for _, p := range moduleTypeProps { + if fieldExistsByNameRecursive(reflect.TypeOf(p).Elem(), prefix, field.Name) { + // Keep any fields that exist in one of the property structs + return true, field + } + } + + return false, field + }) + return typ +} + +func splitPrefix(prefix string) (first, rest string) { + index := strings.IndexByte(prefix, '.') + if index == -1 { + return prefix, "" + } + return prefix[:index], prefix[index+1:] +} + +func fieldExistsByNameRecursive(t reflect.Type, prefix, name string) bool { + if t.Kind() != reflect.Struct { + panic(fmt.Errorf("fieldExistsByNameRecursive can only be called on a reflect.Struct")) + } + + if prefix != "" { + split := strings.SplitN(prefix, ".", 2) + firstPrefix := split[0] + rest := "" + if len(split) > 1 { + rest = split[1] + } + f, exists := t.FieldByName(firstPrefix) + if !exists { + return false + } + ft := f.Type + if ft.Kind() == reflect.Ptr { + ft = ft.Elem() + } + if ft.Kind() != reflect.Struct { + panic(fmt.Errorf("field %q in %q is not a struct", firstPrefix, t)) + } + return fieldExistsByNameRecursive(ft, rest, name) + } else { + _, exists := t.FieldByName(name) + return exists + } +} diff --git a/android/variable_test.go b/android/variable_test.go index ce9ba5485..c1910fe03 100644 --- a/android/variable_test.go +++ b/android/variable_test.go @@ -16,7 +16,10 @@ package android import ( "reflect" + "strconv" "testing" + + "github.com/google/blueprint/proptools" ) type printfIntoPropertyTestCase struct { @@ -122,3 +125,111 @@ func TestPrintfIntoProperty(t *testing.T) { } } } + +type testProductVariableModule struct { + ModuleBase +} + +func (m *testProductVariableModule) GenerateAndroidBuildActions(ctx ModuleContext) { +} + +var testProductVariableProperties = struct { + Product_variables struct { + Eng struct { + Srcs []string + Cflags []string + } + } +}{} + +func testProductVariableModuleFactoryFactory(props interface{}) func() Module { + return func() Module { + m := &testProductVariableModule{} + clonedProps := proptools.CloneProperties(reflect.ValueOf(props)).Interface() + m.AddProperties(clonedProps) + + // Set a default variableProperties, this will be used as the input to the property struct filter + // for this test module. + m.variableProperties = testProductVariableProperties + InitAndroidModule(m) + return m + } +} + +func TestProductVariables(t *testing.T) { + ctx := NewTestContext() + // A module type that has a srcs property but not a cflags property. + ctx.RegisterModuleType("module1", ModuleFactoryAdaptor(testProductVariableModuleFactoryFactory(struct { + Srcs []string + }{}))) + // A module type that has a cflags property but not a srcs property. + ctx.RegisterModuleType("module2", ModuleFactoryAdaptor(testProductVariableModuleFactoryFactory(struct { + Cflags []string + }{}))) + // A module type that does not have any properties that match product_variables. + ctx.RegisterModuleType("module3", ModuleFactoryAdaptor(testProductVariableModuleFactoryFactory(struct { + Foo []string + }{}))) + ctx.PreDepsMutators(func(ctx RegisterMutatorsContext) { + ctx.BottomUp("variable", variableMutator).Parallel() + }) + + // Test that a module can use one product variable even if it doesn't have all the properties + // supported by that product variable. + bp := ` + module1 { + name: "foo", + product_variables: { + eng: { + srcs: ["foo.c"], + }, + }, + } + module2 { + name: "bar", + product_variables: { + eng: { + cflags: ["-DBAR"], + }, + }, + } + + module3 { + name: "baz", + } + ` + + mockFS := map[string][]byte{ + "Android.bp": []byte(bp), + } + + ctx.MockFileSystem(mockFS) + + ctx.Register() + + config := TestConfig(buildDir, nil) + config.TestProductVariables.Eng = proptools.BoolPtr(true) + + _, errs := ctx.ParseFileList(".", []string{"Android.bp"}) + FailIfErrored(t, errs) + _, errs = ctx.PrepareBuildActions(config) + FailIfErrored(t, errs) +} + +func BenchmarkSliceToTypeArray(b *testing.B) { + for _, n := range []int{1, 2, 4, 8, 100} { + var propStructs []interface{} + for i := 0; i < n; i++ { + propStructs = append(propStructs, &struct { + A *string + B string + }{}) + + } + b.Run(strconv.Itoa(n), func(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = sliceToTypeArray(propStructs) + } + }) + } +}