Make ManifestMergerAction worker compatible

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

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>

src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java

@@ -229,10 +229,15 @@ public static void main(String[] args) throws Exception {
         Files.copy(manifest, options.manifestOutput, StandardCopyOption.REPLACE_EXISTING);
       }
     } catch (AndroidManifestProcessor.ManifestProcessingException e) {
-      // We special case ManifestProcessingExceptions here to indicate that this is
-      // caused by a build error, not an Bazel-internal error.
-      logger.log(SEVERE, "Error during merging manifests", e);
-      System.exit(1); // Don't duplicate the error to the user or bubble up the exception.
+      // ManifestProcessingExceptions represent build errors that should be delivered directly to
+      // ResourceProcessorBusyBox where the exception can be delivered with a non-zero status code
+      // to the worker/process
+      // Note that this exception handler is nearly identical to the generic case, except that it
+      // does not have a log print associated with it. This is because the exception will bubble up
+      // to ResourceProcessorBusyBox, which will print an identical error message. It is preferable
+      // to slightly convolute this try/catch block, rather than pollute the user's console with
+      // extra repeated error messages.
+      throw e;
     } catch (Exception e) {
       logger.log(SEVERE, "Error during merging manifests", e);
       throw e; // This is a proper internal exception, so we bubble it up.