From 39c2a36bf8f502d55addd58e9e97bf2ebb46e4b0 Mon Sep 17 00:00:00 2001 From: corysmith Date: Thu, 30 Nov 2017 08:55:49 -0800 Subject: Improve the error messaging by suppressing stack traces for expected errors. RELNOTES: None PiperOrigin-RevId: 177460834 --- .../build/android/ParsedAndroidDataTest.java | 63 +++++++++++----------- .../android/Aapt2ResourcePackagingAction.java | 41 +++++--------- .../devtools/build/android/AaptCommandBuilder.java | 8 ++- .../build/android/AndroidResourceMerger.java | 2 +- .../build/android/ResourceProcessorBusyBox.java | 21 +++++++- .../android/ValidateAndLinkResourcesAction.java | 26 ++++----- .../build/android/aapt2/Aapt2Exception.java | 27 ++++++++++ .../build/android/aapt2/ResourceCompiler.java | 35 +++++++++++- .../build/android/aapt2/ResourceLinker.java | 10 ++-- 9 files changed, 150 insertions(+), 83 deletions(-) create mode 100644 src/tools/android/java/com/google/devtools/build/android/aapt2/Aapt2Exception.java diff --git a/src/test/java/com/google/devtools/build/android/ParsedAndroidDataTest.java b/src/test/java/com/google/devtools/build/android/ParsedAndroidDataTest.java index 1ac69385c9..e7a55bc9a4 100644 --- a/src/test/java/com/google/devtools/build/android/ParsedAndroidDataTest.java +++ b/src/test/java/com/google/devtools/build/android/ParsedAndroidDataTest.java @@ -141,8 +141,8 @@ public class ParsedAndroidDataTest { .createManifest("AndroidManifest.xml", "com.google.foo", "") .buildDependency())); - FullyQualifiedName.Factory fqnFactory = FullyQualifiedName.Factory.from( - ImmutableList.of("hdpi", "v4")); + FullyQualifiedName.Factory fqnFactory = + FullyQualifiedName.Factory.from(ImmutableList.of("hdpi", "v4")); FullyQualifiedName drawable = fqnFactory.parse("drawable/seven_eight"); assertThat(dataSet.getOverwritingResources()) .containsExactly( @@ -166,8 +166,7 @@ public class ParsedAndroidDataTest { FullyQualifiedName layoutFoo = fqnFactory.parse("layout/foo"); assertThat(dataSet.getOverwritingResources()) - .containsExactly( - layoutFoo, DataValueFile.of(root.resolve("res/layout/foo.xml"))); + .containsExactly(layoutFoo, DataValueFile.of(root.resolve("res/layout/foo.xml"))); assertThat(dataSet.getCombiningResources()).isEmpty(); } @@ -177,14 +176,15 @@ public class ParsedAndroidDataTest { Path parent = fs.getPath("parent"); Path root = parent.resolve("transDep"); AndroidDataBuilder.of(root) - .addResource("layout/foo.xml", AndroidDataBuilder.ResourceType.LAYOUT, "") - .addResourceBinary( - "drawable/menu.png", Files.createFile(fs.getPath("menu.png"))) - .createManifest("AndroidManifest.xml", "com.google.foo", "") - .buildDependency(); + .addResource("layout/foo.xml", AndroidDataBuilder.ResourceType.LAYOUT, "") + .addResourceBinary("drawable/menu.png", Files.createFile(fs.getPath("menu.png"))) + .createManifest("AndroidManifest.xml", "com.google.foo", "") + .buildDependency(); ParsedAndroidData dataSet = ParsedAndroidData.from( - new UnvalidatedAndroidData(ImmutableList.of(root), ImmutableList.of(), + new UnvalidatedAndroidData( + ImmutableList.of(root), + ImmutableList.of(), root.resolve("AndroidManifest.xml"))); FullyQualifiedName layoutFoo = fqnFactory.parse("layout/foo"); @@ -336,8 +336,6 @@ public class ParsedAndroidDataTest { DataSource otherRootValuesPath = DataSource.of(otherRoot.resolve("res/values/attr.xml")); FullyQualifiedName idSomeId = fqnFactory.parse("id/some_id"); - - Truth.assertAbout(parsedAndroidData) .that(dataSet) .isEqualTo( @@ -384,7 +382,6 @@ public class ParsedAndroidDataTest { rootValuesPath, IdXmlResourceValue.of()) // value ), ImmutableMap.of())); - } @Test @@ -469,9 +466,7 @@ public class ParsedAndroidDataTest { FullyQualifiedName layoutKey = fqnFactory.parse("layout/databinding_layout"); Path layoutPath = root.resolve("res/layout/databinding_layout.xml"); assertThat(dataSet.getOverwritingResources()) - .containsExactly( - layoutKey, DataValueFile.of(layoutPath) - ); + .containsExactly(layoutKey, DataValueFile.of(layoutPath)); FullyQualifiedName idTextView = fqnFactory.parse("id/MyTextView"); FullyQualifiedName idTextView2 = fqnFactory.parse("id/AnotherTextView"); assertThat(dataSet.getCombiningResources()) @@ -510,9 +505,7 @@ public class ParsedAndroidDataTest { FullyQualifiedName menuKey = fqnFactory.parse("menu/some_menu"); Path menuPath = root.resolve("res/menu/some_menu.xml"); assertThat(dataSet.getOverwritingResources()) - .containsExactly( - menuKey, DataValueFile.of(menuPath) - ); + .containsExactly(menuKey, DataValueFile.of(menuPath)); FullyQualifiedName groupIdKey = fqnFactory.parse("id/some_group"); FullyQualifiedName itemIdKey = fqnFactory.parse("id/action_settings"); assertThat(dataSet.getCombiningResources()) @@ -553,14 +546,12 @@ public class ParsedAndroidDataTest { "") .createManifest("AndroidManifest.xml", "com.carroll.lewis", "") .buildParsed(); - FullyQualifiedName.Factory fqnV21Factory = FullyQualifiedName.Factory.from( - ImmutableList.of("v21")); + FullyQualifiedName.Factory fqnV21Factory = + FullyQualifiedName.Factory.from(ImmutableList.of("v21")); FullyQualifiedName drawableKey = fqnV21Factory.parse("drawable/some_drawable"); Path drawablePath = root.resolve("res/drawable-v21/some_drawable.xml"); assertThat(dataSet.getOverwritingResources()) - .containsExactly( - drawableKey, DataValueFile.of(drawablePath) - ); + .containsExactly(drawableKey, DataValueFile.of(drawablePath)); FullyQualifiedName focusedState = fqnV21Factory.parse("id/focused_state"); FullyQualifiedName defaultState = fqnV21Factory.parse("id/default_state"); FullyQualifiedName pressedState = fqnV21Factory.parse("id/pressed_state"); @@ -604,7 +595,9 @@ public class ParsedAndroidDataTest { builder.buildParsed(); fail("expected MergingException"); } catch (MergingException e) { - assertThat(e).hasMessageThat().isEqualTo("3 Parse Error(s)"); + assertThat(e) + .hasMessageThat() + .isEqualTo(MergingException.withMessage("3 Parse Error(s)").getMessage()); String combinedSuberrors = Joiner.on('\n').join(e.getSuppressed()); assertThat(combinedSuberrors) .contains(fs.getPath("values/unique_strings.xml") + ": ParseError at [row,col]:[3,35]"); @@ -612,8 +605,7 @@ public class ParsedAndroidDataTest { .contains("unrecognized resource type: "); assertThat(combinedSuberrors) .contains(fs.getPath("layout/unique_layout.xml") + ": ParseError at [row,col]:[6,3]"); - assertThat(combinedSuberrors) - .contains("must be terminated by the matching end-tag"); + assertThat(combinedSuberrors).contains("must be terminated by the matching end-tag"); assertThat(combinedSuberrors) .contains(fs.getPath("menu/unique_menu.xml") + ": ParseError at [row,col]:[1,30]"); assertThat(combinedSuberrors) @@ -626,13 +618,16 @@ public class ParsedAndroidDataTest { Path root = fs.getPath("root"); AndroidDataBuilder builder = AndroidDataBuilder.of(root) - .addResource("values/missing_name.xml", - AndroidDataBuilder.ResourceType.VALUE, - "") - .addResource("values/missing_type.xml", + .addResource( + "values/missing_name.xml", + AndroidDataBuilder.ResourceType.VALUE, + "") + .addResource( + "values/missing_type.xml", AndroidDataBuilder.ResourceType.VALUE, "") - .addResource("values/bad_type.xml", + .addResource( + "values/bad_type.xml", AndroidDataBuilder.ResourceType.VALUE, "") .addResource( @@ -648,7 +643,9 @@ public class ParsedAndroidDataTest { builder.buildParsed(); fail("expected MergingException"); } catch (MergingException e) { - assertThat(e).hasMessageThat().isEqualTo("5 Parse Error(s)"); + assertThat(e) + .hasMessageThat() + .isEqualTo(MergingException.withMessage("5 Parse Error(s)").getMessage()); String combinedSuberrors = Joiner.on('\n').join(e.getSuppressed()); assertThat(combinedSuberrors).contains(fs.getPath("values/missing_name.xml").toString()); assertThat(combinedSuberrors).contains("resource name is required for public"); diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java index 5739afa377..a6902c0e67 100644 --- a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java @@ -19,7 +19,6 @@ import static java.util.stream.Collectors.toList; import com.android.utils.StdLogger; import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListeningExecutorService; -import com.google.devtools.build.android.AndroidResourceMerger.MergingException; import com.google.devtools.build.android.AndroidResourceProcessingAction.Options; import com.google.devtools.build.android.aapt2.Aapt2ConfigOptions; import com.google.devtools.build.android.aapt2.CompiledResources; @@ -31,13 +30,11 @@ import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor; import com.google.devtools.common.options.TriState; import java.io.Closeable; -import java.io.IOException; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; import java.util.Collections; import java.util.List; -import java.util.logging.Logger; /** * Provides an entry point for the resource processing using the AOSP build tools. @@ -63,9 +60,6 @@ public class Aapt2ResourcePackagingAction { private static final StdLogger STD_LOGGER = new StdLogger(StdLogger.Level.WARNING); - private static final Logger logger = - Logger.getLogger(Aapt2ResourcePackagingAction.class.getName()); - private static Aapt2ConfigOptions aaptConfigOptions; private static Options options; @@ -112,18 +106,18 @@ public class Aapt2ResourcePackagingAction { // Checks for merge conflicts. MergedAndroidData mergedAndroidData = AndroidResourceMerger.mergeData( - ParsedAndroidData.from(options.primaryData), - options.primaryData.getManifest(), - options.directData, - options.transitiveData, - mergedResources, - mergedAssets, - null /* cruncher. Aapt2 automatically chooses to crunch or not. */, - options.packageType, - options.symbolsOut, - null /* rclassWriter */, - dataDeserializer, - options.throwOnResourceConflict) + ParsedAndroidData.from(options.primaryData), + options.primaryData.getManifest(), + options.directData, + options.transitiveData, + mergedResources, + mergedAssets, + null /* cruncher. Aapt2 automatically chooses to crunch or not. */, + options.packageType, + options.symbolsOut, + null /* rclassWriter */, + dataDeserializer, + options.throwOnResourceConflict) .filter( new DensitySpecificResourceFilter( densitiesToFilter, filteredResources, mergedResources), @@ -131,7 +125,6 @@ public class Aapt2ResourcePackagingAction { profiler.recordEndOf("merging"); - final ListeningExecutorService executorService = ExecutorServiceCloser.createDefaultService(); try (final Closeable closeable = ExecutorServiceCloser.createWith(executorService)) { profiler.startTask("compile"); @@ -209,16 +202,6 @@ public class Aapt2ResourcePackagingAction { profiler.recordEndOf("package"); } } - } catch (MergingException e) { - logger.severe("Merging exception: " + e.getMessage()); - // throw an error, as system.exit will fail tests. - throw new RuntimeException(); - } catch (IOException e) { - logger.severe("File error: " + e.getMessage()); - // throw an error, as system.exit will fail tests. - throw new RuntimeException(); - } catch (Exception e) { - throw e; } } } diff --git a/src/tools/android/java/com/google/devtools/build/android/AaptCommandBuilder.java b/src/tools/android/java/com/google/devtools/build/android/AaptCommandBuilder.java index f4b70fc930..4a7e711b0a 100644 --- a/src/tools/android/java/com/google/devtools/build/android/AaptCommandBuilder.java +++ b/src/tools/android/java/com/google/devtools/build/android/AaptCommandBuilder.java @@ -302,13 +302,17 @@ public class AaptCommandBuilder { final Process process = new ProcessBuilder().command(command).redirectErrorStream(true).start(); processLog.append("Command: "); - Joiner.on("\n\t").appendTo(processLog, command); - processLog.append("\n"); + Joiner.on("\\\n\t").appendTo(processLog, command); + processLog.append("\nOutput:\n"); final InputStreamReader stdout = new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8); while (process.isAlive()) { processLog.append(CharStreams.toString(stdout)); } + // Make sure the full stdout is read. + while (stdout.ready()) { + processLog.append(CharStreams.toString(stdout)); + } if (process.exitValue() != 0) { throw new RuntimeException(String.format("Error during %s:", action) + "\n" + processLog); } diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMerger.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMerger.java index bc9bf6d408..2013617964 100644 --- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMerger.java +++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMerger.java @@ -38,7 +38,7 @@ public class AndroidResourceMerger { } private MergingException(String message) { - super(message); + super("Merging Error: \n" + message); } static MergingException wrapException(Throwable e) { diff --git a/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java b/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java index f1da34a520..a99da9b931 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java +++ b/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java @@ -14,6 +14,8 @@ package com.google.devtools.build.android; +import com.google.devtools.build.android.AndroidResourceMerger.MergingException; +import com.google.devtools.build.android.aapt2.Aapt2Exception; import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; @@ -21,7 +23,10 @@ import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor; +import java.io.IOException; import java.nio.file.FileSystems; +import java.util.Arrays; +import java.util.logging.Logger; /** * Provides an entry point for the resource processing stages. @@ -144,6 +149,8 @@ public class ResourceProcessorBusyBox { abstract void call(String[] args) throws Exception; } + private static final Logger logger = Logger.getLogger(ResourceProcessorBusyBox.class.getName()); + /** Converter for the Tool enum. */ public static final class ToolConverter extends EnumConverter { @@ -177,6 +184,18 @@ public class ResourceProcessorBusyBox { new ShellQuotedParamsFilePreProcessor(FileSystems.getDefault())); optionsParser.parse(args); Options options = optionsParser.getOptions(Options.class); - options.tool.call(optionsParser.getResidue().toArray(new String[0])); + try { + options.tool.call(optionsParser.getResidue().toArray(new String[0])); + } catch (MergingException | IOException e) { + logger.severe(e.getMessage()); + logSuppressedAndExit(e); + } catch (Aapt2Exception e) { + logSuppressedAndExit(e); + } + } + + private static void logSuppressedAndExit(Throwable e) { + Arrays.stream(e.getSuppressed()).map(Throwable::getMessage).forEach(logger::severe); + System.exit(1); } } diff --git a/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java b/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java index 31f7741122..5af2c67a59 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java @@ -51,18 +51,17 @@ public class ValidateAndLinkResourcesAction { public Path compiled; @Option( - name = "compiledDep", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, - defaultValue = "", - converter = Converters.PathListConverter.class, - category = "input", - allowMultiple = true, - help = "Compiled resource dependencies to link." + name = "compiledDep", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + defaultValue = "", + converter = Converters.PathListConverter.class, + category = "input", + allowMultiple = true, + help = "Compiled resource dependencies to link." ) public List compiledDeps; - @Option( name = "manifest", documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, @@ -188,9 +187,12 @@ public class ValidateAndLinkResourcesAction { ResourceLinker.create(aapt2Options.aapt2, scopedTmp.getPath()) .profileUsing(profiler) .dependencies(Optional.ofNullable(options.deprecatedLibraries).orElse(options.libraries)) - .include(options.compiledDeps.stream() - .map(CompiledResources::from) - .collect(Collectors.toList())) + .include( + options + .compiledDeps + .stream() + .map(CompiledResources::from) + .collect(Collectors.toList())) .buildVersion(aapt2Options.buildToolsVersion) .linkStatically(resources) .copyLibraryTo(options.staticLibraryOut) diff --git a/src/tools/android/java/com/google/devtools/build/android/aapt2/Aapt2Exception.java b/src/tools/android/java/com/google/devtools/build/android/aapt2/Aapt2Exception.java new file mode 100644 index 0000000000..4781698e8c --- /dev/null +++ b/src/tools/android/java/com/google/devtools/build/android/aapt2/Aapt2Exception.java @@ -0,0 +1,27 @@ +// Copyright 2017 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. +// Copyright 2017 The Bazel Authors. All rights reserved. + +package com.google.devtools.build.android.aapt2; + +/** Represents an error found during an aapt2 execution pass. */ +public abstract class Aapt2Exception extends RuntimeException { + protected Aapt2Exception(Throwable e) { + super(e); + } + + public Aapt2Exception() { + super(); + } +} diff --git a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java index 5f8b842859..e51192ce1e 100644 --- a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java +++ b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java @@ -17,7 +17,8 @@ package com.google.devtools.build.android.aapt2; import com.android.builder.core.VariantType; import com.android.repository.Revision; import com.google.common.base.Preconditions; -import com.google.common.util.concurrent.Futures; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableList.Builder; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.devtools.build.android.AaptCommandBuilder; @@ -29,12 +30,30 @@ import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.logging.Logger; /** Invokes aapt2 to compile resources. */ public class ResourceCompiler { + static class CompileError extends Aapt2Exception { + + protected CompileError(Throwable e) { + super(e); + } + + private CompileError() { + super(); + } + + public static CompileError of(List compilationErrors) { + final CompileError compileError = new CompileError(); + compilationErrors.forEach(compileError::addSuppressed); + return compileError; + } + } + private static final Logger logger = Logger.getLogger(ResourceCompiler.class.getName()); private final CompilingVisitor compilingVisitor; @@ -131,7 +150,19 @@ public class ResourceCompiler { } List getCompiledArtifacts() throws InterruptedException, ExecutionException { - return Futures.allAsList(tasks).get(); + Builder builder = ImmutableList.builder(); + List compilationErrors = new ArrayList<>(); + for (ListenableFuture task : tasks) { + try { + builder.add(task.get()); + } catch (InterruptedException | ExecutionException e) { + compilationErrors.add(Optional.ofNullable(e.getCause()).orElse(e)); + } + } + if (compilationErrors.isEmpty()) { + return builder.build(); + } + throw CompileError.of(compilationErrors); } } diff --git a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java index 2787722c36..99af38766b 100644 --- a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java +++ b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java @@ -39,11 +39,15 @@ import java.util.stream.Collectors; public class ResourceLinker { /** Represents errors thrown during linking. */ - public static class LinkError extends RuntimeException { + public static class LinkError extends Aapt2Exception { - public LinkError(Throwable e) { + private LinkError(Throwable e) { super(e); } + + public static LinkError of(IOException e) { + return new LinkError(e); + } } private static Logger logger = Logger.getLogger(ResourceLinker.class.getName()); @@ -192,7 +196,7 @@ public class ResourceLinker { profiler.recordEndOf("sourcejar"); return StaticLibrary.from(outPath, rTxt, ImmutableList.of(), sourceJar); } catch (IOException e) { - throw new LinkError(e); + throw LinkError.of(e); } } -- cgit v1.2.3