From bd3a16b5e75e15d91d6a8cb16ccc53d9d8e202a6 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 25 Apr 2023 11:30:51 -0700 Subject: [PATCH] Install sdk variants in unbundled builds and package uninstallable variants This effectively undoes both If6c3ee82d588e2742c85cef7244c090c93f38b8e and I682e4f1f477f3024f7719dfaa67006ef335e0640. SDK variants are now installed again, which will fix unbundled builds of cc_test modules. The platform variants used by com.android.virt are now packagable even though they are not installable. Fix the original problem in b/194403710 by adding a flag to platform variants of modules in apexes that are not platform available, and using that to prevent install and packaging dependencies. That allows the HideFromMake flag to go back to being used for preventing install dependencies but not packaging dependencies. Test: TestPackagingWithSkipInstallDeps Test: TestFileSystemShouldInstallCoreVariantIfTargetBuildAppsIsSet Test: TestFileSystemShouldSkipApexLibraries Bug: 194403710 Bug: 268582372 Fixes: 274443025 Change-Id: If5418df3ddbb940bd631caebdf38daa81e71f40e --- android/deptag.go | 4 +- android/license_metadata.go | 2 +- android/module.go | 30 ++++++++++-- android/packaging_test.go | 4 +- apex/apex_test.go | 52 +++++++++++++++++++++ cc/androidmk.go | 13 ++---- cc/sdk.go | 6 +-- cc/sdk_test.go | 92 ------------------------------------- 8 files changed, 92 insertions(+), 111 deletions(-) diff --git a/android/deptag.go b/android/deptag.go index be5c35c8d..a15443b4a 100644 --- a/android/deptag.go +++ b/android/deptag.go @@ -34,10 +34,10 @@ func (i InstallAlwaysNeededDependencyTag) InstallDepNeeded() bool { var _ InstallNeededDependencyTag = InstallAlwaysNeededDependencyTag{} -// IsInstallDepNeeded returns true if the dependency tag implements the InstallNeededDependencyTag +// IsInstallDepNeededTag returns true if the dependency tag implements the InstallNeededDependencyTag // interface and the InstallDepNeeded returns true, meaning that the installed files of the parent // should depend on the installed files of the child. -func IsInstallDepNeeded(tag blueprint.DependencyTag) bool { +func IsInstallDepNeededTag(tag blueprint.DependencyTag) bool { if i, ok := tag.(InstallNeededDependencyTag); ok { return i.InstallDepNeeded() } diff --git a/android/license_metadata.go b/android/license_metadata.go index 18b63d310..73000a9fa 100644 --- a/android/license_metadata.go +++ b/android/license_metadata.go @@ -74,7 +74,7 @@ func buildLicenseMetadata(ctx ModuleContext, licenseMetadataFile WritablePath) { if ctx.OtherModuleHasProvider(dep, LicenseMetadataProvider) { info := ctx.OtherModuleProvider(dep, LicenseMetadataProvider).(*LicenseMetadataInfo) allDepMetadataFiles = append(allDepMetadataFiles, info.LicenseMetadataPath) - if isContainer || IsInstallDepNeeded(ctx.OtherModuleDependencyTag(dep)) { + if isContainer || isInstallDepNeeded(dep, ctx.OtherModuleDependencyTag(dep)) { allDepMetadataDepSets = append(allDepMetadataDepSets, info.LicenseMetadataDepSet) } diff --git a/android/module.go b/android/module.go index c8670c31a..db602a0aa 100644 --- a/android/module.go +++ b/android/module.go @@ -925,6 +925,12 @@ type commonProperties struct { // and don't create a rule to install the file. SkipInstall bool `blueprint:"mutated"` + // UninstallableApexPlatformVariant is set by MakeUninstallable called by the apex + // mutator. MakeUninstallable also sets HideFromMake. UninstallableApexPlatformVariant + // is used to avoid adding install or packaging dependencies into libraries provided + // by apexes. + UninstallableApexPlatformVariant bool `blueprint:"mutated"` + // Whether the module has been replaced by a prebuilt ReplacedByPrebuilt bool `blueprint:"mutated"` @@ -2009,6 +2015,7 @@ func (m *ModuleBase) IsSkipInstall() bool { // have other side effects, in particular when it adds a NOTICE file target, // which other install targets might depend on. func (m *ModuleBase) MakeUninstallable() { + m.commonProperties.UninstallableApexPlatformVariant = true m.HideFromMake() } @@ -2038,13 +2045,19 @@ func (m *ModuleBase) EffectiveLicenseFiles() Paths { } // computeInstallDeps finds the installed paths of all dependencies that have a dependency -// tag that is annotated as needing installation via the IsInstallDepNeeded method. +// tag that is annotated as needing installation via the isInstallDepNeeded method. func (m *ModuleBase) computeInstallDeps(ctx ModuleContext) ([]*installPathsDepSet, []*packagingSpecsDepSet) { var installDeps []*installPathsDepSet var packagingSpecs []*packagingSpecsDepSet ctx.VisitDirectDeps(func(dep Module) { - if IsInstallDepNeeded(ctx.OtherModuleDependencyTag(dep)) && !dep.IsHideFromMake() && !dep.IsSkipInstall() { - installDeps = append(installDeps, dep.base().installFilesDepSet) + if isInstallDepNeeded(dep, ctx.OtherModuleDependencyTag(dep)) { + // Installation is still handled by Make, so anything hidden from Make is not + // installable. + if !dep.IsHideFromMake() && !dep.IsSkipInstall() { + installDeps = append(installDeps, dep.base().installFilesDepSet) + } + // Add packaging deps even when the dependency is not installed so that uninstallable + // modules can still be packaged. Often the package will be installed instead. packagingSpecs = append(packagingSpecs, dep.base().packagingSpecsDepSet) } }) @@ -2052,6 +2065,17 @@ func (m *ModuleBase) computeInstallDeps(ctx ModuleContext) ([]*installPathsDepSe return installDeps, packagingSpecs } +// isInstallDepNeeded returns true if installing the output files of the current module +// should also install the output files of the given dependency and dependency tag. +func isInstallDepNeeded(dep Module, tag blueprint.DependencyTag) bool { + // Don't add a dependency from the platform to a library provided by an apex. + if dep.base().commonProperties.UninstallableApexPlatformVariant { + return false + } + // Only install modules if the dependency tag is an InstallDepNeeded tag. + return IsInstallDepNeededTag(tag) +} + func (m *ModuleBase) FilesToInstall() InstallPaths { return m.installFiles } diff --git a/android/packaging_test.go b/android/packaging_test.go index 91ac1f386..383343723 100644 --- a/android/packaging_test.go +++ b/android/packaging_test.go @@ -373,7 +373,7 @@ func TestPackagingBaseSingleTarget(t *testing.T) { func TestPackagingWithSkipInstallDeps(t *testing.T) { // package -[dep]-> foo -[dep]-> bar -[dep]-> baz - // OK SKIPPED + // Packaging should continue transitively through modules that are not installed. multiTarget := false runPackagingTest(t, multiTarget, ` @@ -396,5 +396,5 @@ func TestPackagingWithSkipInstallDeps(t *testing.T) { name: "package", deps: ["foo"], } - `, []string{"lib64/foo"}) + `, []string{"lib64/foo", "lib64/bar", "lib64/baz"}) } diff --git a/apex/apex_test.go b/apex/apex_test.go index 3ba4d8d55..d0d4d0d93 100644 --- a/apex/apex_test.go +++ b/apex/apex_test.go @@ -33,6 +33,7 @@ import ( "android/soong/cc" "android/soong/dexpreopt" prebuilt_etc "android/soong/etc" + "android/soong/filesystem" "android/soong/java" "android/soong/rust" "android/soong/sh" @@ -10375,3 +10376,54 @@ func TestStubLibrariesMultipleApexViolation(t *testing.T) { } } } + +func TestFileSystemShouldSkipApexLibraries(t *testing.T) { + context := android.GroupFixturePreparers( + android.PrepareForIntegrationTestWithAndroid, + cc.PrepareForIntegrationTestWithCc, + PrepareForTestWithApexBuildComponents, + prepareForTestWithMyapex, + filesystem.PrepareForTestWithFilesystemBuildComponents, + ) + result := context.RunTestWithBp(t, ` + android_system_image { + name: "myfilesystem", + deps: [ + "libfoo", + ], + linker_config_src: "linker.config.json", + } + + cc_library { + name: "libfoo", + shared_libs: [ + "libbar", + ], + stl: "none", + } + + cc_library { + name: "libbar", + stl: "none", + apex_available: ["myapex"], + } + + apex { + name: "myapex", + native_shared_libs: ["libbar"], + key: "myapex.key", + updatable: false, + } + + apex_key { + name: "myapex.key", + public_key: "testkey.avbpubkey", + private_key: "testkey.pem", + } + `) + + inputs := result.ModuleForTests("myfilesystem", "android_common").Output("deps.zip").Implicits + android.AssertStringListDoesNotContain(t, "filesystem should not have libbar", + inputs.Strings(), + "out/soong/.intermediates/libbar/android_arm64_armv8-a_shared/libbar.so") +} diff --git a/cc/androidmk.go b/cc/androidmk.go index 980dd0762..ce35b5c44 100644 --- a/cc/androidmk.go +++ b/cc/androidmk.go @@ -124,17 +124,14 @@ func (c *Module) AndroidMkEntries() []android.AndroidMkEntries { } } } - if c.Properties.IsSdkVariant { + if c.Properties.IsSdkVariant && c.Properties.SdkAndPlatformVariantVisibleToMake { // Make the SDK variant uninstallable so that there are not two rules to install // to the same location. entries.SetBool("LOCAL_UNINSTALLABLE_MODULE", true) - - if c.Properties.SdkAndPlatformVariantVisibleToMake { - // Add the unsuffixed name to SOONG_SDK_VARIANT_MODULES so that Make can rewrite - // dependencies to the .sdk suffix when building a module that uses the SDK. - entries.SetString("SOONG_SDK_VARIANT_MODULES", - "$(SOONG_SDK_VARIANT_MODULES) $(patsubst %.sdk,%,$(LOCAL_MODULE))") - } + // Add the unsuffixed name to SOONG_SDK_VARIANT_MODULES so that Make can rewrite + // dependencies to the .sdk suffix when building a module that uses the SDK. + entries.SetString("SOONG_SDK_VARIANT_MODULES", + "$(SOONG_SDK_VARIANT_MODULES) $(patsubst %.sdk,%,$(LOCAL_MODULE))") } }, }, diff --git a/cc/sdk.go b/cc/sdk.go index 4f361eb16..6341926ff 100644 --- a/cc/sdk.go +++ b/cc/sdk.go @@ -47,16 +47,16 @@ func sdkMutator(ctx android.BottomUpMutatorContext) { // Mark the SDK variant. modules[1].(*Module).Properties.IsSdkVariant = true - // SDK variant is not supposed to be installed - modules[1].(*Module).Properties.PreventInstall = true if ctx.Config().UnbundledBuildApps() { // For an unbundled apps build, hide the platform variant from Make. modules[0].(*Module).Properties.HideFromMake = true + modules[0].(*Module).Properties.PreventInstall = true } else { // For a platform build, mark the SDK variant so that it gets a ".sdk" suffix when // exposed to Make. modules[1].(*Module).Properties.SdkAndPlatformVariantVisibleToMake = true + modules[1].(*Module).Properties.PreventInstall = true } ctx.AliasVariation("") } else if isCcModule && ccModule.isImportedApiLibrary() { @@ -74,8 +74,8 @@ func sdkMutator(ctx android.BottomUpMutatorContext) { if apiLibrary.hasApexStubs() { // For an unbundled apps build, hide the platform variant from Make. modules[1].(*Module).Properties.HideFromMake = true - modules[1].(*Module).Properties.PreventInstall = true } + modules[1].(*Module).Properties.PreventInstall = true } else { // For a platform build, mark the SDK variant so that it gets a ".sdk" suffix when // exposed to Make. diff --git a/cc/sdk_test.go b/cc/sdk_test.go index 790440cb9..61925e30c 100644 --- a/cc/sdk_test.go +++ b/cc/sdk_test.go @@ -101,95 +101,3 @@ func TestSdkMutator(t *testing.T) { assertDep(t, libsdkNDK, libcxxNDK) assertDep(t, libsdkPlatform, libcxxPlatform) } - -func TestMakeModuleNameForSdkVariant(t *testing.T) { - bp := ` - cc_library { - name: "libfoo", - srcs: ["main_test.cpp"], - sdk_version: "current", - stl: "none", - } - ` - platformVariant := "android_arm64_armv8-a_shared" - sdkVariant := "android_arm64_armv8-a_sdk_shared" - testCases := []struct { - name string - unbundledApps []string - variant string - skipInstall bool // soong skips install - hideFromMake bool // no make entry - makeUninstallable bool // make skips install - makeModuleName string - }{ - { - name: "platform variant in normal builds", - unbundledApps: nil, - variant: platformVariant, - // installable in soong - skipInstall: false, - // visiable in Make as "libfoo" - hideFromMake: false, - makeModuleName: "libfoo", - // installable in Make - makeUninstallable: false, - }, - { - name: "sdk variant in normal builds", - unbundledApps: nil, - variant: sdkVariant, - // soong doesn't install - skipInstall: true, - // visible in Make as "libfoo.sdk" - hideFromMake: false, - makeModuleName: "libfoo.sdk", - // but not installed - makeUninstallable: true, - }, - { - name: "platform variant in unbunded builds", - unbundledApps: []string{"bar"}, - variant: platformVariant, - // installable in soong - skipInstall: false, - // hidden from make - hideFromMake: true, - }, - { - name: "sdk variant in unbunded builds", - unbundledApps: []string{"bar"}, - variant: sdkVariant, - // soong doesn't install - skipInstall: true, - // visible in Make as "libfoo" - hideFromMake: false, - makeModuleName: "libfoo", - // but not installed - makeUninstallable: true, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - fixture := android.GroupFixturePreparers(prepareForCcTest, - android.FixtureModifyProductVariables(func(variables android.FixtureProductVariables) { - variables.Unbundled_build_apps = tc.unbundledApps - }), - ) - ctx := fixture.RunTestWithBp(t, bp).TestContext - module := ctx.ModuleForTests("libfoo", tc.variant).Module().(*Module) - android.AssertBoolEquals(t, "IsSkipInstall", tc.skipInstall, module.IsSkipInstall()) - android.AssertBoolEquals(t, "HideFromMake", tc.hideFromMake, module.HiddenFromMake()) - if !tc.hideFromMake { - entries := android.AndroidMkEntriesForTest(t, ctx, module)[0] - android.AssertStringEquals(t, "LOCAL_MODULE", - tc.makeModuleName, entries.EntryMap["LOCAL_MODULE"][0]) - actualUninstallable := false - if actual, ok := entries.EntryMap["LOCAL_UNINSTALLABLE_MODULE"]; ok { - actualUninstallable = "true" == actual[0] - } - android.AssertBoolEquals(t, "LOCAL_UNINSTALLABLE_MODULE", - tc.makeUninstallable, actualUninstallable) - } - }) - } -}