From b97b1579d3374ada49887d356dd6dd693d32a18d Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Thu, 29 Apr 2021 21:50:40 +0100 Subject: [PATCH] Remove duplicate component from sdk snapshot Previously, an sdk snapshot could contain the following: * A java_sdk_library_import module, e.g. "foo" which creates component modules "foo.stubs", etc. * A corresponding versioned module, e.g. "sdk_foo@current" which created component modules "sdk_foo@current.stubs", etc. * An internal (to the sdk snapshot) java_import for one of "foo"'s components, e.g. "sdk_foo.stubs" * A corresponding versioned module, e.g. "sdk_foo.stubs@current". That causes a few problems: 1. The "foo.stubs" is duplicated. 2. The names of the components created by the versioned java_sdk_library_import are invalid, as they append the component's suffix to the version and not the name before the version. The latter causes problems when building against prebuilts and fixing that causes the generated snapshot to be invalid because it contains duplicate definitions of the "sdk_foo.stubs@current" module. One explicitly in the Android.bp file and one created by the "sdk_foo@current" module. Removing the duplicates from the snapshot causes errors as the name generated by the snapshot for the component module, i.e. "sdk_foo.stubs@current" does not match the name generated by the "sdk_foo@current", i.e. "sdk_foo@current.stubs". This change fixes them together. Bug: 179354495 Test: m nothing Merged-In: I515f235fe21755b5275af12366e96c24c94c0273 Change-Id: I515f235fe21755b5275af12366e96c24c94c0273 (cherry picked from commit a1aa7387f74a49c8c974ba2198def0e081488624) --- android/sdk.go | 69 +++++++++++++++++++++++++ java/sdk_library.go | 22 ++++++-- java/sdk_library_test.go | 13 ++++- sdk/java_sdk_test.go | 27 ++-------- sdk/update.go | 109 ++++++++++++++++++++++++++++++++------- 5 files changed, 195 insertions(+), 45 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index 93beb6e21..5c58612b0 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -38,6 +38,36 @@ type RequiredSdks interface { type sdkAwareWithoutModule interface { RequiredSdks + // SdkMemberComponentName will return the name to use for a component of this module based on the + // base name of this module. + // + // The baseName is the name returned by ModuleBase.BaseModuleName(), i.e. the name specified in + // the name property in the .bp file so will not include the prebuilt_ prefix. + // + // The componentNameCreator is a func for creating the name of a component from the base name of + // the module, e.g. it could just append ".component" to the name passed in. + // + // This is intended to be called by prebuilt modules that create component models. It is because + // prebuilt module base names come in a variety of different forms: + // * unversioned - this is the same as the source module. + // * internal to an sdk - this is the unversioned name prefixed by the base name of the sdk + // module. + // * versioned - this is the same as the internal with the addition of an "@" suffix. + // + // While this can be called from a source module in that case it will behave the same way as the + // unversioned name and return the result of calling the componentNameCreator func on the supplied + // base name. + // + // e.g. Assuming the componentNameCreator func simply appends ".component" to the name passed in + // then this will work as follows: + // * An unversioned name of "foo" will return "foo.component". + // * An internal to the sdk name of "sdk_foo" will return "sdk_foo.component". + // * A versioned name of "sdk_foo@current" will return "sdk_foo.component@current". + // + // Note that in the latter case the ".component" suffix is added before the version. Adding it + // after would change the version. + SdkMemberComponentName(baseName string, componentNameCreator func(string) string) string + sdkBase() *SdkBase MakeMemberOf(sdk SdkRef) IsInAnySdk() bool @@ -135,6 +165,18 @@ func (s *SdkBase) sdkBase() *SdkBase { return s } +func (s *SdkBase) SdkMemberComponentName(baseName string, componentNameCreator func(string) string) string { + if s.MemberName() == "" { + return componentNameCreator(baseName) + } else { + index := strings.LastIndex(baseName, "@") + unversionedName := baseName[:index] + unversionedComponentName := componentNameCreator(unversionedName) + versionSuffix := baseName[index:] + return unversionedComponentName + versionSuffix + } +} + // MakeMemberOf sets this module to be a member of a specific SDK func (s *SdkBase) MakeMemberOf(sdk SdkRef) { s.properties.ContainingSdk = &sdk @@ -659,3 +701,30 @@ type SdkMemberContext interface { // into which to copy the prebuilt files. Name() string } + +// ExportedComponentsInfo contains information about the components that this module exports to an +// sdk snapshot. +// +// A component of a module is a child module that the module creates and which forms an integral +// part of the functionality that the creating module provides. A component module is essentially +// owned by its creator and is tightly coupled to the creator and other components. +// +// e.g. the child modules created by prebuilt_apis are not components because they are not tightly +// coupled to the prebuilt_apis module. Once they are created the prebuilt_apis ignores them. The +// child impl and stub library created by java_sdk_library (and corresponding import) are components +// because the creating module depends upon them in order to provide some of its own functionality. +// +// A component is exported if it is part of an sdk snapshot. e.g. The xml and impl child modules are +// components but they are not exported as they are not part of an sdk snapshot. +// +// This information is used by the sdk snapshot generation code to ensure that it does not create +// an sdk snapshot that contains a declaration of the component module and the module that creates +// it as that would result in duplicate modules when attempting to use the snapshot. e.g. a snapshot +// that included the java_sdk_library_import "foo" and also a java_import "foo.stubs" would fail +// as there would be two modules called "foo.stubs". +type ExportedComponentsInfo struct { + // The names of the exported components. + Components []string +} + +var ExportedComponentsInfoProvider = blueprint.NewProvider(ExportedComponentsInfo{}) diff --git a/java/sdk_library.go b/java/sdk_library.go index 101a658e5..b07dcd151 100644 --- a/java/sdk_library.go +++ b/java/sdk_library.go @@ -634,7 +634,7 @@ type commonToSdkLibraryAndImportProperties struct { // commonSdkLibraryAndImportModule defines the interface that must be provided by a module that // embeds the commonToSdkLibraryAndImport struct. type commonSdkLibraryAndImportModule interface { - android.Module + android.SdkAware BaseModuleName() string } @@ -700,13 +700,19 @@ func (c *commonToSdkLibraryAndImport) xmlPermissionsModuleName() string { // Name of the java_library module that compiles the stubs source. func (c *commonToSdkLibraryAndImport) stubsLibraryModuleName(apiScope *apiScope) string { - return c.namingScheme.stubsLibraryModuleName(apiScope, c.module.BaseModuleName()) + baseName := c.module.BaseModuleName() + return c.module.SdkMemberComponentName(baseName, func(name string) string { + return c.namingScheme.stubsLibraryModuleName(apiScope, name) + }) } // Name of the droidstubs module that generates the stubs source and may also // generate/check the API. func (c *commonToSdkLibraryAndImport) stubsSourceModuleName(apiScope *apiScope) string { - return c.namingScheme.stubsSourceModuleName(apiScope, c.module.BaseModuleName()) + baseName := c.module.BaseModuleName() + return c.module.SdkMemberComponentName(baseName, func(name string) string { + return c.namingScheme.stubsSourceModuleName(apiScope, name) + }) } // The component names for different outputs of the java_sdk_library. @@ -1170,6 +1176,10 @@ func (module *SdkLibrary) GenerateAndroidBuildActions(ctx android.ModuleContext) module.Library.GenerateAndroidBuildActions(ctx) } + // Collate the components exported by this module. All scope specific modules are exported but + // the impl and xml component modules are not. + exportedComponents := map[string]struct{}{} + // Record the paths to the header jars of the library (stubs and impl). // When this java_sdk_library is depended upon from others via "libs" property, // the recorded paths will be returned depending on the link type of the caller. @@ -1184,8 +1194,14 @@ func (module *SdkLibrary) GenerateAndroidBuildActions(ctx android.ModuleContext) // Extract information from the dependency. The exact information extracted // is determined by the nature of the dependency which is determined by the tag. scopeTag.extractDepInfo(ctx, to, scopePaths) + + exportedComponents[ctx.OtherModuleName(to)] = struct{}{} } }) + + // Make the set of components exported by this module available for use elsewhere. + exportedComponentInfo := android.ExportedComponentsInfo{Components: android.SortedStringKeys(exportedComponents)} + ctx.SetProvider(android.ExportedComponentsInfoProvider, exportedComponentInfo) } func (module *SdkLibrary) AndroidMkEntries() []android.AndroidMkEntries { diff --git a/java/sdk_library_test.go b/java/sdk_library_test.go index 2520dde6e..65af95314 100644 --- a/java/sdk_library_test.go +++ b/java/sdk_library_test.go @@ -110,7 +110,7 @@ func TestJavaSdkLibrary(t *testing.T) { `) // check the existence of the internal modules - result.ModuleForTests("foo", "android_common") + foo := result.ModuleForTests("foo", "android_common") result.ModuleForTests(apiScopePublic.stubsLibraryModuleName("foo"), "android_common") result.ModuleForTests(apiScopeSystem.stubsLibraryModuleName("foo"), "android_common") result.ModuleForTests(apiScopeTest.stubsLibraryModuleName("foo"), "android_common") @@ -122,6 +122,17 @@ func TestJavaSdkLibrary(t *testing.T) { result.ModuleForTests("foo.api.system.28", "") result.ModuleForTests("foo.api.test.28", "") + exportedComponentsInfo := result.ModuleProvider(foo.Module(), ExportedComponentsInfoProvider).(ExportedComponentsInfo) + expectedFooExportedComponents := []string{ + "foo.stubs", + "foo.stubs.source", + "foo.stubs.source.system", + "foo.stubs.source.test", + "foo.stubs.system", + "foo.stubs.test", + } + android.AssertArrayString(t, "foo exported components", expectedFooExportedComponents, exportedComponentsInfo.Components) + bazJavac := result.ModuleForTests("baz", "android_common").Rule("javac") // tests if baz is actually linked to the stubs lib android.AssertStringDoesContain(t, "baz javac classpath", bazJavac.Args["classpath"], "foo.stubs.system.jar") diff --git a/sdk/java_sdk_test.go b/sdk/java_sdk_test.go index ad2bd7599..a2cfe6d16 100644 --- a/sdk/java_sdk_test.go +++ b/sdk/java_sdk_test.go @@ -722,14 +722,6 @@ java_import { jars: ["java/system-module.jar"], } -java_import { - name: "mysdk_myjavalib.stubs", - prefer: false, - visibility: ["//visibility:private"], - apex_available: ["//apex_available:platform"], - jars: ["java/myjavalib.stubs.jar"], -} - java_sdk_library_import { name: "myjavalib", prefer: false, @@ -752,7 +744,7 @@ java_system_modules_import { libs: [ "mysdk_system-module", "exported-system-module", - "mysdk_myjavalib.stubs", + "myjavalib.stubs", ], } `), @@ -775,14 +767,6 @@ java_import { jars: ["java/system-module.jar"], } -java_import { - name: "mysdk_myjavalib.stubs@current", - sdk_member_name: "myjavalib.stubs", - visibility: ["//visibility:private"], - apex_available: ["//apex_available:platform"], - jars: ["java/myjavalib.stubs.jar"], -} - java_sdk_library_import { name: "mysdk_myjavalib@current", sdk_member_name: "myjavalib", @@ -820,7 +804,6 @@ sdk_snapshot { checkAllCopyRules(` .intermediates/exported-system-module/android_common/turbine-combined/exported-system-module.jar -> java/exported-system-module.jar .intermediates/system-module/android_common/turbine-combined/system-module.jar -> java/system-module.jar -.intermediates/myjavalib.stubs/android_common/turbine-combined/myjavalib.stubs.jar -> java/myjavalib.stubs.jar .intermediates/myjavalib.stubs/android_common/javac/myjavalib.stubs.jar -> sdk_library/public/myjavalib-stubs.jar .intermediates/myjavalib.stubs.source/android_common/metalava/myjavalib.stubs.source_api.txt -> sdk_library/public/myjavalib.txt .intermediates/myjavalib.stubs.source/android_common/metalava/myjavalib.stubs.source_removed.txt -> sdk_library/public/myjavalib-removed.txt @@ -1153,10 +1136,10 @@ sdk_snapshot { ".intermediates/mysdk/common_os/tmp/sdk_library/test/myjavalib_stub_sources.zip", ), snapshotTestChecker(checkSnapshotWithoutSource, func(t *testing.T, result *android.TestResult) { - // Show that the existing behavior is incorrect as the suffix for the child modules is added - // to the version not before it. - result.Module("mysdk_myjavalib@current.stubs", "android_common") - result.Module("mysdk_myjavalib@current.stubs.source", "android_common") + // Make sure that the name of the child modules created by a versioned java_sdk_library_import + // module is correct, i.e. the suffix is added before the version and not after. + result.Module("mysdk_myjavalib.stubs@current", "android_common") + result.Module("mysdk_myjavalib.stubs.source@current", "android_common") }), ) } diff --git a/sdk/update.go b/sdk/update.go index e2e59979e..3f613399a 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -22,7 +22,6 @@ import ( "android/soong/apex" "android/soong/cc" - "github.com/google/blueprint" "github.com/google/blueprint/proptools" @@ -148,8 +147,8 @@ func (gf *generatedFile) build(pctx android.PackageContext, ctx android.BuilderC // Collect all the members. // -// Updates the sdk module with a list of sdkMemberVariantDeps and details as to which multilibs -// (32/64/both) are used by this sdk variant. +// Updates the sdk module with a list of sdkMemberVariantDep instances and details as to which +// multilibs (32/64/both) are used by this sdk variant. func (s *sdk) collectMembers(ctx android.ModuleContext) { s.multilibUsages = multilibNone ctx.WalkDeps(func(child android.Module, parent android.Module) bool { @@ -165,8 +164,15 @@ func (s *sdk) collectMembers(ctx android.ModuleContext) { // Keep track of which multilib variants are used by the sdk. s.multilibUsages = s.multilibUsages.addArchType(child.Target().Arch.ArchType) + var exportedComponentsInfo android.ExportedComponentsInfo + if ctx.OtherModuleHasProvider(child, android.ExportedComponentsInfoProvider) { + exportedComponentsInfo = ctx.OtherModuleProvider(child, android.ExportedComponentsInfoProvider).(android.ExportedComponentsInfo) + } + export := memberTag.ExportMember() - s.memberVariantDeps = append(s.memberVariantDeps, sdkMemberVariantDep{s, memberType, child.(android.SdkAware), export}) + s.memberVariantDeps = append(s.memberVariantDeps, sdkMemberVariantDep{ + s, memberType, child.(android.SdkAware), export, exportedComponentsInfo, + }) // Recurse down into the member's dependencies as it may have dependencies that need to be // automatically added to the sdk. @@ -253,26 +259,41 @@ func versionedSdkMemberName(ctx android.ModuleContext, memberName string, versio // the contents (header files, stub libraries, etc) into the zip file. func (s *sdk) buildSnapshot(ctx android.ModuleContext, sdkVariants []*sdk) android.OutputPath { - allMembersByName := make(map[string]struct{}) - exportedMembersByName := make(map[string]struct{}) + // Aggregate all the sdkMemberVariantDep instances from all the sdk variants. hasLicenses := false var memberVariantDeps []sdkMemberVariantDep for _, sdkVariant := range sdkVariants { memberVariantDeps = append(memberVariantDeps, sdkVariant.memberVariantDeps...) + } - // Record the names of all the members, both explicitly specified and implicitly - // included. - for _, memberVariantDep := range sdkVariant.memberVariantDeps { - name := memberVariantDep.variant.Name() - allMembersByName[name] = struct{}{} + // Filter out any sdkMemberVariantDep that is a component of another. + memberVariantDeps = filterOutComponents(ctx, memberVariantDeps) - if memberVariantDep.export { - exportedMembersByName[name] = struct{}{} - } + // Record the names of all the members, both explicitly specified and implicitly + // included. + allMembersByName := make(map[string]struct{}) + exportedMembersByName := make(map[string]struct{}) - if memberVariantDep.memberType == android.LicenseModuleSdkMemberType { - hasLicenses = true - } + addMember := func(name string, export bool) { + allMembersByName[name] = struct{}{} + if export { + exportedMembersByName[name] = struct{}{} + } + } + + for _, memberVariantDep := range memberVariantDeps { + name := memberVariantDep.variant.Name() + export := memberVariantDep.export + + addMember(name, export) + + // Add any components provided by the module. + for _, component := range memberVariantDep.exportedComponentsInfo.Components { + addMember(component, export) + } + + if memberVariantDep.memberType == android.LicenseModuleSdkMemberType { + hasLicenses = true } } @@ -431,6 +452,47 @@ be unnecessary as every module in the sdk already has its own licenses property. return outputZipFile } +// filterOutComponents removes any item from the deps list that is a component of another item in +// the deps list, e.g. if the deps list contains "foo" and "foo.stubs" which is component of "foo" +// then it will remove "foo.stubs" from the deps. +func filterOutComponents(ctx android.ModuleContext, deps []sdkMemberVariantDep) []sdkMemberVariantDep { + // Collate the set of components that all the modules added to the sdk provide. + components := map[string]*sdkMemberVariantDep{} + for i, _ := range deps { + dep := &deps[i] + for _, c := range dep.exportedComponentsInfo.Components { + components[c] = dep + } + } + + // If no module provides components then return the input deps unfiltered. + if len(components) == 0 { + return deps + } + + filtered := make([]sdkMemberVariantDep, 0, len(deps)) + for _, dep := range deps { + name := android.RemoveOptionalPrebuiltPrefix(ctx.OtherModuleName(dep.variant)) + if owner, ok := components[name]; ok { + // This is a component of another module that is a member of the sdk. + + // If the component is exported but the owning module is not then the configuration is not + // supported. + if dep.export && !owner.export { + ctx.ModuleErrorf("Module %s is internal to the SDK but provides component %s which is used outside the SDK") + continue + } + + // This module must not be added to the list of members of the sdk as that would result in a + // duplicate module in the sdk snapshot. + continue + } + + filtered = append(filtered, dep) + } + return filtered +} + // addSnapshotModule adds the sdk_snapshot/module_exports_snapshot module to the builder. func (s *sdk) addSnapshotModule(ctx android.ModuleContext, builder *snapshotBuilder, sdkVariants []*sdk, memberVariantDeps []sdkMemberVariantDep) { bpFile := builder.bpFile @@ -1152,9 +1214,18 @@ func addSdkMemberPropertiesToSet(ctx *memberContext, memberProperties android.Sd type sdkMemberVariantDep struct { // The sdk variant that depends (possibly indirectly) on the member variant. sdkVariant *sdk + + // The type of sdk member the variant is to be treated as. memberType android.SdkMemberType - variant android.SdkAware - export bool + + // The variant that is added to the sdk. + variant android.SdkAware + + // True if the member should be exported, i.e. accessible, from outside the sdk. + export bool + + // The names of additional component modules provided by the variant. + exportedComponentsInfo android.ExportedComponentsInfo } var _ android.SdkMember = (*sdkMember)(nil)