Refactor the contruction of the manifest check inputs.

This is a no-op change for a majority of cases.

Before this change, the contruction of the manifest check inputs is
confusing. It mutates uses_libs properties in place just for the
manifest check, by replacing module names with library names for
direct dependencies and merging library names from CLC for both direct
denpendencies and transitive denpendencies, and then constructs manifest
check inputs from those mutated uses_libs properties. This is
error-prone and leads to insistency: the goal is to check that the CLC
matches the manifest, but the inputs to the check don't reflect the CLC.

After this change, we no longer mutate uses_libs properties in place.
Instead, we maintain a separate list of missing denpendencies, and then
construct manifest check inputs directly from the CLC for all existing
libraries, no matter they are direct or transtive, and from the separate
list of missing libraries. This change makes the logic more
consistent and straightforward, and it also allows us to easily do the
next change, which is to propagate transtive missing denpendencies.

In fact, this change revealed several bugs around library optionality
and order in CLC construction, and fixed them.

Bug: 331528424
Test: m --no-skip-soong-tests
Ignore-AOSP-First: Depends on internal changes. Will cherry-pick once merged.
Merged-In: I0de82e76c47995b54aba9efd41538d950256a95f
Change-Id: I0de82e76c47995b54aba9efd41538d950256a95f
This commit is contained in:
Jiakai Zhang 2024-04-15 11:15:41 +00:00 committed by Android Build Cherrypicker Worker
parent f875565c7f
commit f98da19a07
9 changed files with 113 additions and 73 deletions

View file

