Add flag to turn Android resource merge conflicts from warnings into errors

RELNOTES: none
PiperOrigin-RevId: 161831232

5 files changed, 98 insertions, 30 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/AarGeneratorAction.java b/src/tools/android/java/com/google/devtools/build/android/AarGeneratorAction.java
index 7a6947d698..16091348c1 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AarGeneratorAction.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AarGeneratorAction.java
@@ -19,6 +19,7 @@ import com.google.common.base.Joiner;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Ordering;
+import com.google.devtools.build.android.AndroidDataMerger.MergeConflictException;
 import com.google.devtools.build.android.AndroidResourceMerger.MergingException;
 import com.google.devtools.build.android.Converters.ExistingPathConverter;
 import com.google.devtools.build.android.Converters.PathConverter;
@@ -124,6 +125,14 @@ public class AarGeneratorAction {
       help = "Path to write the archive."
     )
     public Path aarOutput;
+
+    @Option(name = "throwOnResourceConflict",
+        defaultValue = "false",
+        category = "config",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help = "If passed, resource merge conflicts will be treated as errors instead of warnings")
+    public boolean throwOnResourceConflict;
   }
 
   public static void main(String[] args) {
@@ -153,12 +162,17 @@ public class AarGeneratorAction {
               null,
               VariantType.LIBRARY,
               null,
-              /* filteredResources= */ ImmutableList.<String>of());
+              /* filteredResources= */ ImmutableList.<String>of(),
+              options.throwOnResourceConflict
+          );
       logger.fine(String.format("Merging finished at %dms", timer.elapsed(TimeUnit.MILLISECONDS)));
 
       writeAar(options.aarOutput, mergedData, options.manifest, options.rtxt, options.classes);
       logger.fine(
           String.format("Packaging finished at %dms", timer.elapsed(TimeUnit.MILLISECONDS)));
