From f233523bea7cdbd7d8272fb26c0ab276435934e3 Mon Sep 17 00:00:00 2001 From: Benjamin Lee Date: Tue, 10 May 2022 13:08:03 -0700 Subject: [PATCH] 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 --- .../android/ManifestMergerActionTest.java | 37 +++++++++++++++++++ .../build/android/testing/manifestmerge/BUILD | 2 + .../brokenManifest/AndroidManifest.xml | 10 +++++ .../build/android/ManifestMergerAction.java | 13 +++++-- 4 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml 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..cfec08b3a4134d 100644 --- 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")); 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..79cd5c4f2bdb2a 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,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/src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml new file mode 100644 index 00000000000000..fe650964e05333 --- /dev/null +++ b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml @@ -0,0 +1,10 @@ + + + + + \ No newline at end of file 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 64832bc6b9472d..8d7af22a9a0ed9 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 @@ -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.