@ -323,6 +323,7 @@ func (clcMap ClassLoaderContextMap) addContext(ctx android.ModuleInstallPathCont
} else if clc.Host == hostPath && clc.Device == devicePath {
// Ok, the same library with the same paths. Don't re-add it, but don't raise an error
// either, as the same library may be reachable via different transitional dependencies.
clc.Optional = clc.Optional && optional
return nil
} else {
// Fail, as someone is trying to add the same library with different paths. This likely

View file

@ -805,12 +805,12 @@ func (a *AndroidLibrary) OutputFiles(tag string) (android.Paths, error) {
var _ AndroidLibraryDependency = (*AndroidLibrary)(nil)
func (a *AndroidLibrary) DepsMutator(ctx android.BottomUpMutatorContext) {
a.usesLibrary.deps(ctx, false)
a.Module.deps(ctx)
sdkDep := decodeSdkDep(ctx, android.SdkContext(a))
if sdkDep.hasFrameworkLibs() {
a.aapt.deps(ctx, sdkDep)
}
a.usesLibrary.deps(ctx, false)
for _, aconfig_declaration := range a.aaptProperties.Flags_packages {
ctx.AddDependency(ctx.Module(), aconfigDeclarationTag, aconfig_declaration)
@ -1374,6 +1374,12 @@ func (a *AARImport) ShouldSupportSdkVersion(ctx android.BaseModuleContext,
var _ android.PrebuiltInterface = (*AARImport)(nil)
func (a *AARImport) UsesLibrary() *usesLibrary {
return &a.usesLibrary
}
var _ ModuleWithUsesLibrary = (*AARImport)(nil)
// android_library_import imports an `.aar` file into the build graph as if it was built with android_library.
//
// This module is not suitable for installing on a device, but can be used as a `static_libs` dependency of

View file

@ -249,13 +249,13 @@ func (c Certificate) AndroidMkString() string {
}
func (a *AndroidApp) DepsMutator(ctx android.BottomUpMutatorContext) {
a.Module.deps(ctx)
if String(a.appProperties.Stl) == "c++_shared" && !a.SdkVersion(ctx).Specified() {
ctx.PropertyErrorf("stl", "sdk_version must be set in order to use c++_shared")
}
sdkDep := decodeSdkDep(ctx, android.SdkContext(a))
a.usesLibrary.deps(ctx, sdkDep.hasFrameworkLibs())
a.Module.deps(ctx)
if sdkDep.hasFrameworkLibs() {
a.aapt.deps(ctx, sdkDep)
}
@ -285,9 +285,6 @@ func (a *AndroidApp) DepsMutator(ctx android.BottomUpMutatorContext) {
}
ctx.AddFarVariationDependencies(variation, jniLibTag, a.appProperties.Jni_libs...)
}
a.usesLibrary.deps(ctx, sdkDep.hasFrameworkLibs())
for _, aconfig_declaration := range a.aaptProperties.Flags_packages {
ctx.AddDependency(ctx.Module(), aconfigDeclarationTag, aconfig_declaration)
}
@ -811,18 +808,10 @@ func (a *AndroidApp) generateAndroidBuildActions(ctx android.ModuleContext) {
// The decision to enforce <uses-library> checks is made before adding implicit SDK libraries.
a.usesLibrary.freezeEnforceUsesLibraries()
// Add implicit SDK libraries to <uses-library> list.
requiredUsesLibs, optionalUsesLibs := a.classLoaderContexts.UsesLibs()
for _, usesLib := range requiredUsesLibs {
a.usesLibrary.addLib(usesLib, false)
}
for _, usesLib := range optionalUsesLibs {
a.usesLibrary.addLib(usesLib, true)
}
// Check that the <uses-library> list is coherent with the manifest.
if a.usesLibrary.enforceUsesLibraries() {
manifestCheckFile := a.usesLibrary.verifyUsesLibrariesManifest(ctx, a.mergedManifestFile)
manifestCheckFile := a.usesLibrary.verifyUsesLibrariesManifest(
ctx, a.mergedManifestFile, &a.classLoaderContexts)
apkDeps = append(apkDeps, manifestCheckFile)
}
@ -1580,6 +1569,9 @@ type UsesLibraryProperties struct {
// provide the android.test.base statically and use jarjar to rename them so they do not collide
// with the classes provided by the android.test.base library.
Exclude_uses_libs []string
// The module names of optional uses-library libraries that are missing from the source tree.
Missing_optional_uses_libs []string `blueprint:"mutated"`
}
// usesLibrary provides properties and helper functions for AndroidApp and AndroidAppImport to verify that the
@ -1596,20 +1588,11 @@ type usesLibrary struct {
shouldDisableDexpreopt bool
}
func (u *usesLibrary) addLib(lib string, optional bool) {
if !android.InList(lib, u.usesLibraryProperties.Uses_libs) && !android.InList(lib, u.usesLibraryProperties.Optional_uses_libs) {
if optional {
u.usesLibraryProperties.Optional_uses_libs = append(u.usesLibraryProperties.Optional_uses_libs, lib)
} else {
u.usesLibraryProperties.Uses_libs = append(u.usesLibraryProperties.Uses_libs, lib)
}
}
}
func (u *usesLibrary) deps(ctx android.BottomUpMutatorContext, addCompatDeps bool) {
if !ctx.Config().UnbundledBuild() || ctx.Config().UnbundledBuildImage() {
ctx.AddVariationDependencies(nil, usesLibReqTag, u.usesLibraryProperties.Uses_libs...)
ctx.AddVariationDependencies(nil, usesLibOptTag, u.presentOptionalUsesLibs(ctx)...)
presentOptionalUsesLibs := u.presentOptionalUsesLibs(ctx)
ctx.AddVariationDependencies(nil, usesLibOptTag, presentOptionalUsesLibs...)
// Only add these extra dependencies if the module is an app that depends on framework
// libs. This avoids creating a cyclic dependency:
// e.g. framework-res -> org.apache.http.legacy -> ... -> framework-res.
@ -1620,6 +1603,8 @@ func (u *usesLibrary) deps(ctx android.BottomUpMutatorContext, addCompatDeps boo
ctx.AddVariationDependencies(nil, usesLibCompat28OptTag, dexpreopt.OptionalCompatUsesLibs28...)
ctx.AddVariationDependencies(nil, usesLibCompat30OptTag, dexpreopt.OptionalCompatUsesLibs30...)
}
_, diff, _ := android.ListSetDifference(u.usesLibraryProperties.Optional_uses_libs, presentOptionalUsesLibs)
u.usesLibraryProperties.Missing_optional_uses_libs = diff
} else {
ctx.AddVariationDependencies(nil, r8LibraryJarTag, u.usesLibraryProperties.Uses_libs...)
ctx.AddVariationDependencies(nil, r8LibraryJarTag, u.presentOptionalUsesLibs(ctx)...)
@ -1638,15 +1623,6 @@ func (u *usesLibrary) presentOptionalUsesLibs(ctx android.BaseModuleContext) []s
return optionalUsesLibs
}
// Helper function to replace string in a list.
func replaceInList(list []string, oldstr, newstr string) {
for i, str := range list {
if str == oldstr {
list[i] = newstr
}
}
}
// Returns a map of module names of shared library dependencies to the paths to their dex jars on
// host and on device.
func (u *usesLibrary) classLoaderContextForUsesLibDeps(ctx android.ModuleContext) dexpreopt.ClassLoaderContextMap {
@ -1688,11 +1664,6 @@ func (u *usesLibrary) classLoaderContextForUsesLibDeps(ctx android.ModuleContext
libName := dep
if ulib, ok := m.(ProvidesUsesLib); ok && ulib.ProvidesUsesLib() != nil {
libName = *ulib.ProvidesUsesLib()
// Replace module name with library name in `uses_libs`/`optional_uses_libs` in
// order to pass verify_uses_libraries check (which compares these properties
// against library names written in the manifest).
replaceInList(u.usesLibraryProperties.Uses_libs, dep, libName)
replaceInList(u.usesLibraryProperties.Optional_uses_libs, dep, libName)
}
clcMap.AddContext(ctx, tag.sdkVersion, libName, tag.optional,
lib.DexJarBuildPath(ctx).PathOrNil(), lib.DexJarInstallPath(),
@ -1726,7 +1697,7 @@ func (u *usesLibrary) freezeEnforceUsesLibraries() {
// an APK with the manifest embedded in it (manifest_check will know which one it is by the file
// extension: APKs are supposed to end with '.apk').
func (u *usesLibrary) verifyUsesLibraries(ctx android.ModuleContext, inputFile android.Path,
outputFile android.WritablePath) android.Path {
outputFile android.WritablePath, classLoaderContexts *dexpreopt.ClassLoaderContextMap) android.Path {
statusFile := dexpreopt.UsesLibrariesStatusFile(ctx)
@ -1754,27 +1725,37 @@ func (u *usesLibrary) verifyUsesLibraries(ctx android.ModuleContext, inputFile a
cmd.Flag("--enforce-uses-libraries-relax")
}
for _, lib := range u.usesLibraryProperties.Uses_libs {
requiredUsesLibs, optionalUsesLibs := classLoaderContexts.UsesLibs()
for _, lib := range requiredUsesLibs {
cmd.FlagWithArg("--uses-library ", lib)
}
for _, lib := range u.usesLibraryProperties.Optional_uses_libs {
for _, lib := range optionalUsesLibs {
cmd.FlagWithArg("--optional-uses-library ", lib)
}
// Also add missing optional uses libs, as the manifest check expects them.
// Note that what we add here are the module names of those missing libs, not library names, while
// the manifest check actually expects library names. However, the case where a library is missing
// and the module name != the library name is too rare for us to handle.
for _, lib := range u.usesLibraryProperties.Missing_optional_uses_libs {
cmd.FlagWithArg("--missing-optional-uses-library ", lib)
}
rule.Build("verify_uses_libraries", "verify <uses-library>")
return outputFile
}
// verifyUsesLibrariesManifest checks the <uses-library> tags in an AndroidManifest.xml against
// the build system and returns the path to a copy of the manifest.
func (u *usesLibrary) verifyUsesLibrariesManifest(ctx android.ModuleContext, manifest android.Path) android.Path {
func (u *usesLibrary) verifyUsesLibrariesManifest(ctx android.ModuleContext, manifest android.Path,
classLoaderContexts *dexpreopt.ClassLoaderContextMap) android.Path {
outputFile := android.PathForModuleOut(ctx, "manifest_check", "AndroidManifest.xml")
return u.verifyUsesLibraries(ctx, manifest, outputFile)
return u.verifyUsesLibraries(ctx, manifest, outputFile, classLoaderContexts)
}
// verifyUsesLibrariesAPK checks the <uses-library> tags in the manifest of an APK against the build
// system and returns the path to a copy of the APK.
func (u *usesLibrary) verifyUsesLibrariesAPK(ctx android.ModuleContext, apk android.Path) {
u.verifyUsesLibraries(ctx, apk, nil) // for APKs manifest_check does not write output file
func (u *usesLibrary) verifyUsesLibrariesAPK(ctx android.ModuleContext, apk android.Path,
classLoaderContexts *dexpreopt.ClassLoaderContextMap) {
u.verifyUsesLibraries(ctx, apk, nil, classLoaderContexts) // for APKs manifest_check does not write output file
}

View file

@ -355,7 +355,7 @@ func (a *AndroidAppImport) generateAndroidBuildActions(ctx android.ModuleContext
}
if a.usesLibrary.enforceUsesLibraries() {
a.usesLibrary.verifyUsesLibrariesAPK(ctx, srcApk)
a.usesLibrary.verifyUsesLibrariesAPK(ctx, srcApk, &a.dexpreopter.classLoaderContexts)
}
a.dexpreopter.dexpreopt(ctx, android.RemoveOptionalPrebuiltPrefix(ctx.ModuleName()), jnisUncompressed)
@ -611,6 +611,12 @@ func createArchDpiVariantGroupType(archNames []string, dpiNames []string) reflec
return return_struct
}
func (a *AndroidAppImport) UsesLibrary() *usesLibrary {
return &a.usesLibrary
}
var _ ModuleWithUsesLibrary = (*AndroidAppImport)(nil)
// android_app_import imports a prebuilt apk with additional processing specified in the module.
// DPI-specific apk source files can be specified using dpi_variants. Example:
//

View file

@ -3280,7 +3280,7 @@ func TestUsesLibraries(t *testing.T) {
sdk_version: "current",
optional_uses_libs: [
"bar",
"baz",
"missing-lib-b",
],
}
@ -3295,7 +3295,7 @@ func TestUsesLibraries(t *testing.T) {
],
optional_uses_libs: [
"bar",
"baz",
"missing-lib-b",
],
}
`
@ -3317,10 +3317,10 @@ func TestUsesLibraries(t *testing.T) {
// propagated from dependencies.
actualManifestFixerArgs := app.Output("manifest_fixer/AndroidManifest.xml").Args["args"]
expectManifestFixerArgs := `--extract-native-libs=true ` +
`--uses-library qux ` +
`--uses-library quuz ` +
`--uses-library foo ` +
`--uses-library com.non.sdk.lib ` +
`--uses-library qux ` +
`--uses-library quuz ` +
`--uses-library runtime-library ` +
`--uses-library runtime-required-x ` +
`--uses-library runtime-required-y ` +
@ -3339,9 +3339,9 @@ func TestUsesLibraries(t *testing.T) {
`--uses-library runtime-required-x ` +
`--uses-library runtime-required-y ` +
`--optional-uses-library bar ` +
`--optional-uses-library baz ` +
`--optional-uses-library runtime-optional-x ` +
`--optional-uses-library runtime-optional-y `
`--optional-uses-library runtime-optional-y ` +
`--missing-optional-uses-library missing-lib-b `
android.AssertStringDoesContain(t, "verify cmd args", verifyCmd, verifyArgs)
// Test that all libraries are verified for an APK (library order matters).
@ -3350,7 +3350,7 @@ func TestUsesLibraries(t *testing.T) {
`--uses-library com.non.sdk.lib ` +
`--uses-library android.test.runner ` +
`--optional-uses-library bar ` +
`--optional-uses-library baz `
`--missing-optional-uses-library missing-lib-b `
android.AssertStringDoesContain(t, "verify apk cmd args", verifyApkCmd, verifyApkArgs)
// Test that necessary args are passed for constructing CLC in Ninja phase.

View file

@ -837,9 +837,11 @@ func (j *Module) deps(ctx android.BottomUpMutatorContext) {
if dep != nil {
if component, ok := dep.(SdkLibraryComponentDependency); ok {
if lib := component.OptionalSdkLibraryImplementation(); lib != nil {
// Add library as optional if it's one of the optional compatibility libs.
// Add library as optional if it's one of the optional compatibility libs or it's
// explicitly listed in the optional_uses_libs property.
tag := usesLibReqTag
if android.InList(*lib, dexpreopt.OptionalCompatUsesLibs) {
if android.InList(*lib, dexpreopt.OptionalCompatUsesLibs) ||
android.InList(*lib, j.usesLibrary.usesLibraryProperties.Optional_uses_libs) {
tag = usesLibOptTag
}
ctx.AddVariationDependencies(nil, tag, *lib)
@ -2706,3 +2708,13 @@ type ModuleWithStem interface {
}
var _ ModuleWithStem = (*Module)(nil)
type ModuleWithUsesLibrary interface {
UsesLibrary() *usesLibrary
}
func (j *Module) UsesLibrary() *usesLibrary {
return &j.usesLibrary
}
var _ ModuleWithUsesLibrary = (*Module)(nil)

View file

@ -960,8 +960,8 @@ func (j *Library) GenerateAndroidBuildActions(ctx android.ModuleContext) {
}
func (j *Library) DepsMutator(ctx android.BottomUpMutatorContext) {
j.deps(ctx)
j.usesLibrary.deps(ctx, false)
j.deps(ctx)
}
const (
@ -3138,7 +3138,13 @@ func addCLCFromDep(ctx android.ModuleContext, depModule android.Module,
// <uses_library> and should not be added to CLC, but the transitive <uses-library> dependencies
// from its CLC should be added to the current CLC.
if sdkLib != nil {
clcMap.AddContext(ctx, dexpreopt.AnySdkVersion, *sdkLib, false,
optional := false
if module, ok := ctx.Module().(ModuleWithUsesLibrary); ok {
if android.InList(*sdkLib, module.UsesLibrary().usesLibraryProperties.Optional_uses_libs) {
optional = true
}
}
clcMap.AddContext(ctx, dexpreopt.AnySdkVersion, *sdkLib, optional,
dep.DexJarBuildPath(ctx).PathOrNil(), dep.DexJarInstallPath(), dep.ClassLoaderContexts())
} else {
clcMap.AddContextMap(dep.ClassLoaderContexts(), depName)

View file

@ -51,6 +51,14 @@ def parse_args():
help='specify uses-library entries known to the build system with '
'required:false'
)
parser.add_argument(
'--missing-optional-uses-library',
dest='missing_optional_uses_libraries',
action='append',
help='specify uses-library entries missing from the build system with '
'required:false',
default=[]
)
parser.add_argument(
'--enforce-uses-libraries',
dest='enforce_uses_libraries',
@ -91,7 +99,7 @@ C_OFF = "\033[0m"
C_BOLD = "\033[1m"
def enforce_uses_libraries(manifest, required, optional, relax, is_apk, path):
def enforce_uses_libraries(manifest, required, optional, missing_optional, relax, is_apk, path):
"""Verify that the <uses-library> tags in the manifest match those provided
by the build system.
@ -119,7 +127,12 @@ def enforce_uses_libraries(manifest, required, optional, relax, is_apk, path):
required = trim_namespace_parts(required)
optional = trim_namespace_parts(optional)
if manifest_required == required and manifest_optional == optional:
existing_manifest_optional = [
lib for lib in manifest_optional if lib not in missing_optional]
# The order of the existing libraries matter, while the order of the missing
# ones doesn't.
if manifest_required == required and existing_manifest_optional == optional:
return None
#pylint: disable=line-too-long
@ -129,6 +142,7 @@ def enforce_uses_libraries(manifest, required, optional, relax, is_apk, path):
'\t- required libraries in build system: %s[%s]%s\n' % (C_RED, ', '.join(required), C_OFF),
'\t vs. in the manifest: %s[%s]%s\n' % (C_RED, ', '.join(manifest_required), C_OFF),
'\t- optional libraries in build system: %s[%s]%s\n' % (C_RED, ', '.join(optional), C_OFF),
'\t and missing ones in build system: %s[%s]%s\n' % (C_RED, ', '.join(missing_optional), C_OFF),
'\t vs. in the manifest: %s[%s]%s\n' % (C_RED, ', '.join(manifest_optional), C_OFF),
'\t- tags in the manifest (%s):\n' % path,
'\t\t%s\n' % '\t\t'.join(tags),
@ -340,11 +354,14 @@ def main():
if args.enforce_uses_libraries:
# Load dexpreopt.config files and build a mapping from module
# names to library names. This is necessary because build system
# addresses libraries by their module name (`uses_libs`,
# `optional_uses_libs`, `LOCAL_USES_LIBRARIES`,
# `LOCAL_OPTIONAL_LIBRARY_NAMES` all contain module names), while
# the manifest addresses libraries by their name.
# names to library names. This is for Make only and it's necessary
# because Make passes module names from `LOCAL_USES_LIBRARIES`,
# `LOCAL_OPTIONAL_LIBRARY_NAMES`, while the manifest addresses
# libraries by their name. Soong doesn't use it and doesn't need it
# because it converts the module names to the library names and
# passes the library names. There is no need to translate missing
# optional libs because they are missing and therefore there is no
# mapping for them.
mod_to_lib = load_dexpreopt_configs(args.dexpreopt_configs)
required = translate_libnames(args.uses_libraries, mod_to_lib)
optional = translate_libnames(args.optional_uses_libraries,
@ -354,8 +371,8 @@ def main():
# those in the manifest. Raise an exception on mismatch, unless the
# script was passed a special parameter to suppress exceptions.
errmsg = enforce_uses_libraries(manifest, required, optional,
args.enforce_uses_libraries_relax,
is_apk, args.input)
args.missing_optional_uses_libraries,
args.enforce_uses_libraries_relax, is_apk, args.input)
# Create a status file that is empty on success, or contains an
# error message on failure. When exceptions are suppressed,

View file

@ -44,15 +44,17 @@ def required_apk(value):
class EnforceUsesLibrariesTest(unittest.TestCase):
"""Unit tests for add_extract_native_libs function."""
def run_test(self, xml, apk, uses_libraries=[], optional_uses_libraries=[]): #pylint: disable=dangerous-default-value
def run_test(self, xml, apk, uses_libraries=[], optional_uses_libraries=[],
missing_optional_uses_libraries=[]): #pylint: disable=dangerous-default-value
doc = minidom.parseString(xml)
try:
relax = False
manifest_check.enforce_uses_libraries(
doc, uses_libraries, optional_uses_libraries, relax, False,
'path/to/X/AndroidManifest.xml')
doc, uses_libraries, optional_uses_libraries, missing_optional_uses_libraries,
relax, False, 'path/to/X/AndroidManifest.xml')
manifest_check.enforce_uses_libraries(apk, uses_libraries,
optional_uses_libraries,
missing_optional_uses_libraries,
relax, True,
'path/to/X/X.apk')
return True
@ -102,6 +104,15 @@ class EnforceUsesLibrariesTest(unittest.TestCase):
matches = self.run_test(xml, apk, optional_uses_libraries=['foo'])
self.assertFalse(matches)
def test_expected_missing_optional_uses_library(self):
xml = self.xml_tmpl % (
uses_library_xml('foo') + uses_library_xml('missing') + uses_library_xml('bar'))
apk = self.apk_tmpl % (
uses_library_apk('foo') + uses_library_apk('missing') + uses_library_apk('bar'))
matches = self.run_test(xml, apk, optional_uses_libraries=['foo', 'bar'],
missing_optional_uses_libraries=['missing'])
self.assertFalse(matches)
def test_missing_uses_library(self):
xml = self.xml_tmpl % ('')
apk = self.apk_tmpl % ('')