+    } catch (MergeConflictException e) {
+      logger.log(Level.SEVERE, e.getMessage());
+      System.exit(1);
     } catch (IOException | MergingException e) {
       logger.log(Level.SEVERE, "Error during merging resources", e);
       System.exit(1);
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
index 44967168b9..f5b0b0040e 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
@@ -36,6 +36,17 @@ import java.util.logging.Logger;
 /** Handles the Merging of ParsedAndroidData. */
 class AndroidDataMerger {
 
+  public static class MergeConflictException extends RuntimeException {
+
+    private MergeConflictException(String message) {
+      super(message);
+    }
+
+    static MergeConflictException withMessage(String message) {
+      return new MergeConflictException(message);
+    }
+  }
+
   private static final Logger logger = Logger.getLogger(AndroidDataMerger.class.getCanonicalName());
 
   /** Interface for comparing paths. */
@@ -70,7 +81,7 @@ class AndroidDataMerger {
               // getFileSize did not return correct size.
               logger.severe(
                   String.format(
-                      "Filesystem size of %s (%s) or %s (%s) is inconsistant with bytes read %s.",
+                      "Filesystem size of %s (%s) or %s (%s) is inconsistent with bytes read %s.",
                       one, one.getFileSize(), two, two.getFileSize(), bytesRead));
               return false;
             }
@@ -131,15 +142,15 @@ class AndroidDataMerger {
    * ParsedAndroidData}.
    *
    * @see AndroidDataMerger#merge(ParsedAndroidData, ParsedAndroidData, UnvalidatedAndroidData,
-   *     boolean) for details.
+   *     boolean, boolean) for details.
    */
   UnwrittenMergedAndroidData loadAndMerge(
       List<? extends SerializedAndroidData> transitive,
       List<? extends SerializedAndroidData> direct,
       ParsedAndroidData primary,
       Path primaryManifest,
-      boolean allowPrimaryOverrideAll)
-      throws MergingException {
+      boolean allowPrimaryOverrideAll,
+      boolean throwOnResourceConflict) {
     Stopwatch timer = Stopwatch.createStarted();
     try {
       logger.fine(
@@ -150,7 +161,8 @@ class AndroidDataMerger {
           ParsedAndroidData.loadedFrom(direct, executorService, deserializer),
           primary,
           primaryManifest,
-          allowPrimaryOverrideAll);
+          allowPrimaryOverrideAll,
+          throwOnResourceConflict);
     } finally {
       logger.fine(String.format("Resources merged in %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
     }
@@ -218,20 +230,26 @@ class AndroidDataMerger {
    *     the ultimate source of truth, provided it doesn't conflict with itself.
    * @return An UnwrittenMergedAndroidData, containing DataResource objects that can be written to
    *     disk for aapt processing or serialized for future merge passes.
-   * @throws MergingException if there are merge conflicts or issues with parsing resources from
+   * @throws MergingException if there are issues with parsing resources from
    *     primaryData.
+   * @throws MergeConflictException if there are merge conflicts
    */
   UnwrittenMergedAndroidData merge(
       ParsedAndroidData transitive,
       ParsedAndroidData direct,
       UnvalidatedAndroidData primaryData,
-      boolean allowPrimaryOverrideAll)
-      throws MergingException {
+      boolean allowPrimaryOverrideAll,
+      boolean throwOnResourceConflict) {
     try {
       // Extract the primary resources.
       ParsedAndroidData parsedPrimary = ParsedAndroidData.from(primaryData);
       return doMerge(
-          transitive, direct, parsedPrimary, primaryData.getManifest(), allowPrimaryOverrideAll);
+          transitive,
+          direct,
+          parsedPrimary,
+          primaryData.getManifest(),
+          allowPrimaryOverrideAll,
+          throwOnResourceConflict);
     } catch (IOException e) {
       throw MergingException.wrapException(e);
     }
@@ -242,15 +260,14 @@ class AndroidDataMerger {
       ParsedAndroidData direct,
       ParsedAndroidData parsedPrimary,
       Path primaryManifest,
-      boolean allowPrimaryOverrideAll)
-      throws MergingException {
+      boolean allowPrimaryOverrideAll,
+      boolean throwOnResourceConflict) {
     try {
       // Create the builders for the final parsed data.
       final ParsedAndroidData.Builder primaryBuilder = ParsedAndroidData.Builder.newBuilder();
       final ParsedAndroidData.Builder transitiveBuilder = ParsedAndroidData.Builder.newBuilder();
       final KeyValueConsumers transitiveConsumers = transitiveBuilder.consumers();
       final KeyValueConsumers primaryConsumers = primaryBuilder.consumers();
-
       final Set<MergeConflict> conflicts = new HashSet<>();
 
       // Find all internal conflicts.
@@ -286,7 +303,7 @@ class AndroidDataMerger {
         if (allowPrimaryOverrideAll && parsedPrimary.containsOverwritable(entry.getKey())) {
           continue;
         }
-        // If a transitive value is in the direct map report a conflict, as it is commonly
+        // If a transitive value is in the direct map, report a conflict, as it is commonly
         // unintentional.
         if (direct.containsOverwritable(entry.getKey())) {
           conflicts.add(direct.foundResourceConflict(entry.getKey(), entry.getValue()));
@@ -367,8 +384,11 @@ class AndroidDataMerger {
           }
         }
         if (!messages.isEmpty()) {
-          // TODO(corysmith): Turn these into errors.
-          logger.warning(Joiner.on("").join(messages));
+          String conflictMessage = Joiner.on("").join(messages);
+          if (throwOnResourceConflict) {
+            throw MergeConflictException.withMessage(conflictMessage);
+          }
+          logger.warning(conflictMessage);
         }
       }
       return UnwrittenMergedAndroidData.of(
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 8fc6075382..eb7fd70711 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
@@ -64,8 +64,8 @@ public class AndroidResourceMerger {
       final VariantType type,
       @Nullable final Path symbolsOut,
       @Nullable AndroidResourceClassWriter rclassWriter,
-      AndroidDataDeserializer deserializer)
-      throws MergingException {
+      AndroidDataDeserializer deserializer,
+      boolean throwOnResourceConflict) {
     Stopwatch timer = Stopwatch.createStarted();
     final ListeningExecutorService executorService =
         MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(15));
@@ -78,7 +78,8 @@ public class AndroidResourceMerger {
               primary,
               primaryManifest,
               type != VariantType.LIBRARY,
-              deserializer);
+              deserializer,
+              throwOnResourceConflict);
       timer.reset().start();
       if (symbolsOut != null) {
         AndroidDataSerializer serializer = AndroidDataSerializer.create();
@@ -114,14 +115,19 @@ public class AndroidResourceMerger {
       ParsedAndroidData primary,
       Path primaryManifest,
       boolean allowPrimaryOverrideAll,
-      AndroidDataDeserializer deserializer)
-      throws MergingException {
+      AndroidDataDeserializer deserializer,
+      boolean throwOnResourceConflict) {
     Stopwatch timer = Stopwatch.createStarted();
     try {
       AndroidDataMerger merger =
           AndroidDataMerger.createWithPathDeduplictor(executorService, deserializer);
       return merger.loadAndMerge(
-          transitive, direct, primary, primaryManifest, allowPrimaryOverrideAll);
+          transitive,
+          direct,
+          primary,
+          primaryManifest,
+          allowPrimaryOverrideAll,
+          throwOnResourceConflict);
     } finally {
       logger.fine(String.format("merge finished in %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
     }
@@ -140,8 +146,8 @@ public class AndroidResourceMerger {
       @Nullable final PngCruncher cruncher,
       final VariantType type,
       @Nullable final Path symbolsOut,
-      final List<String> filteredResources)
-      throws MergingException {
+      final List<String> filteredResources,
+      boolean throwOnResourceConflict) {
     try {
       final ParsedAndroidData parsedPrimary = ParsedAndroidData.from(primary);
       return mergeData(
@@ -155,7 +161,8 @@ public class AndroidResourceMerger {
           type,
           symbolsOut,
           null /* rclassWriter */,
-          AndroidDataDeserializer.withFilteredResources(filteredResources));
+          AndroidDataDeserializer.withFilteredResources(filteredResources),
+          throwOnResourceConflict);
     } catch (IOException e) {
       throw MergingException.wrapException(e);
     }
@@ -175,8 +182,8 @@ public class AndroidResourceMerger {
       @Nullable final PngCruncher cruncher,
       final VariantType type,
       @Nullable final Path symbolsOut,
-      @Nullable final AndroidResourceClassWriter rclassWriter)
-      throws MergingException {
+      @Nullable final AndroidResourceClassWriter rclassWriter,
+      boolean throwOnResourceConflict) {
     final ParsedAndroidData.Builder primaryBuilder = ParsedAndroidData.Builder.newBuilder();
     final AndroidDataDeserializer deserializer = AndroidDataDeserializer.create();
     primary.deserialize(deserializer, primaryBuilder.consumers());
@@ -192,7 +199,8 @@ public class AndroidResourceMerger {
         type,
         symbolsOut,
         rclassWriter,
-        deserializer);
+        deserializer,
+        throwOnResourceConflict);
   }
 }
 
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMergingAction.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMergingAction.java
index 9459ae6d99..4506fae73e 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMergingAction.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceMergingAction.java
@@ -21,6 +21,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Strings;
 import com.google.common.io.Files;
+import com.google.devtools.build.android.AndroidDataMerger.MergeConflictException;
 import com.google.devtools.build.android.AndroidResourceMerger.MergingException;
 import com.google.devtools.build.android.AndroidResourceProcessor.AaptConfigOptions;
 import com.google.devtools.build.android.Converters.ExistingPathConverter;
@@ -180,6 +181,14 @@ public class AndroidResourceMergingAction {
       help = "Path to where data binding's layout info output should be written."
     )
     public Path dataBindingInfoOut;
+
+    @Option(name = "throwOnResourceConflict",
+        defaultValue = "false",
+        category = "config",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help = "If passed, resource merge conflicts will be treated as errors instead of warnings")
+    public boolean throwOnResourceConflict;
   }
 
   public static void main(String[] args) throws Exception {
@@ -223,7 +232,8 @@ public class AndroidResourceMergingAction {
               new StubPngCruncher(),
               packageType,
               options.symbolsBinOut,
-              resourceClassWriter);
+              resourceClassWriter,
+              options.throwOnResourceConflict);
 
       logger.fine(String.format("Merging finished at %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
 
@@ -268,6 +278,9 @@ public class AndroidResourceMergingAction {
             String.format(
                 "Create resources.zip finished at %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
       }
+    } catch (MergeConflictException e) {
+      logger.log(Level.SEVERE, e.getMessage());
+      System.exit(1);
     } catch (MergingException e) {
       logger.log(Level.SEVERE, "Error during merging resources", e);
       throw e;
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java
index 13eb0f5c90..0dfd21e5f5 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java
@@ -22,6 +22,7 @@ import com.android.ide.common.process.LoggedProcessOutputHandler;
 import com.android.utils.StdLogger;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.android.AndroidDataMerger.MergeConflictException;
 import com.google.devtools.build.android.AndroidResourceMerger.MergingException;
 import com.google.devtools.build.android.AndroidResourceProcessor.AaptConfigOptions;
 import com.google.devtools.build.android.AndroidResourceProcessor.FlagAaptOptions;
@@ -295,6 +296,14 @@ public class AndroidResourceProcessingAction {
       help = "A list of resources that were filtered out in analysis."
     )
     public List<String> prefilteredResources;
+
+    @Option(name = "throwOnResourceConflict",
+        defaultValue = "false",
+        category = "config",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help = "If passed, resource merge conflicts will be treated as errors instead of warnings")
+    public boolean throwOnResourceConflict;
   }
 
   private static AaptConfigOptions aaptConfigOptions;
@@ -346,7 +355,8 @@ public class AndroidResourceProcessingAction {
               selectPngCruncher(),
               options.packageType,
               options.symbolsOut,
-              options.prefilteredResources);
+              options.prefilteredResources,
+              options.throwOnResourceConflict);
 
       logger.fine(String.format("Merging finished at %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
 
@@ -429,6 +439,9 @@ public class AndroidResourceProcessingAction {
       }
       logger.fine(
           String.format("Packaging finished at %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
+    } catch (MergeConflictException e) {
+      logger.severe(e.getMessage());
+      System.exit(1);
     } catch (MergingException e) {
       logger.log(java.util.logging.Level.SEVERE, "Error during merging resources", e);
       throw e;