Merge ManifestMergerAction-related commits into release-5.3.0 (bazelb…

…uild#15824)

* Support uses-permission merging in manifest merger

Adding support for conditionally merging `uses-permissions`.

bazelbuild#10628
bazelbuild#5411

Closes bazelbuild#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 bazelbuild#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 bazelbuild#13760.

PiperOrigin-RevId: 438643774

Co-authored-by: Ben Lee <ben@ben.cm>

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();

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;

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)

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;
+ }
+}

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)

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) {

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",

src/test/java/com/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>