From 191c25f589c8018fddb5c3c4e12888c25ae61365 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Thu, 10 Sep 2020 00:40:37 +0100 Subject: [PATCH] Handle property structs and BpPropertySets as values to AddProperty. Both will create a nested property set, that may be merged with an existing one. Test: m nothing Bug: 151303681 Change-Id: I30696ba3eb8960ca6fa54c9ee2cf6229ab9f5da9 --- android/sdk.go | 18 ++++++- sdk/bp.go | 82 ++++++++++++++++++++++++++++-- sdk/bp_test.go | 134 +++++++++++++++++++++++++++++++++++++++++++++++++ sdk/testing.go | 16 ++++++ 4 files changed, 246 insertions(+), 4 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index 9ea7ff49c..f2cdc880b 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -237,9 +237,25 @@ type BpPropertySet interface { // * string // * array of the above // * bool + // For these types it is an error if multiple properties with the same name + // are added. + // + // * pointer to a struct // * BpPropertySet // - // It is an error if multiple properties with the same name are added. + // A pointer to a Blueprint-style property struct is first converted into a + // BpPropertySet by traversing the fields and adding their values as + // properties in a BpPropertySet. A field with a struct value is itself + // converted into a BpPropertySet before adding. + // + // Adding a BpPropertySet is done as follows: + // * If no property with the name exists then the BpPropertySet is added + // directly to this property. Care must be taken to ensure that it does not + // introduce a cycle. + // * If a property exists with the name and the current value is a + // BpPropertySet then every property of the new BpPropertySet is added to + // the existing BpPropertySet. + // * Otherwise, if a property exists with the name then it is an error. AddProperty(name string, value interface{}) // Add a property with an associated tag diff --git a/sdk/bp.go b/sdk/bp.go index 68fe7ab4e..11ec8c6bd 100644 --- a/sdk/bp.go +++ b/sdk/bp.go @@ -16,6 +16,8 @@ package sdk import ( "fmt" + "reflect" + "strings" "android/soong/android" ) @@ -33,7 +35,82 @@ func (s *bpPropertySet) init() { s.tags = make(map[string]android.BpPropertyTag) } +// Converts the given value, which is assumed to be a struct, to a +// bpPropertySet. +func convertToPropertySet(value reflect.Value) *bpPropertySet { + res := newPropertySet() + structType := value.Type() + + for i := 0; i < structType.NumField(); i++ { + field := structType.Field(i) + fieldVal := value.Field(i) + + switch fieldVal.Type().Kind() { + case reflect.Ptr: + if fieldVal.IsNil() { + continue // nil pointer means the property isn't set. + } + fieldVal = fieldVal.Elem() + case reflect.Slice: + if fieldVal.IsNil() { + continue // Ignore a nil slice (but not one with length zero). + } + } + + if fieldVal.Type().Kind() == reflect.Struct { + fieldVal = fieldVal.Addr() // Avoid struct copy below. + } + res.AddProperty(strings.ToLower(field.Name), fieldVal.Interface()) + } + + return res +} + +// Converts the given value to something that can be set in a property. +func coercePropertyValue(value interface{}) interface{} { + val := reflect.ValueOf(value) + switch val.Kind() { + case reflect.Struct: + // convertToPropertySet requires an addressable struct, and this is probably + // a mistake. + panic(fmt.Sprintf("Value is a struct, not a pointer to one: %v", value)) + case reflect.Ptr: + if _, ok := value.(*bpPropertySet); !ok { + derefValue := reflect.Indirect(val) + if derefValue.Kind() != reflect.Struct { + panic(fmt.Sprintf("A pointer must be to a struct, got: %v", value)) + } + return convertToPropertySet(derefValue) + } + } + return value +} + +// Merges the fields of the given property set into s. +func (s *bpPropertySet) mergePropertySet(propSet *bpPropertySet) { + for _, name := range propSet.order { + if tag, ok := propSet.tags[name]; ok { + s.AddPropertyWithTag(name, propSet.properties[name], tag) + } else { + s.AddProperty(name, propSet.properties[name]) + } + } +} + func (s *bpPropertySet) AddProperty(name string, value interface{}) { + value = coercePropertyValue(value) + + if propSetValue, ok := value.(*bpPropertySet); ok { + if curValue, ok := s.properties[name]; ok { + if curSet, ok := curValue.(*bpPropertySet); ok { + curSet.mergePropertySet(propSetValue) + return + } + // If the current value isn't a property set we got conflicting types. + // Continue down to the check below to complain about it. + } + } + if s.properties[name] != nil { panic(fmt.Sprintf("Property %q already exists in property set", name)) } @@ -48,9 +125,8 @@ func (s *bpPropertySet) AddPropertyWithTag(name string, value interface{}, tag a } func (s *bpPropertySet) AddPropertySet(name string) android.BpPropertySet { - set := newPropertySet() - s.AddProperty(name, set) - return set + s.AddProperty(name, newPropertySet()) + return s.properties[name].(android.BpPropertySet) } func (s *bpPropertySet) getValue(name string) interface{} { diff --git a/sdk/bp_test.go b/sdk/bp_test.go index c630c2524..e1edc5131 100644 --- a/sdk/bp_test.go +++ b/sdk/bp_test.go @@ -18,8 +18,142 @@ import ( "testing" "android/soong/android" + + "github.com/google/blueprint/proptools" ) +func propertySetFixture() interface{} { + set := newPropertySet() + set.AddProperty("x", "taxi") + set.AddPropertyWithTag("y", 1729, "tag_y") + subset := set.AddPropertySet("sub") + subset.AddPropertyWithTag("x", "taxi", "tag_x") + subset.AddProperty("y", 1729) + return set +} + +func intPtr(i int) *int { return &i } + +type propertyStruct struct { + X *string + Y *int + Unset *bool + Sub struct { + X *string + Y *int + Unset *bool + } +} + +func propertyStructFixture() interface{} { + str := &propertyStruct{} + str.X = proptools.StringPtr("taxi") + str.Y = intPtr(1729) + str.Sub.X = proptools.StringPtr("taxi") + str.Sub.Y = intPtr(1729) + return str +} + +func checkPropertySetFixture(h *TestHelper, val interface{}, hasTags bool) { + set := val.(*bpPropertySet) + h.AssertDeepEquals("wrong x value", "taxi", set.getValue("x")) + h.AssertDeepEquals("wrong y value", 1729, set.getValue("y")) + + subset := set.getValue("sub").(*bpPropertySet) + h.AssertDeepEquals("wrong sub.x value", "taxi", subset.getValue("x")) + h.AssertDeepEquals("wrong sub.y value", 1729, subset.getValue("y")) + + if hasTags { + h.AssertDeepEquals("wrong y tag", "tag_y", set.getTag("y")) + h.AssertDeepEquals("wrong sub.x tag", "tag_x", subset.getTag("x")) + } else { + h.AssertDeepEquals("wrong y tag", nil, set.getTag("y")) + h.AssertDeepEquals("wrong sub.x tag", nil, subset.getTag("x")) + } +} + +func TestAddPropertySimple(t *testing.T) { + h := &TestHelper{t} + set := newPropertySet() + for name, val := range map[string]interface{}{ + "x": "taxi", + "y": 1729, + "t": true, + "f": false, + "arr": []string{"a", "b", "c"}, + } { + set.AddProperty(name, val) + h.AssertDeepEquals("wrong value", val, set.getValue(name)) + } + h.AssertPanic("adding x again should panic", + func() { set.AddProperty("x", "taxi") }) + h.AssertPanic("adding arr again should panic", + func() { set.AddProperty("arr", []string{"d"}) }) +} + +func TestAddPropertySubset(t *testing.T) { + h := &TestHelper{t} + getFixtureMap := map[string]func() interface{}{ + "property set": propertySetFixture, + "property struct": propertyStructFixture, + } + + t.Run("add new subset", func(t *testing.T) { + for name, getFixture := range getFixtureMap { + t.Run(name, func(t *testing.T) { + set := propertySetFixture().(*bpPropertySet) + set.AddProperty("new", getFixture()) + checkPropertySetFixture(h, set, true) + checkPropertySetFixture(h, set.getValue("new"), name == "property set") + }) + } + }) + + t.Run("merge existing subset", func(t *testing.T) { + for name, getFixture := range getFixtureMap { + t.Run(name, func(t *testing.T) { + set := newPropertySet() + subset := set.AddPropertySet("sub") + subset.AddProperty("flag", false) + subset.AddPropertySet("sub") + set.AddProperty("sub", getFixture()) + merged := set.getValue("sub").(*bpPropertySet) + h.AssertDeepEquals("wrong flag value", false, merged.getValue("flag")) + checkPropertySetFixture(h, merged, name == "property set") + }) + } + }) + + t.Run("add conflicting subset", func(t *testing.T) { + set := propertySetFixture().(*bpPropertySet) + h.AssertPanic("adding x again should panic", + func() { set.AddProperty("x", propertySetFixture()) }) + }) + + t.Run("add non-pointer struct", func(t *testing.T) { + set := propertySetFixture().(*bpPropertySet) + str := propertyStructFixture().(*propertyStruct) + h.AssertPanic("adding a non-pointer struct should panic", + func() { set.AddProperty("new", *str) }) + }) +} + +func TestAddPropertySetNew(t *testing.T) { + h := &TestHelper{t} + set := newPropertySet() + subset := set.AddPropertySet("sub") + subset.AddProperty("new", "d^^b") + h.AssertDeepEquals("wrong sub.new value", "d^^b", set.getValue("sub").(*bpPropertySet).getValue("new")) +} + +func TestAddPropertySetExisting(t *testing.T) { + h := &TestHelper{t} + set := propertySetFixture().(*bpPropertySet) + subset := set.AddPropertySet("sub") + subset.AddProperty("new", "d^^b") + h.AssertDeepEquals("wrong sub.new value", "d^^b", set.getValue("sub").(*bpPropertySet).getValue("new")) +} + type removeFredTransformation struct { identityTransformation } diff --git a/sdk/testing.go b/sdk/testing.go index b53558d9f..68058ee0b 100644 --- a/sdk/testing.go +++ b/sdk/testing.go @@ -217,6 +217,22 @@ func (h *TestHelper) AssertDeepEquals(message string, expected interface{}, actu } } +func (h *TestHelper) AssertPanic(message string, funcThatShouldPanic func()) { + h.t.Helper() + panicked := false + func() { + defer func() { + if x := recover(); x != nil { + panicked = true + } + }() + funcThatShouldPanic() + }() + if !panicked { + h.t.Error(message) + } +} + // Encapsulates result of processing an SDK definition. Provides support for // checking the state of the build structures. type testSdkResult struct {