From 4b8b79394fa5eeddc532aa9e38870dd1c3a291b5 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 6 May 2020 12:35:38 +0100 Subject: [PATCH] Allow extractCommonProperties to return an error Refactoring in preparation for follow up changes. Also: * Adds a new AssertErrorMessageEquals() helper method. * Improved error reporting in the accessor and added name to extractorProperty to ensure meaningful errors are reported. * Added String() string method to propertiesContainer. * Reports errors using the field name as the errors are not really fixable by developers and it is more meaningful to the build team. Bug: 155628860 Test: m nothing Change-Id: I5c5b8436bcbc39e4e7cd35df2577b2dac53e702a --- sdk/sdk_test.go | 14 +++++++++- sdk/testing.go | 9 +++++++ sdk/update.go | 71 ++++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index 7ae3a03b9..095f83607 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -231,6 +231,7 @@ type EmbeddedPropertiesStruct struct { } type testPropertiesStruct struct { + name string private string Public_Kept string `sdk:"keep"` S_Common string @@ -246,10 +247,17 @@ func (p *testPropertiesStruct) optimizableProperties() interface{} { return p } +func (p *testPropertiesStruct) String() string { + return p.name +} + +var _ propertiesContainer = (*testPropertiesStruct)(nil) + func TestCommonValueOptimization(t *testing.T) { - common := &testPropertiesStruct{} + common := &testPropertiesStruct{name: "common"} structs := []propertiesContainer{ &testPropertiesStruct{ + name: "struct-0", private: "common", Public_Kept: "common", S_Common: "common", @@ -264,6 +272,7 @@ func TestCommonValueOptimization(t *testing.T) { }, }, &testPropertiesStruct{ + name: "struct-1", private: "common", Public_Kept: "common", S_Common: "common", @@ -285,6 +294,7 @@ func TestCommonValueOptimization(t *testing.T) { h := TestHelper{t} h.AssertDeepEquals("common properties not correct", &testPropertiesStruct{ + name: "common", private: "", Public_Kept: "", S_Common: "common", @@ -302,6 +312,7 @@ func TestCommonValueOptimization(t *testing.T) { h.AssertDeepEquals("updated properties[0] not correct", &testPropertiesStruct{ + name: "struct-0", private: "common", Public_Kept: "common", S_Common: "", @@ -319,6 +330,7 @@ func TestCommonValueOptimization(t *testing.T) { h.AssertDeepEquals("updated properties[1] not correct", &testPropertiesStruct{ + name: "struct-1", private: "common", Public_Kept: "common", S_Common: "", diff --git a/sdk/testing.go b/sdk/testing.go index 9e272019e..14a397c68 100644 --- a/sdk/testing.go +++ b/sdk/testing.go @@ -173,6 +173,15 @@ func (h *TestHelper) AssertStringEquals(message string, expected string, actual } } +func (h *TestHelper) AssertErrorMessageEquals(message string, expected string, actual error) { + h.t.Helper() + if actual == nil { + h.t.Errorf("Expected error but was nil") + } else if actual.Error() != expected { + h.t.Errorf("%s: expected %s, actual %s", message, expected, actual.Error()) + } +} + func (h *TestHelper) AssertTrimmedStringEquals(message string, expected string, actual string) { h.t.Helper() h.AssertStringEquals(message, strings.TrimSpace(expected), strings.TrimSpace(actual)) diff --git a/sdk/update.go b/sdk/update.go index ae74b9dd5..03a5c0379 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -306,13 +306,13 @@ func (s *sdk) buildSnapshot(ctx android.ModuleContext, sdkVariants []*sdk) andro for _, sdkVariant := range sdkVariants { properties := sdkVariant.dynamicMemberTypeListProperties osTypeToMemberProperties[sdkVariant.Target().Os] = sdkVariant - dynamicMemberPropertiesContainers = append(dynamicMemberPropertiesContainers, &dynamicMemberPropertiesContainer{properties}) + dynamicMemberPropertiesContainers = append(dynamicMemberPropertiesContainers, &dynamicMemberPropertiesContainer{sdkVariant, properties}) } // Extract the common lists of members into a separate struct. commonDynamicMemberProperties := s.dynamicSdkMemberTypes.createMemberListProperties() extractor := newCommonValueExtractor(commonDynamicMemberProperties) - extractor.extractCommonProperties(commonDynamicMemberProperties, dynamicMemberPropertiesContainers) + extractCommonProperties(ctx, extractor, commonDynamicMemberProperties, dynamicMemberPropertiesContainers) // Add properties common to all os types. s.addMemberPropertiesToPropertySet(builder, snapshotModule, commonDynamicMemberProperties) @@ -389,6 +389,13 @@ func (s *sdk) buildSnapshot(ctx android.ModuleContext, sdkVariants []*sdk) andro return outputZipFile } +func extractCommonProperties(ctx android.ModuleContext, extractor *commonValueExtractor, commonProperties interface{}, inputPropertiesSlice interface{}) { + err := extractor.extractCommonProperties(commonProperties, inputPropertiesSlice) + if err != nil { + ctx.ModuleErrorf("error extracting common properties: %s", err) + } +} + func (s *sdk) addMemberPropertiesToPropertySet(builder *snapshotBuilder, propertySet android.BpPropertySet, dynamicMemberTypeListProperties interface{}) { for _, memberListProperty := range s.memberListProperties() { names := memberListProperty.getter(dynamicMemberTypeListProperties) @@ -829,6 +836,8 @@ type osTypeSpecificInfo struct { archInfos []*archTypeSpecificInfo } +var _ propertiesContainer = (*osTypeSpecificInfo)(nil) + type variantPropertiesFactoryFunc func() android.SdkMemberProperties // Create a new osTypeSpecificInfo for the specified os type and its properties @@ -886,7 +895,7 @@ func newOsTypeSpecificInfo(ctx android.SdkMemberContext, osType android.OsType, // Optimize the properties by extracting common properties from arch type specific // properties into os type specific properties. -func (osInfo *osTypeSpecificInfo) optimizeProperties(commonValueExtractor *commonValueExtractor) { +func (osInfo *osTypeSpecificInfo) optimizeProperties(ctx *memberContext, commonValueExtractor *commonValueExtractor) { // Nothing to do if there is only a single common architecture. if len(osInfo.archInfos) == 0 { return @@ -897,10 +906,10 @@ func (osInfo *osTypeSpecificInfo) optimizeProperties(commonValueExtractor *commo multilib = multilib.addArchType(archInfo.archType) // Optimize the arch properties first. - archInfo.optimizeProperties(commonValueExtractor) + archInfo.optimizeProperties(ctx, commonValueExtractor) } - commonValueExtractor.extractCommonProperties(osInfo.Properties, osInfo.archInfos) + extractCommonProperties(ctx.sdkMemberContext, commonValueExtractor, osInfo.Properties, osInfo.archInfos) // Choose setting for compile_multilib that is appropriate for the arch variants supplied. osInfo.Properties.Base().Compile_multilib = multilib.String() @@ -973,6 +982,10 @@ func (osInfo *osTypeSpecificInfo) addToPropertySet(ctx *memberContext, bpModule } } +func (osInfo *osTypeSpecificInfo) String() string { + return fmt.Sprintf("OsType{%s}", osInfo.osType) +} + type archTypeSpecificInfo struct { baseInfo @@ -981,6 +994,8 @@ type archTypeSpecificInfo struct { linkInfos []*linkTypeSpecificInfo } +var _ propertiesContainer = (*archTypeSpecificInfo)(nil) + // Create a new archTypeSpecificInfo for the specified arch type and its properties // structures populated with information from the variants. func newArchSpecificInfo(ctx android.SdkMemberContext, archType android.ArchType, variantPropertiesFactory variantPropertiesFactoryFunc, archVariants []android.Module) *archTypeSpecificInfo { @@ -1038,12 +1053,12 @@ func getLinkType(variant android.Module) string { // Optimize the properties by extracting common properties from link type specific // properties into arch type specific properties. -func (archInfo *archTypeSpecificInfo) optimizeProperties(commonValueExtractor *commonValueExtractor) { +func (archInfo *archTypeSpecificInfo) optimizeProperties(ctx *memberContext, commonValueExtractor *commonValueExtractor) { if len(archInfo.linkInfos) == 0 { return } - commonValueExtractor.extractCommonProperties(archInfo.Properties, archInfo.linkInfos) + extractCommonProperties(ctx.sdkMemberContext, commonValueExtractor, archInfo.Properties, archInfo.linkInfos) } // Add the properties for an arch type to a property set. @@ -1058,12 +1073,18 @@ func (archInfo *archTypeSpecificInfo) addToPropertySet(ctx *memberContext, archP } } +func (archInfo *archTypeSpecificInfo) String() string { + return fmt.Sprintf("ArchType{%s}", archInfo.archType) +} + type linkTypeSpecificInfo struct { baseInfo linkType string } +var _ propertiesContainer = (*linkTypeSpecificInfo)(nil) + // Create a new linkTypeSpecificInfo for the specified link type and its properties // structures populated with information from the variant. func newLinkSpecificInfo(ctx android.SdkMemberContext, linkType string, variantPropertiesFactory variantPropertiesFactoryFunc, linkVariant android.Module) *linkTypeSpecificInfo { @@ -1079,6 +1100,10 @@ func newLinkSpecificInfo(ctx android.SdkMemberContext, linkType string, variantP return linkInfo } +func (l *linkTypeSpecificInfo) String() string { + return fmt.Sprintf("LinkType{%s}", l.linkType) +} + type memberContext struct { sdkMemberContext android.ModuleContext builder *snapshotBuilder @@ -1143,11 +1168,11 @@ func (s *sdk) createMemberSnapshot(ctx *memberContext, member *sdkMember, bpModu osSpecificPropertiesContainers = append(osSpecificPropertiesContainers, osInfo) // Optimize the properties across all the variants for a specific os type. - osInfo.optimizeProperties(commonValueExtractor) + osInfo.optimizeProperties(ctx, commonValueExtractor) } // Extract properties which are common across all architectures and os types. - commonValueExtractor.extractCommonProperties(commonProperties, osSpecificPropertiesContainers) + extractCommonProperties(ctx.sdkMemberContext, commonValueExtractor, commonProperties, osSpecificPropertiesContainers) // Add the common properties to the module. commonProperties.AddToPropertySet(ctx, bpModule) @@ -1192,6 +1217,9 @@ type fieldAccessorFunc func(structValue reflect.Value) reflect.Value // A property that can be optimized by the commonValueExtractor. type extractorProperty struct { + // The name of the field for this property. + name string + // Retrieves the value on which common value optimization will be performed. getter fieldAccessorFunc @@ -1199,6 +1227,10 @@ type extractorProperty struct { emptyValue reflect.Value } +func (p extractorProperty) String() string { + return p.name +} + // Supports extracting common values from a number of instances of a properties // structure into a separate common set of properties. type commonValueExtractor struct { @@ -1240,6 +1272,9 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS // Save a copy of the field index for use in the function. fieldIndex := f + + name := field.Name + fieldGetter := func(value reflect.Value) reflect.Value { if containingStructAccessor != nil { // This is an embedded structure so first access the field for the embedded @@ -1250,6 +1285,12 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS // Skip through interface and pointer values to find the structure. value = getStructValue(value) + defer func() { + if r := recover(); r != nil { + panic(fmt.Errorf("%s for fieldIndex %d of field %s of value %#v", r, fieldIndex, name, value.Interface())) + } + }() + // Return the field. return value.Field(fieldIndex) } @@ -1259,6 +1300,7 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS e.gatherFields(field.Type, fieldGetter) } else { property := extractorProperty{ + name, fieldGetter, reflect.Zero(field.Type), } @@ -1288,12 +1330,15 @@ foundStruct: // Allows additional information to be associated with the properties, e.g. for // filtering. type propertiesContainer interface { + fmt.Stringer + // Get the properties that need optimizing. optimizableProperties() interface{} } // A wrapper for dynamic member properties to allow them to be optimized. type dynamicMemberPropertiesContainer struct { + sdkVariant *sdk dynamicMemberProperties interface{} } @@ -1301,6 +1346,10 @@ func (c dynamicMemberPropertiesContainer) optimizableProperties() interface{} { return c.dynamicMemberProperties } +func (c dynamicMemberPropertiesContainer) String() string { + return c.sdkVariant.String() +} + // Extract common properties from a slice of property structures of the same type. // // All the property structures must be of the same type. @@ -1311,7 +1360,7 @@ func (c dynamicMemberPropertiesContainer) optimizableProperties() interface{} { // have the same value (using DeepEquals) across all the input properties. If it does not then no // change is made. Otherwise, the common value is stored in the field in the commonProperties // and the field in each of the input properties structure is set to its default value. -func (e *commonValueExtractor) extractCommonProperties(commonProperties interface{}, inputPropertiesSlice interface{}) { +func (e *commonValueExtractor) extractCommonProperties(commonProperties interface{}, inputPropertiesSlice interface{}) error { commonPropertiesValue := reflect.ValueOf(commonProperties) commonStructValue := commonPropertiesValue.Elem() @@ -1356,4 +1405,6 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac } } } + + return nil }