From 9ec2a79ab2620f34440072a3541898ec2886fc18 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 6 May 2020 12:50:19 +0100 Subject: [PATCH 1/4] Correct order of parameters to AssertDeepEquals The expected/actual were around the wrong way. Bug: 155628860 Test: m nothing Merged-In: I98b575b8b85dcbfd2075e77689f0329aa2eadbf0 Change-Id: I98b575b8b85dcbfd2075e77689f0329aa2eadbf0 (cherry picked from commit 1d6c0df597d939ae408ba90214f023086fb13aed) --- sdk/sdk_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index 2f125e2d9..7ae3a03b9 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -283,7 +283,7 @@ func TestCommonValueOptimization(t *testing.T) { extractor.extractCommonProperties(common, structs) h := TestHelper{t} - h.AssertDeepEquals("common properties not correct", common, + h.AssertDeepEquals("common properties not correct", &testPropertiesStruct{ private: "", Public_Kept: "", @@ -297,9 +297,10 @@ func TestCommonValueOptimization(t *testing.T) { S_Embedded_Common: "embedded_common", S_Embedded_Different: "", }, - }) + }, + common) - h.AssertDeepEquals("updated properties[0] not correct", structs[0], + h.AssertDeepEquals("updated properties[0] not correct", &testPropertiesStruct{ private: "common", Public_Kept: "common", @@ -313,9 +314,10 @@ func TestCommonValueOptimization(t *testing.T) { S_Embedded_Common: "", S_Embedded_Different: "embedded_upper", }, - }) + }, + structs[0]) - h.AssertDeepEquals("updated properties[1] not correct", structs[1], + h.AssertDeepEquals("updated properties[1] not correct", &testPropertiesStruct{ private: "common", Public_Kept: "common", @@ -329,5 +331,6 @@ func TestCommonValueOptimization(t *testing.T) { S_Embedded_Common: "", S_Embedded_Different: "embedded_lower", }, - }) + }, + structs[1]) } From d7acc7b73e5dcc42d7a06cad90a6f631e6239a7b Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 6 May 2020 12:41:39 +0100 Subject: [PATCH 2/4] Remove reference to obsolete BuildSnapshot() Bug: 155628860 Test: m nothing Merged-In: If673d482e8318b5fbb86780236123b0bd59eb5d3 Change-Id: If673d482e8318b5fbb86780236123b0bd59eb5d3 (cherry picked from commit 425b0eacaaa7f37acd8c99195d53a3299de97df6) --- android/sdk.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index 6f62f552c..873e08942 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -331,13 +331,7 @@ type SdkMemberType interface { // Add a prebuilt module that the sdk will populate. // - // Returning nil from this will cause the sdk module type to use the deprecated BuildSnapshot - // method to build the snapshot. That method is deprecated because it requires the SdkMemberType - // implementation to do all the word. - // - // Otherwise, returning a non-nil value from this will cause the sdk module type to do the - // majority of the work to generate the snapshot. The sdk module code generates the snapshot - // as follows: + // The sdk module code generates the snapshot as follows: // // * A properties struct of type SdkMemberProperties is created for each variant and // populated with information from the variant by calling PopulateFromVariant(SdkAware) From 1c58f5795ae4d9f05f637c60d662da114b993f7b Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 6 May 2020 12:35:38 +0100 Subject: [PATCH 3/4] 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 Merged-In: I5c5b8436bcbc39e4e7cd35df2577b2dac53e702a Change-Id: I5c5b8436bcbc39e4e7cd35df2577b2dac53e702a (cherry picked from commit 4b8b79394fa5eeddc532aa9e38870dd1c3a291b5) --- 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 570ea0fb2..362a352d8 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 } From 2aaef53b6d581ab7f1f931767b5c4fe2c159be24 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 29 Apr 2020 16:47:28 +0100 Subject: [PATCH 4/4] Switch java_sdk_library to use SetDefaultableHook() Previously, java_sdk_library used AddLoadHook() to register a hook that when called would create its child modules. That meant the java_sdk_library properties that were used to create the child modules, e.g. sdk_version could not be defaulted because the modules are created before the defaults are applied. This change switches java_sdk_library to use the new SetDefaultableHook() mechanism to register the hook instead of the AddLoadHook() mechanism. It also prevents the child modules from being created if the module has been disabled. Bug: 155295806 Test: m checkapi Merged-In: Ic6f90eb4449338e549878f64e8119e286b9aa549 Change-Id: Ic6f90eb4449338e549878f64e8119e286b9aa549 (cherry picked from commit f022920bde04b32bdb7479998c65f3405808996f) --- java/sdk_library.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/java/sdk_library.go b/java/sdk_library.go index 3a6c5deb3..1ad5132c4 100644 --- a/java/sdk_library.go +++ b/java/sdk_library.go @@ -383,7 +383,7 @@ func (module *SdkLibrary) apiDistPath(apiScope *apiScope) string { } // Get the sdk version for use when compiling the stubs library. -func (module *SdkLibrary) sdkVersionForStubsLibrary(mctx android.LoadHookContext, apiScope *apiScope) string { +func (module *SdkLibrary) sdkVersionForStubsLibrary(mctx android.DefaultableHookContext, apiScope *apiScope) string { sdkDep := decodeSdkDep(mctx, sdkContext(&module.Library)) if sdkDep.hasStandardLibs() { // If building against a standard sdk then use the sdk version appropriate for the scope. @@ -403,7 +403,7 @@ func (module *SdkLibrary) latestRemovedApiFilegroupName(apiScope *apiScope) stri } // Creates a static java library that has API stubs -func (module *SdkLibrary) createStubsLibrary(mctx android.LoadHookContext, apiScope *apiScope) { +func (module *SdkLibrary) createStubsLibrary(mctx android.DefaultableHookContext, apiScope *apiScope) { props := struct { Name *string Srcs []string @@ -474,7 +474,7 @@ func (module *SdkLibrary) createStubsLibrary(mctx android.LoadHookContext, apiSc // Creates a droidstubs module that creates stubs source files from the given full source // files -func (module *SdkLibrary) createStubsSources(mctx android.LoadHookContext, apiScope *apiScope) { +func (module *SdkLibrary) createStubsSources(mctx android.DefaultableHookContext, apiScope *apiScope) { props := struct { Name *string Srcs []string @@ -593,7 +593,7 @@ func (module *SdkLibrary) DepIsInSameApex(mctx android.BaseModuleContext, dep an } // Creates the xml file that publicizes the runtime library -func (module *SdkLibrary) createXmlFile(mctx android.LoadHookContext) { +func (module *SdkLibrary) createXmlFile(mctx android.DefaultableHookContext) { props := struct { Name *string Lib_name *string @@ -714,7 +714,12 @@ func (module *SdkLibrary) getApiDir() string { // For a java_sdk_library module, create internal modules for stubs, docs, // runtime libs and xml file. If requested, the stubs and docs are created twice // once for public API level and once for system API level -func (module *SdkLibrary) CreateInternalModules(mctx android.LoadHookContext) { +func (module *SdkLibrary) CreateInternalModules(mctx android.DefaultableHookContext) { + // If the module has been disabled then don't create any child modules. + if !module.Enabled() { + return + } + if len(module.Library.Module.properties.Srcs) == 0 { mctx.PropertyErrorf("srcs", "java_sdk_library must specify srcs") return @@ -800,7 +805,7 @@ func SdkLibraryFactory() android.Module { module.InitSdkLibraryProperties() android.InitApexModule(module) InitJavaModule(module, android.HostAndDeviceSupported) - android.AddLoadHook(module, func(ctx android.LoadHookContext) { module.CreateInternalModules(ctx) }) + module.SetDefaultableHook(func(ctx android.DefaultableHookContext) { module.CreateInternalModules(ctx) }) return module }