Merge ManifestMergerAction-related commits into release-5.3.0 (#15824)

* Support uses-permission merging in manifest merger Adding support for conditionally merging `uses-permissions`. #10628 #5411 Closes #13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035 * Make ManifestMergerAction worker compatible Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output. Closes #14427. PiperOrigin-RevId: 447808701 * Fix ManifestMergerAction test case on windows `manifest.toString().replaceFirst("^/", "")` silently fails on windows machines causing `removePermissions` to write to the original test file. This pull request creates a new temp file that `removePermissions` can write the modified manifest to. Pulling this change out of another PR so that it's easier to merge. Original PR here https://github.com/bazelbuild/bazel/pull/13445/files#r631575251 Closes #13760. PiperOrigin-RevId: 438643774 Co-authored-by: Ben Lee <[email protected]>
bazelbuild · Jul 6, 2022 · 3ea9eb2 · 3ea9eb2
1 parent 024f552
commit 3ea9eb2
Show file tree

Hide file tree

Showing 10 changed files with 377 additions and 23 deletions.
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
@@ -93,6 +93,7 @@
 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.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 +347,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
@@ -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
@@ -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
@@ -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
@@ -27,6 +27,7 @@ public class ManifestMergerActionBuilder {
   private Artifact manifest;
   private Map<Artifact, Label> mergeeManifests;
   private boolean isLibrary;
+  private boolean mergeManifestPermissions;
   private Map<String, String> 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<String, String> 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
@@ -15,6 +15,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.Assert.assertThrows;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
@@ -92,6 +93,42 @@ private static Path rlocation(String path) throws IOException {
     working.toFile().deleteOnExit();
   }
 
+  @Test
+  public void testMergeManifestWithBrokenManifestSyntax() throws Exception {
+    String dataDir =
+        Paths.get(System.getenv("TEST_WORKSPACE"), System.getenv("TEST_BINARY"))
+            .resolveSibling("testing/manifestmerge")
+            .toString()
+            .replace('\\', '/');
+    Files.createDirectories(working.resolve("output"));
+    final Path mergedManifest = working.resolve("output/mergedManifest.xml");
+    final Path brokenMergerManifest = rlocation(dataDir + "/brokenManifest/AndroidManifest.xml");
+    assertThat(brokenMergerManifest.toFile().exists()).isTrue();
+
+    AndroidManifestProcessor.ManifestProcessingException e =
+        assertThrows(
+            AndroidManifestProcessor.ManifestProcessingException.class,
+            () -> {
+              ManifestMergerAction.main(
+                  generateArgs(
+                          brokenMergerManifest,
+                          ImmutableMap.of(),
+                          false, /* isLibrary */
+                          ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
+                          "", /* custom_package */
+                          mergedManifest,
+                          false /* mergeManifestPermissions */)
+                      .toArray(new String[0]));
+            });
+    assertThat(e)
+        .hasMessageThat()
+        .contains(
+            "com.android.manifmerger.ManifestMerger2$MergeFailureException: "
+                + "org.xml.sax.SAXParseException; lineNumber: 6; columnNumber: 6; "
+                + "The markup in the document following the root element must be well-formed.");
+    assertThat(mergedManifest.toFile().exists()).isFalse();
+  }
+
   @Test
   public void testMerge_GenerateDummyManifest() throws Exception {
     Files.createDirectories(working.resolve("output"));
@@ -159,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(
@@ -174,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<String> 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");
@@ -198,8 +294,15 @@ public void testMerge_GenerateDummyManifest() throws Exception {
         .getManifest();
 
     // libFoo manifest merging
-    List<String> args = generateArgs(libFooManifest, ImmutableMap.<Path, String>of(), true,
-        ImmutableMap.<String, String>of(), "", libFooOutput);
+    List<String> args =
+        generateArgs(
+            libFooManifest,
+            ImmutableMap.<Path, String>of(),
+            true,
+            ImmutableMap.<String, String>of(),
+            "",
+            libFooOutput,
+            false);
     ManifestMergerAction.main(args.toArray(new String[0]));
     assertThat(Joiner.on(" ")
         .join(Files.readAllLines(libFooOutput, UTF_8))
@@ -211,8 +314,15 @@ public void testMerge_GenerateDummyManifest() throws Exception {
             + "</manifest>");
 
     // libBar manifest merging
-    args = generateArgs(libBarManifest, ImmutableMap.<Path, String>of(), true,
-        ImmutableMap.<String, String>of(), "com.google.libbar", libBarOutput);
+    args =
+        generateArgs(
+            libBarManifest,
+            ImmutableMap.<Path, String>of(),
+            true,
+            ImmutableMap.<String, String>of(),
+            "com.google.libbar",
+            libBarOutput,
+            false);
     ManifestMergerAction.main(args.toArray(new String[0]));
     assertThat(Joiner.on(" ")
         .join(Files.readAllLines(libBarOutput, UTF_8))
@@ -235,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))
@@ -255,14 +366,26 @@ private List<String> generateArgs(
       boolean library,
       Map<String, String> manifestValues,
       String customPackage,
-      Path manifestOutput) {
-    return ImmutableList.of(
+      Path manifestOutput,
+      boolean mergeManifestPermissions) {
+    ImmutableList.Builder<String> 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 <K, V> String mapToDictionaryString(Map<K, V> 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
@@ -13,6 +13,8 @@ filegroup(
 filegroup(
     name = "test_data",
     srcs = [
+        "brokenManifest/AndroidManifest.xml",
+        "expected-merged-permissions/AndroidManifest.xml",
         "expected/AndroidManifest.xml",
         "mergeeOne/AndroidManifest.xml",
         "mergeeTwo/AndroidManifest.xml",

diff --git a/...om/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml b/...om/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml
@@ -0,0 +1,10 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+          package="com.example.bazel"
+          android:versionCode="1"
+          android:versionName="1.0"/>
+
+    <uses-sdk
+            android:minSdkVersion="21"
+            android:targetSdkVersion="26"/>
+
+</manifest>