Update resource visibility validation to only analyze dependencies with private resources This change introduces a new field to StarlarkAndroidResourcesInfo, transitive_compiled_resources_with_public_xml, which gathers compiled resources only from transitive dependencies that include a public.xml file. This reduces the number of symbols.zip files that need to be loaded and analyzed during the resource validation phase, vastly improving build performance. ValidateAndLinkResourcesAction and Aapt2ResourcePackagingAction are updated to use this new set of dependencies passed via the --compiledDepWithPublicXml flag when performing resource reference visibility checks. If this flag is provided, only symbols from these dependencies are loaded for visibility checks. Otherwise, it defaults to checking against all transitive dependencies. PiperOrigin-RevId: 952334790 Change-Id: I5a5e2e0d7f4fd135414f88e7c0ef6d240c468c06
diff --git a/providers/providers.bzl b/providers/providers.bzl index be82377..6f09790 100644 --- a/providers/providers.bzl +++ b/providers/providers.bzl
@@ -73,6 +73,13 @@ ), ) +# Temporary constructor to handle version skew with release rules. +# Remove when release rules populate 'transitive_compiled_resources_with_public_xml'. +def _starlark_android_resources_info_init(**kwargs): + if "transitive_compiled_resources_with_public_xml" not in kwargs: + kwargs["transitive_compiled_resources_with_public_xml"] = depset() + return kwargs + StarlarkAndroidResourcesInfo = provider( doc = "Provides information about direct and transitive resources", fields = dict( @@ -83,6 +90,7 @@ transitive_compiled_assets = "Depset of transitive compiled assets", direct_compiled_resources = "Depset of direct compiled_resources, can contain multiple files due to exports", transitive_compiled_resources = "Depset of transitive compiled resources", + transitive_compiled_resources_with_public_xml = "Depset of transitive compiled resources from targets containing explicit public.xml declarations", transitive_manifests = "Depset of transitive manifests", transitive_r_txts = "Depset of transitive R.txt files", transitive_resource_files = "Depset of transitive resource files", @@ -90,7 +98,8 @@ transitive_resource_apks = "Depset of transitive resource only apk files", package = "String, the package used for the generated Java resources", ), -) + init = _starlark_android_resources_info_init, +)[0] AndroidLintRulesInfo = provider( doc = "Provides extra lint rules to use with AndroidLint.",
diff --git a/rules/busybox.bzl b/rules/busybox.bzl index 13b7919..b6ba9b5 100644 --- a/rules/busybox.bzl +++ b/rules/busybox.bzl
@@ -229,6 +229,7 @@ transitive_compiled_assets = [], transitive_resource_files = [], transitive_compiled_resources = [], + transitive_compiled_resources_with_public_xml = [], transitive_r_txts = [], additional_apks_to_link_against = [], resource_apks = depset(), @@ -286,6 +287,8 @@ resource files. transitive_compiled_resources: List of Depsets. Depsets contain all transitive compiled_resources. + transitive_compiled_resources_with_public_xml: List of Depsets. Depsets contain + all transitive compiled_resources from targets containing explicit public.xml declarations. transitive_r_txts: List of Depsets. Depsets contain all transitive R txt files. additional_apks_to_link_against: A list of Files. Additional APKs to link against. Optional. @@ -350,6 +353,13 @@ transitive_input_files.extend(transitive_assets) transitive_input_files.extend(transitive_compiled_assets) transitive_input_files.extend(transitive_compiled_resources) + if transitive_compiled_resources_with_public_xml: + compiled_dep_with_public_xml = depset(transitive = transitive_compiled_resources_with_public_xml) + args.add_joined( + "--compiledDepWithPublicXml", + compiled_dep_with_public_xml, + join_with = ":", + ) transitive_input_files.extend(transitive_manifests) transitive_input_files.extend(transitive_r_txts) args.add( @@ -591,6 +601,7 @@ out_file = None, compiled_resources = None, transitive_compiled_resources = depset(), + transitive_compiled_resources_with_public_xml = depset(), java_package = None, manifest = None, feature_flags = "", @@ -610,6 +621,8 @@ this target. transitive_compiled_resources: Depset of Files. The symbols.zip of the compiled resources from the transitive dependencies of this target. + transitive_compiled_resources_with_public_xml: Depset of Files. The symbols.zip of the + compiled resources from transitive dependencies that declare public.xml. java_package: A string. The Java package for the generated R.java. manifest: A File. The AndroidManifest.xml. resource_apks: List of direct resource only apk files. @@ -639,6 +652,12 @@ join_with = ":", ) transitive_input_files.append(transitive_compiled_resources) + if transitive_compiled_resources_with_public_xml: + args.add_joined( + "--compiledDepWithPublicXml", + transitive_compiled_resources_with_public_xml, + join_with = ":", + ) args.add("--manifest", manifest) input_files.append(manifest) if java_package:
diff --git a/rules/resources.bzl b/rules/resources.bzl index fa4e37a..6351866 100644 --- a/rules/resources.bzl +++ b/rules/resources.bzl
@@ -614,6 +614,7 @@ transitive_compiled_assets = [] transitive_resource_files = [] transitive_compiled_resources = [] + transitive_compiled_resources_with_public_xml = [] transitive_manifests = [] transitive_r_txts = [] packages_to_r_txts_depset = dict() @@ -627,6 +628,7 @@ transitive_compiled_assets.append(dep.transitive_compiled_assets) transitive_resource_files.append(dep.transitive_resource_files) transitive_compiled_resources.append(dep.transitive_compiled_resources) + transitive_compiled_resources_with_public_xml.append(dep.transitive_compiled_resources_with_public_xml) transitive_manifests.append(dep.transitive_manifests) transitive_r_txts.append(dep.transitive_r_txts) for pkg, r_txts in dep.packages_to_r_txts.items(): @@ -766,6 +768,7 @@ transitive_compiled_assets = transitive_compiled_assets, transitive_resource_files = transitive_resource_files, transitive_compiled_resources = transitive_compiled_resources, + transitive_compiled_resources_with_public_xml = transitive_compiled_resources_with_public_xml, transitive_manifests = transitive_manifests, transitive_r_txts = transitive_r_txts, resource_configs = resource_configs, @@ -1285,6 +1288,7 @@ transitive_compiled_assets = [] direct_compiled_resources = [] transitive_compiled_resources = [] + transitive_compiled_resources_with_public_xml = [] transitive_resources_files = [] transitive_manifests = [] transitive_r_txts = [] @@ -1299,6 +1303,7 @@ transitive_compiled_assets.append(dep.transitive_compiled_assets) direct_compiled_resources.append(dep.direct_compiled_resources) transitive_compiled_resources.append(dep.transitive_compiled_resources) + transitive_compiled_resources_with_public_xml.append(dep.transitive_compiled_resources_with_public_xml) transitive_resources_files.append(dep.transitive_resource_files) transitive_manifests.append(dep.transitive_manifests) transitive_r_txts.append(dep.transitive_r_txts) @@ -1312,6 +1317,7 @@ exports_transitive_compiled_assets = [] exports_direct_compiled_resources = [] exports_transitive_compiled_resources = [] + exports_transitive_compiled_resources_with_public_xml = [] exports_transitive_resources_files = [] exports_transitive_manifests = [] exports_transitive_r_txts = [] @@ -1323,6 +1329,7 @@ exports_transitive_compiled_assets.append(dep.transitive_compiled_assets) exports_direct_compiled_resources.append(dep.direct_compiled_resources) exports_transitive_compiled_resources.append(dep.transitive_compiled_resources) + exports_transitive_compiled_resources_with_public_xml.append(dep.transitive_compiled_resources_with_public_xml) exports_transitive_resources_files.append(dep.transitive_resource_files) exports_transitive_manifests.append(dep.transitive_manifests) exports_transitive_r_txts.append(dep.transitive_r_txts) @@ -1337,6 +1344,7 @@ transitive_compiled_assets.extend(exports_transitive_compiled_assets) direct_compiled_resources.extend(exports_direct_compiled_resources) transitive_compiled_resources.extend(exports_transitive_compiled_resources) + transitive_compiled_resources_with_public_xml.extend(exports_transitive_compiled_resources_with_public_xml) transitive_resources_files.extend(exports_transitive_resources_files) transitive_manifests.extend(exports_transitive_manifests) transitive_r_txts.extend(exports_transitive_r_txts) @@ -1383,6 +1391,7 @@ transitive_compiled_assets = transitive_compiled_assets, transitive_resource_files = transitive_resources_files, transitive_compiled_resources = transitive_compiled_resources, + transitive_compiled_resources_with_public_xml = transitive_compiled_resources_with_public_xml, transitive_manifests = transitive_manifests, transitive_r_txts = transitive_r_txts, feature_flags = feature_flags, @@ -1562,6 +1571,10 @@ transitive = transitive_compiled_resources, order = "preorder", ), + transitive_compiled_resources_with_public_xml = depset( + transitive = transitive_compiled_resources_with_public_xml, + order = "preorder", + ), java_package = java_package, manifest = processed_manifest, feature_flags = feature_flags, @@ -1615,6 +1628,8 @@ for pkg, depsets in packages_to_r_txts_depset.items(): packages_to_r_txts[pkg] = depset(transitive = depsets) + has_public_xml = any([f.basename == "public.xml" for f in processed_resources]) if processed_resources else False + # TODO(b/159916013): Audit neverlink behavior. Some processing can likely be skipped if the target is neverlink. # TODO(b/69668042): Don't propagate exported providers/artifacts. Exports should respect neverlink. if resources_neverlink: @@ -1651,6 +1666,10 @@ transitive = exports_transitive_compiled_resources, order = "preorder", ), + transitive_compiled_resources_with_public_xml = depset( + transitive = exports_transitive_compiled_resources_with_public_xml, + order = "preorder", + ), transitive_manifests = depset( [processed_manifest] if processed_manifest else [], transitive = exports_transitive_manifests, @@ -1723,6 +1742,11 @@ transitive = transitive_compiled_resources + exports_transitive_compiled_resources, order = "preorder", ), + transitive_compiled_resources_with_public_xml = depset( + [compiled_resources] if (compiled_resources and has_public_xml) else [], + transitive = transitive_compiled_resources_with_public_xml + exports_transitive_compiled_resources_with_public_xml, + order = "preorder", + ), transitive_manifests = depset( [processed_manifest] if processed_manifest else [], transitive = transitive_manifests + exports_transitive_manifests,
diff --git a/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java b/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java index 3a52308..982e0f7 100644 --- a/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java +++ b/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
@@ -30,6 +30,7 @@ import com.google.devtools.build.android.Converters.ColonSplitter; import com.google.devtools.build.android.Converters.CompatDependencyAndroidDataConverter; import com.google.devtools.build.android.Converters.CompatPathConverter; +import com.google.devtools.build.android.Converters.CompatPathListConverter; import com.google.devtools.build.android.Converters.CompatSerializedAndroidDataConverter; import com.google.devtools.build.android.Converters.CompatUnvalidatedAndroidDataConverter; import com.google.devtools.build.android.aapt2.Aapt2ConfigOptions; @@ -132,6 +133,12 @@ public List<Path> additionalApksToLinkAgainst = ImmutableList.of(); @Parameter( + names = "--compiledDepWithPublicXml", + listConverter = CompatPathListConverter.class, + description = "Compiled resource dependencies containing explicit public.xml declarations.") + public List<Path> compiledDepsWithPublicXml = ImmutableList.of(); + + @Parameter( names = "--packageId", description = "Resource ID prefix; see AAPT2 documentation for --package-id.") public int packageId = -1; @@ -426,8 +433,14 @@ manifestReferences = XmlUtils.getAllResourceReferences(compiled.getManifest()); } + ImmutableList<CompiledResources> visibilityDeps = + !options.compiledDepsWithPublicXml.isEmpty() + ? options.compiledDepsWithPublicXml.stream() + .map(CompiledResources::from) + .collect(toImmutableList()) + : ImmutableList.copyOf(compiledResourceDeps); ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - manifestReferences, compiled, compiledResourceDeps); + manifestReferences, compiled, visibilityDeps); profiler.recordEndOf("validate");
diff --git a/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java b/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java index 90232fb..4923545 100644 --- a/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java +++ b/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java
@@ -58,6 +58,12 @@ description = "Compiled resource dependencies to link.") public List<Path> compiledDeps = ImmutableList.of(); + @Parameter( + names = "--compiledDepWithPublicXml", + listConverter = Converters.CompatPathListConverter.class, + description = "Compiled resource dependencies containing explicit public.xml declarations.") + public List<Path> compiledDepsWithPublicXml = ImmutableList.of(); + /** * TODO(b/64570523): Still used by blaze. Will be removed as part of the command line cleanup. * @@ -151,12 +157,18 @@ options.packageForR)); ImmutableList<CompiledResources> includes = options.compiledDeps.stream().map(CompiledResources::from).collect(toImmutableList()); + ImmutableList<CompiledResources> visibilityIncludes = + !options.compiledDepsWithPublicXml.isEmpty() + ? options.compiledDepsWithPublicXml.stream() + .map(CompiledResources::from) + .collect(toImmutableList()) + : includes; profiler.recordEndOf("manifest").startTask("validate"); // TODO(b/146663858): distinguish direct/transitive deps for "strict deps". // TODO(b/128711690): validate AndroidManifest.xml checkVisibilityOfResourceReferences( - /* manifestReferences= */ ImmutableList.of(), resources, includes); + /* manifestReferences= */ ImmutableList.of(), resources, visibilityIncludes); ImmutableList<StaticLibrary> resourceApks = ImmutableList.of(); if (options.resourceApks != null) {
diff --git a/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java b/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java index 60d37b1..8b66c67 100644 --- a/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java +++ b/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java
@@ -234,4 +234,48 @@ + tempDir.resolve("lib/res/values/values.xml") + ") references external private resources [string/private_string]"); } + + @Test + public void testCompiledDepWithPublicXml_validation() throws Exception { + Map<String, String> depFiles = new HashMap<>(); + depFiles.put( + "values/public.xml", + "<resources><public name=\"public_string\" type=\"string\"/></resources>"); + depFiles.put( + "values/strings.xml", + "<resources><string name=\"private_string\">hello</string></resources>"); + + CompiledResources dep = + createCompiledResources("dep", depFiles, "<manifest package=\"com.dep\"/>"); + + Map<String, String> libFiles = new HashMap<>(); + libFiles.put( + "values/values.xml", + "<resources><string name=\"lib_string\">@string/private_string</string></resources>"); + + CompiledResources lib = + createCompiledResources("lib", libFiles, "<manifest package=\"com.lib\"/>"); + + Path outLib = tempDir.resolve("lib.apk"); + Path outSrcJar = tempDir.resolve("r.srcjar"); + Path outRTxt = tempDir.resolve("R.txt"); + + String[] args = + new String[] { + "--aapt2", + aapt2.toString(), + "--resources", + lib.getZip().toString() + ":" + lib.getManifest().toString(), + "--compiledDepWithPublicXml", + dep.getZip().toString(), + "--staticLibraryOut", + outLib.toString(), + "--sourceJarOut", + outSrcJar.toString(), + "--rTxtOut", + outRTxt.toString() + }; + + assertThrows(UserException.class, () -> ValidateAndLinkResourcesAction.main(args)); + } }
diff --git a/test/rules/resources/BUILD b/test/rules/resources/BUILD index df9e1aa..ed0e5d2 100644 --- a/test/rules/resources/BUILD +++ b/test/rules/resources/BUILD
@@ -82,6 +82,38 @@ ) starlark_process( + name = "resource_processing_with_public_xml", + custom_package = "test.rules.resources", + manifest = "AndroidManifest.xml", + resource_files = glob(["res_public_test/**"]), +) + +resource_processing_with_public_xml_node = ExpectedResourcesNodeInfo( + assets_symbols = "resource_processing_with_public_xml_symbols/assets.bin", + compiled_resources = "resource_processing_with_public_xml_symbols/symbols.zip", + label = ":resource_processing_with_public_xml", + manifest = "resource_processing_with_public_xml_processed_manifest/AndroidManifest.xml", + r_txt = "resource_processing_with_public_xml_symbols/R.aapt2.txt", +) + +resource_processing_with_public_xml_info = ExpectedStarlarkAndroidResourcesInfo( + direct_resources_nodes = [resource_processing_with_public_xml_node], + transitive_assets_symbols = ["resource_processing_with_public_xml_symbols/assets.bin"], + transitive_compiled_resources = ["resource_processing_with_public_xml_symbols/symbols.zip"], + transitive_compiled_resources_with_public_xml = ["resource_processing_with_public_xml_symbols/symbols.zip"], + transitive_resources_nodes = [], +) + +starlark_process_test( + name = "test_resource_processing_with_public_xml", + expected_r_class_fields = [ + "string.public_hello", + ], + expected_starlark_android_resources_info = resource_processing_with_public_xml_info, + target_under_test = ":resource_processing_with_public_xml", +) + +starlark_process( name = "resource_processing_without_stamping_manifest", assets = ["assets/some_asset.txt"], assets_dir = "assets",
diff --git a/test/rules/resources/res_public_test/res/values/public.xml b/test/rules/resources/res_public_test/res/values/public.xml new file mode 100644 index 0000000..92a3cc6 --- /dev/null +++ b/test/rules/resources/res_public_test/res/values/public.xml
@@ -0,0 +1,4 @@ +<?xml version="1.0" encoding="utf-8"?> +<resources> + <public name="public_hello" type="string" /> +</resources>
diff --git a/test/rules/resources/res_public_test/res/values/strings.xml b/test/rules/resources/res_public_test/res/values/strings.xml new file mode 100644 index 0000000..1d1c3c1 --- /dev/null +++ b/test/rules/resources/res_public_test/res/values/strings.xml
@@ -0,0 +1,5 @@ +<?xml version="1.0" encoding="utf-8"?> +<resources> + <string name="public_hello">Hello</string> + <string name="private_world">World</string> +</resources>
diff --git a/test/utils/asserts.bzl b/test/utils/asserts.bzl index c0f93a5..9bd0de5 100644 --- a/test/utils/asserts.bzl +++ b/test/utils/asserts.bzl
@@ -101,6 +101,7 @@ transitive_assets = ctx.attr.transitive_assets, transitive_assets_symbols = ctx.attr.transitive_assets_symbols, transitive_compiled_resources = ctx.attr.transitive_compiled_resources, + transitive_compiled_resources_with_public_xml = ctx.attr.transitive_compiled_resources_with_public_xml, packages_to_r_txts = ctx.attr.packages_to_r_txts, ), ] @@ -117,6 +118,7 @@ transitive_assets = attr.string_list(), transitive_assets_symbols = attr.string_list(), transitive_compiled_resources = attr.string_list(), + transitive_compiled_resources_with_public_xml = attr.string_list(), packages_to_r_txts = attr.string_list_dict(), ), ) @@ -127,10 +129,11 @@ transitive_assets = [], transitive_assets_symbols = [], transitive_compiled_resources = [], + transitive_compiled_resources_with_public_xml = [], packages_to_r_txts = {}, name = "unused"): # appease linter name = (str(direct_resources_nodes) + str(transitive_resources_nodes) + str(transitive_assets) + - str(transitive_assets_symbols) + str(transitive_compiled_resources)) + str(transitive_assets_symbols) + str(transitive_compiled_resources) + str(transitive_compiled_resources_with_public_xml)) name = ":_data_" + str(hash(name)) # Allow multiple tests to share the same expected info by checking if rule exists @@ -142,6 +145,7 @@ transitive_assets = transitive_assets, transitive_assets_symbols = transitive_assets_symbols, transitive_compiled_resources = transitive_compiled_resources, + transitive_compiled_resources_with_public_xml = transitive_compiled_resources_with_public_xml, packages_to_r_txts = packages_to_r_txts, ) return name @@ -513,6 +517,12 @@ "StarlarkAndroidResourcesInfo.transitive_compiled_resources", ignore_label_prefix, ) + _assert_file_depset( + expected.transitive_compiled_resources_with_public_xml, + actual.transitive_compiled_resources_with_public_xml, + "StarlarkAndroidResourcesInfo.transitive_compiled_resources_with_public_xml", + ignore_label_prefix, + ) for pkg, value in expected.packages_to_r_txts.items(): if pkg in actual.packages_to_r_txts: _assert_file_depset(