From ebbf24f66d92d88b3f3ef1bebdab6946f2cac706 Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Tue, 5 Apr 2022 10:14:58 -0700 Subject: [PATCH] Support uses-permission merging in manifest merger Adding support for conditionally merging `uses-permissions`. https://github.com/bazelbuild/bazel/issues/10628 https://github.com/bazelbuild/bazel/issues/5411 Closes #13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035 --- .../bazel/rules/BazelRuleClassProvider.java | 3 + .../lib/rules/android/AndroidDataContext.java | 6 + .../lib/rules/android/AndroidManifest.java | 14 +++ .../android/BazelAndroidConfiguration.java | 62 ++++++++++ .../android/ManifestMergerActionBuilder.java | 10 ++ .../android/ManifestMergerActionTest.java | 112 ++++++++++++++++-- .../AndroidManifest.xml | 107 +++++++++++++++++ .../build/android/ManifestMergerAction.java | 20 +++- 8 files changed, 318 insertions(+), 16 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java 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/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 5fae569920e950..279930ff327bb9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -93,6 +93,8 @@ import com.google.devtools.build.lib.rules.android.AndroidSdkProvider; import com.google.devtools.build.lib.rules.android.AndroidStarlarkCommon; import com.google.devtools.build.lib.rules.android.ApkInfo; +import com.google.devtools.build.lib.rules.android.BaselineProfileProvider; +import com.google.devtools.build.lib.rules.android.BazelAndroidConfiguration; import com.google.devtools.build.lib.rules.android.DexArchiveAspect; import com.google.devtools.build.lib.rules.android.ProguardMappingProvider; import com.google.devtools.build.lib.rules.android.databinding.DataBindingV2Provider; @@ -346,6 +348,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) { String toolsRepository = checkNotNull(builder.getToolsRepository()); builder.addConfigurationFragment(AndroidConfiguration.class); + builder.addConfigurationFragment(BazelAndroidConfiguration.class); builder.addConfigurationFragment(AndroidLocalTestConfiguration.class); AndroidNeverlinkAspect androidNeverlinkAspect = new AndroidNeverlinkAspect(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java index 630c1fdb7d169f..ef6036539dcfa6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.packages.TriState; import com.google.devtools.build.lib.starlarkbuildapi.android.AndroidDataContextApi; import com.google.devtools.build.lib.vfs.PathFragment; +import javax.annotation.Nullable; /** * Wraps common tools and settings used for working with Android assets, resources, and manifests. @@ -207,6 +208,11 @@ public AndroidConfiguration getAndroidConfig() { return ruleContext.getConfiguration().getFragment(AndroidConfiguration.class); } + @Nullable + public BazelAndroidConfiguration getBazelAndroidConfig() { + return ruleContext.getConfiguration().getFragment(BazelAndroidConfiguration.class); + } + /** Indicates whether Busybox actions should be passed the "--debug" flag */ public boolean useDebug() { return getActionConstructionContext().getConfiguration().getCompilationMode() != OPT; 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 ed9d097fa7a56d..b0ebd5224ec930 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 @@ -188,6 +188,18 @@ public AndroidManifest(Artifact manifest, @Nullable String pkg, boolean exported this.exported = exported; } + /** Checks if manifest permission merging is enabled. */ + private boolean getMergeManifestPermissionsEnabled(AndroidDataContext dataContext) { + // Only enable manifest merging if BazelAndroidConfiguration exists. If the class does not + // exist, then return false immediately. Otherwise, return the user-specified value of + // mergeAndroidManifestPermissions. + BazelAndroidConfiguration bazelAndroidConfig = dataContext.getBazelAndroidConfig(); + if (bazelAndroidConfig == null) { + return false; + } + return bazelAndroidConfig.getMergeAndroidManifestPermissions(); + } + /** If needed, stamps the manifest with the correct Java package */ public StampedAndroidManifest stamp(AndroidDataContext dataContext) { Artifact outputManifest = getManifest(); @@ -196,6 +208,7 @@ public StampedAndroidManifest stamp(AndroidDataContext dataContext) { new ManifestMergerActionBuilder() .setManifest(manifest) .setLibrary(true) + .setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext)) .setCustomPackage(pkg) .setManifestOutput(outputManifest) .build(dataContext); @@ -240,6 +253,7 @@ public StampedAndroidManifest mergeWithDeps( .setManifest(manifest) .setMergeeManifests(mergeeManifests) .setLibrary(false) + .setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext)) .setManifestValues(manifestValues) .setCustomPackage(pkg) .setManifestOutput(newManifest) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java new file mode 100644 index 00000000000000..7b7559b4e6b5db --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java @@ -0,0 +1,62 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.rules.android; + +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.Fragment; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.build.lib.analysis.config.RequiresOptions; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; +import com.google.errorprone.annotations.CheckReturnValue; + +/** Configuration fragment for Android rules that is specific to Bazel. */ +@Immutable +@RequiresOptions(options = {BazelAndroidConfiguration.Options.class}) +@CheckReturnValue +public class BazelAndroidConfiguration extends Fragment { + + @Override + public boolean isImmutable() { + return true; // immutable and Starlark-hashable + } + + /** Android configuration options that are specific to Bazel. */ + public static class Options extends FragmentOptions { + + @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; + } + + private final boolean mergeAndroidManifestPermissions; + + public BazelAndroidConfiguration(BuildOptions buildOptions) { + Options options = buildOptions.get(BazelAndroidConfiguration.Options.class); + this.mergeAndroidManifestPermissions = options.mergeAndroidManifestPermissions; + } + + public boolean getMergeAndroidManifestPermissions() { + return this.mergeAndroidManifestPermissions; + } +} 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 cfec08b3a4134d..ee8bb7b53683de 100644 --- a/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java +++ b/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java @@ -196,7 +196,8 @@ 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( @@ -211,6 +212,64 @@ public void testMerge_GenerateDummyManifest() throws Exception { .trim()); } + @Test + public void testMergeWithMergePermissionsEnabled() throws Exception { + // Largely copied from testMerge() above. Perhaps worth combining the two test methods into one + // method in the future? + 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-merged-permission/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(); + assertThat(expectedManifestDirectory.exists()).isTrue(); + 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( + 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 fullIntegration() throws Exception { Files.createDirectories(working.resolve("output")); final Path binaryOutput = working.resolve("output/binaryManifest.xml"); @@ -235,8 +294,15 @@ public void testMerge_GenerateDummyManifest() throws Exception { .getManifest(); // libFoo manifest merging - List args = generateArgs(libFooManifest, ImmutableMap.of(), true, - ImmutableMap.of(), "", libFooOutput); + List args = + generateArgs( + libFooManifest, + ImmutableMap.of(), + true, + ImmutableMap.of(), + "", + libFooOutput, + false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(libFooOutput, UTF_8)) @@ -248,8 +314,15 @@ public void testMerge_GenerateDummyManifest() throws Exception { + ""); // libBar manifest merging - args = generateArgs(libBarManifest, ImmutableMap.of(), true, - ImmutableMap.of(), "com.google.libbar", libBarOutput); + args = + generateArgs( + libBarManifest, + ImmutableMap.of(), + true, + ImmutableMap.of(), + "com.google.libbar", + libBarOutput, + false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(libBarOutput, UTF_8)) @@ -272,7 +345,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)) @@ -292,14 +366,26 @@ 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), - "--mergeType", library ? "LIBRARY" : "APPLICATION", - "--manifestValues", mapToDictionaryString(manifestValues), - "--customPackage", customPackage, - "--manifestOutput", manifestOutput.toString()); + "--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/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 8d7af22a9a0ed9..cc1f8dbff5b9d9 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,15 @@ 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 = @@ -195,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;