From 7c584d1390ed5e9c5d47984aad2858a7c78a99e0 Mon Sep 17 00:00:00 2001 From: Benjamin Lee Date: Thu, 6 May 2021 17:00:45 -0700 Subject: [PATCH] Support uses-permission merging in manifest merger --- .../rules/android/AndroidConfiguration.java | 15 +++ .../lib/rules/android/AndroidManifest.java | 2 + .../android/ManifestMergerActionBuilder.java | 10 ++ .../android/ManifestMergerActionTest.java | 78 +++++++++++-- .../build/android/testing/manifestmerge/BUILD | 1 + .../AndroidManifest.xml | 107 ++++++++++++++++++ .../build/android/ManifestMergerAction.java | 21 +++- 7 files changed, 224 insertions(+), 10 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java index 4e61531d0cc201..09f7347a0ed5c1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java @@ -980,6 +980,15 @@ public static class Options extends FragmentOptions { + " transition` with changed options to avoid potential action conflicts.") public boolean androidPlatformsTransitionsUpdateAffected; + @Option( + name = "merge_android_manifest_permissions", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + help = "If enabled, the manifest merger will merge uses-permission and " + + "uses-permission-sdk-23 attributes.") + public boolean mergeAndroidManifestPermissions; + @Override public FragmentOptions getHost() { Options host = (Options) super.getHost(); @@ -1065,6 +1074,7 @@ public FragmentOptions getHost() { private final boolean hwasan; private final boolean getJavaResourcesFromOptimizedJar; private final boolean includeProguardLocationReferences; + private final boolean mergeAndroidManifestPermissions; public AndroidConfiguration(BuildOptions buildOptions) throws InvalidConfigurationException { Options options = buildOptions.get(Options.class); @@ -1126,6 +1136,7 @@ public AndroidConfiguration(BuildOptions buildOptions) throws InvalidConfigurati this.hwasan = options.hwasan; this.getJavaResourcesFromOptimizedJar = options.getJavaResourcesFromOptimizedJar; this.includeProguardLocationReferences = options.includeProguardLocationReferences; + this.mergeAndroidManifestPermissions = options.mergeAndroidManifestPermissions; if (incrementalDexingShardsAfterProguard < 0) { throw new InvalidConfigurationException( @@ -1411,6 +1422,10 @@ public boolean includeProguardLocationReferences() { return includeProguardLocationReferences; } + public boolean mergeAndroidManifestPermissions() { + return mergeAndroidManifestPermissions; + } + /** Returns the label provided with --legacy_main_dex_list_generator, if any. */ // TODO(b/147692286): Move R8's main dex list tool into tool repository. @StarlarkConfigurationField( diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java index df9d95b56760e7..e77a21f7d903b4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java @@ -191,6 +191,7 @@ public StampedAndroidManifest stamp(AndroidDataContext dataContext) { new ManifestMergerActionBuilder() .setManifest(manifest) .setLibrary(true) + .setMergeManifestPermissions(dataContext.getAndroidConfig().mergeAndroidManifestPermissions()) .setCustomPackage(pkg) .setManifestOutput(outputManifest) .build(dataContext); @@ -235,6 +236,7 @@ public StampedAndroidManifest mergeWithDeps( .setManifest(manifest) .setMergeeManifests(mergeeManifests) .setLibrary(false) + .setMergeManifestPermissions(dataContext.getAndroidConfig().mergeAndroidManifestPermissions()) .setManifestValues(manifestValues) .setCustomPackage(pkg) .setManifestOutput(newManifest) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java index 29a92f1e4234d5..e8ef4a7bfcdb99 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java @@ -27,6 +27,7 @@ public class ManifestMergerActionBuilder { private Artifact manifest; private Map mergeeManifests; private boolean isLibrary; + private boolean mergeManifestPermissions; private Map manifestValues; private String customPackage; private Artifact manifestOutput; @@ -47,6 +48,11 @@ public ManifestMergerActionBuilder setLibrary(boolean isLibrary) { return this; } + public ManifestMergerActionBuilder setMergeManifestPermissions(boolean mergeManifestPermissions) { + this.mergeManifestPermissions = mergeManifestPermissions; + return this; + } + public ManifestMergerActionBuilder setManifestValues(Map manifestValues) { this.manifestValues = manifestValues; return this; @@ -80,6 +86,10 @@ public void build(AndroidDataContext dataContext) { mergeeManifests.keySet()); } + if (mergeManifestPermissions) { + builder.addFlag("--mergeManifestPermissions"); + } + builder .maybeAddFlag("--mergeType", isLibrary) .maybeAddFlag("LIBRARY", isLibrary) diff --git a/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java b/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java index fda1cd88e16341..28ee19f10b3996 100644 --- a/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java +++ b/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java @@ -159,7 +159,62 @@ public void testMerge_GenerateDummyManifest() throws Exception { false, /* isLibrary */ ImmutableMap.of("applicationId", "com.google.android.apps.testapp"), "", /* custom_package */ - mergedManifest); + mergedManifest, + /* mergeManifestPermissions */false); + ManifestMergerAction.main(args.toArray(new String[0])); + + assertThat( + Joiner.on(" ") + .join(Files.readAllLines(mergedManifest, UTF_8)) + .replaceAll("\\s+", " ") + .trim()) + .isEqualTo( + Joiner.on(" ") + .join(Files.readAllLines(expectedManifest, UTF_8)) + .replaceAll("\\s+", " ") + .trim()); + } + + @Test public void testMergeWithMergePermissionsEnabled() throws Exception { + String dataDir = + Paths.get(System.getenv("TEST_WORKSPACE"), System.getenv("TEST_BINARY")) + .resolveSibling("testing/manifestmerge") + .toString() + .replace("\\", "/"); + + final Path mergerManifest = rlocation(dataDir + "/merger/AndroidManifest.xml"); + final Path mergeeManifestOne = rlocation(dataDir + "/mergeeOne/AndroidManifest.xml"); + final Path mergeeManifestTwo = rlocation(dataDir + "/mergeeTwo/AndroidManifest.xml"); + assertThat(mergerManifest.toFile().exists()).isTrue(); + assertThat(mergeeManifestOne.toFile().exists()).isTrue(); + assertThat(mergeeManifestTwo.toFile().exists()).isTrue(); + + // The following code retrieves the path of the only AndroidManifest.xml in the expected/ + // manifests directory. Unfortunately, this test runs internally and externally and the files + // have different names. + final File expectedManifestDirectory = + mergerManifest.getParent().resolveSibling("expected-merged-permissions").toFile(); + final String[] debug = + expectedManifestDirectory.list(new PatternFilenameFilter(".*AndroidManifest\\.xml$")); + assertThat(debug).isNotNull(); + final File[] expectedManifestDirectoryManifests = + expectedManifestDirectory.listFiles((File dir, String name) -> true); + assertThat(expectedManifestDirectoryManifests).isNotNull(); + assertThat(expectedManifestDirectoryManifests).hasLength(1); + final Path expectedManifest = expectedManifestDirectoryManifests[0].toPath(); + + Files.createDirectories(working.resolve("output")); + final Path mergedManifest = working.resolve("output/mergedManifest.xml"); + + List args = + generateArgs( + mergerManifest, + ImmutableMap.of(mergeeManifestOne, "mergeeOne", mergeeManifestTwo, "mergeeTwo"), + false, /* isLibrary */ + ImmutableMap.of("applicationId", "com.google.android.apps.testapp"), + "", /* custom_package */ + mergedManifest, + /* mergeManifestPermissions */true); ManifestMergerAction.main(args.toArray(new String[0])); assertThat( @@ -199,7 +254,7 @@ public void testMerge_GenerateDummyManifest() throws Exception { // libFoo manifest merging List args = generateArgs(libFooManifest, ImmutableMap.of(), true, - ImmutableMap.of(), "", libFooOutput); + ImmutableMap.of(), "", libFooOutput, false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(libFooOutput, UTF_8)) @@ -212,7 +267,7 @@ public void testMerge_GenerateDummyManifest() throws Exception { // libBar manifest merging args = generateArgs(libBarManifest, ImmutableMap.of(), true, - ImmutableMap.of(), "com.google.libbar", libBarOutput); + ImmutableMap.of(), "com.google.libbar", libBarOutput, false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(libBarOutput, UTF_8)) @@ -235,7 +290,8 @@ public void testMerge_GenerateDummyManifest() throws Exception { "applicationId", "com.google.android.app", "foo", "this \\\\: is \"a, \"bad string"), /* customPackage= */ "", - binaryOutput); + binaryOutput, + /* mergeManifestPermissions */false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(binaryOutput, UTF_8)) @@ -255,14 +311,22 @@ private List generateArgs( boolean library, Map manifestValues, String customPackage, - Path manifestOutput) { - return ImmutableList.of( + Path manifestOutput, + boolean mergeManifestPermissions) { + ImmutableList.Builder builder = ImmutableList.builder(); + builder.add( "--manifest", manifest.toString(), - "--mergeeManifests", mapToDictionaryString(mergeeManifests), + "--mergeeManifests", mapToDictionaryString(mergeeManifests)); + if (mergeManifestPermissions) { + builder.add("--mergeManifestPermissions"); + } + + builder.add( "--mergeType", library ? "LIBRARY" : "APPLICATION", "--manifestValues", mapToDictionaryString(manifestValues), "--customPackage", customPackage, "--manifestOutput", manifestOutput.toString()); + return builder.build(); } private String mapToDictionaryString(Map map) { diff --git a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD index 7d7dcc4c112a5a..45a90241d7127c 100644 --- a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD +++ b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD @@ -13,6 +13,7 @@ filegroup( filegroup( name = "test_data", srcs = [ + "expected-merged-permissions/AndroidManifest.xml", "expected/AndroidManifest.xml", "mergeeOne/AndroidManifest.xml", "mergeeTwo/AndroidManifest.xml", diff --git a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml new file mode 100644 index 00000000000000..1cbf1982188b05 --- /dev/null +++ b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml @@ -0,0 +1,107 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java b/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java index 51d2ad40d2f847..f31665a45e4c56 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java @@ -62,6 +62,7 @@ * --manifestValues key value pairs of manifest overrides * --customPackage package to write for library manifest * --manifestOutput path to write output manifest + * --mergeManifestPermissions merge manifest uses-permissions * */ public class ManifestMergerAction { @@ -152,6 +153,16 @@ public static final class Options extends OptionsBase { help = "Path to where the merger log should be written." ) public Path log; + + @Option( + name = "mergeManifestPermissions", + defaultValue = "false", + category = "output", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + help = "If enabled, manifest permissions will be merged." + ) + public boolean mergeManifestPermissions; } private static final String[] PERMISSION_TAGS = @@ -194,13 +205,17 @@ public static void main(String[] args) throws Exception { Path mergedManifest; AndroidManifestProcessor manifestProcessor = AndroidManifestProcessor.with(stdLogger); - // Remove uses-permission tags from mergees before the merge. Path tmp = Files.createTempDirectory("manifest_merge_tmp"); tmp.toFile().deleteOnExit(); ImmutableMap.Builder mergeeManifests = ImmutableMap.builder(); for (Map.Entry mergeeManifest : options.mergeeManifests.entrySet()) { - mergeeManifests.put( - removePermissions(mergeeManifest.getKey(), tmp), mergeeManifest.getValue()); + if (!options.mergeManifestPermissions) { + // Remove uses-permission tags from mergees before the merge. + mergeeManifests.put( + removePermissions(mergeeManifest.getKey(), tmp), mergeeManifest.getValue()); + } else { + mergeeManifests.put(mergeeManifest); + } } Path manifest = options.manifest;