Introduce hidden configuration flag to list dexopts used by DexFileMerger tool during incremental dexing

RELNOTES: none

PiperOrigin-RevId: 152838197

3 files changed, 87 insertions, 44 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index 65c1669fe1..655d546568 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -22,7 +22,6 @@ import static com.google.devtools.build.lib.analysis.OutputGroupProvider.INTERNA
 import com.google.common.base.Function;
 import com.google.common.base.Functions;
 import com.google.common.base.Optional;
-import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -1369,11 +1368,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
             : AndroidCommon.getAndroidConfig(ruleContext).getIncrementalDexingBinaries();
     if (!result.isEmpty()) {
       Iterable<String> blacklistedDexopts =
-          Iterables.filter(
-              dexopts,
-              new FlagMatcher(AndroidCommon
-                  .getAndroidConfig(ruleContext)
-                  .getTargetDexoptsThatPreventIncrementalDexing()));
+          DexArchiveAspect.blacklistedDexopts(ruleContext, dexopts);
       if (!Iterables.isEmpty(blacklistedDexopts)) {
         // target's dexopts include flags blacklisted with --non_incremental_per_target_dexopts. If
         // incremental_dexing attribute is explicitly set for this target then we'll warn and
@@ -1412,15 +1407,12 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
         .addInputArgument(inputJar)
         .addArgument("--output")
         .addOutputArgument(classesDex)
-        .addArguments(DexArchiveAspect.incrementalDexopts(ruleContext, dexopts))
+        .addArguments(DexArchiveAspect.mergerDexopts(ruleContext, dexopts))
         .addArgument("--multidex=" + multidexStrategy)
         .setMnemonic("DexMerger")
         .setProgressMessage("Assembling dex files into " + classesDex.prettyPrint());
     if (mainDexList != null) {
       dexmerger.addArgument("--main-dex-list").addInputArgument(mainDexList);
-      if (dexopts.contains("--minimal-main-dex")) {
-        dexmerger.addArgument("--minimal-main-dex");
-      }
     }
     ruleContext.registerAction(dexmerger.build(ruleContext));
   }
@@ -1814,22 +1806,4 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
     return ruleContext.getUniqueDirectoryArtifact("_dx", baseName,
         ruleContext.getBinOrGenfilesDirectory());
   }
-
-  private static class FlagMatcher implements Predicate<String> {
-    private final ImmutableList<String> matching;
-
-    FlagMatcher(ImmutableList<String> matching) {
-      this.matching = matching;
-    }
-
-    @Override
-    public boolean apply(String input) {
-      for (String match : matching) {
-        if (input.contains(match)) {
-          return true;
-        }
-      }
-      return false;
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
index 37d3bc36a7..cf9c618d07 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
@@ -318,9 +318,11 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
       help = "Do not use.")
     public boolean incrementalDexingErrorOnMissedJars;
 
+    // Do not use on the command line.
+    // This flag is intended to be updated as we add supported flags to the incremental dexing tools
     @Option(name = "non_incremental_per_target_dexopts",
         converter = Converters.CommaSeparatedOptionListConverter.class,
-        defaultValue = "--set-max-idx-number,--positions",
+        defaultValue = "--positions",
         category = "semantics",
         help = "dx flags that that prevent incremental dexing for binary targets that list any of "
             + "the flags listed here in their 'dexopts' attribute, which are ignored with "
@@ -336,9 +338,19 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
         converter = Converters.CommaSeparatedOptionListConverter.class,
         defaultValue = "--no-optimize,--no-locals",
         category = "hidden",
-        help = "dx flags supported in incremental dexing.")
+        help = "dx flags supported when converting Jars to dex archives incrementally.")
     public List<String> dexoptsSupportedInIncrementalDexing;
 
+    // Do not use on the command line.
+    // This flag is intended to be updated as we add supported flags to the incremental dexing tools
+    // TODO(b/31711689): remove --no-optimize and --no-locals as DexFileMerger no longer needs them
+    @Option(name = "dexopts_supported_in_dexmerger",
+        converter = Converters.CommaSeparatedOptionListConverter.class,
+        defaultValue = "--no-optimize,--no-locals,--minimal-main-dex,--set-max-idx-number",
+        category = "hidden",
+        help = "dx flags supported in tool that merges dex archives into final classes.dex files.")
+    public List<String> dexoptsSupportedInDexMerger;
+
     @Option(
       name = "experimental_android_rewrite_dexes_with_rex",
       defaultValue = "false",
@@ -455,6 +467,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
       host.incrementalDexingErrorOnMissedJars = incrementalDexingErrorOnMissedJars;
       host.nonIncrementalPerTargetDexopts = nonIncrementalPerTargetDexopts;
       host.dexoptsSupportedInIncrementalDexing = dexoptsSupportedInIncrementalDexing;
+      host.dexoptsSupportedInDexMerger = dexoptsSupportedInDexMerger;
       host.manifestMerger = manifestMerger;
       return host;
     }
@@ -502,6 +515,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
   private final boolean incrementalDexingErrorOnMissedJars;
   private final ImmutableList<String> dexoptsSupportedInIncrementalDexing;
   private final ImmutableList<String> targetDexoptsThatPreventIncrementalDexing;
+  private final ImmutableList<String> dexoptsSupportedInDexMerger;
   private final boolean desugarJava8;
   private final boolean useRexToCompressDexFiles;
   private final boolean allowAndroidLibraryDepsWithoutSrcs;
@@ -515,7 +529,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
   private final boolean useSingleJarForProguardLibraryJars;
   private final boolean compressJavaResources;
 
-  AndroidConfiguration(Options options, Label androidSdk) {
+  AndroidConfiguration(Options options, Label androidSdk) throws InvalidConfigurationException {
     this.sdk = androidSdk;
     this.incrementalNativeLibs = options.incrementalNativeLibs;
     this.cpu = options.cpu;
@@ -531,6 +545,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
         ImmutableList.copyOf(options.dexoptsSupportedInIncrementalDexing);
     this.targetDexoptsThatPreventIncrementalDexing =
         ImmutableList.copyOf(options.nonIncrementalPerTargetDexopts);
+    this.dexoptsSupportedInDexMerger = ImmutableList.copyOf(options.dexoptsSupportedInDexMerger);
     this.desugarJava8 = options.desugarJava8;
     this.allowAndroidLibraryDepsWithoutSrcs = options.allowAndroidLibraryDepsWithoutSrcs;
     this.useAndroidResourceShrinking = options.useAndroidResourceShrinking
@@ -544,6 +559,12 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
     this.useRexToCompressDexFiles = options.useRexToCompressDexFiles;
     this.resourceFilter = options.resourceFilter;
     this.compressJavaResources = options.compressJavaResources;
+
+    if (!dexoptsSupportedInIncrementalDexing.contains("--no-locals")) {
+      // TODO(bazel-team): Still needed? See DexArchiveAspect
+      throw new InvalidConfigurationException("--dexopts_supported_in_incremental_dexing must "
+          + "include '--no-locals' to enable coverage builds");
+    }
   }
 
   public String getCpu() {
@@ -588,6 +609,13 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
   }
 
   /**
+   * dx flags supported in dexmerger actions.
+   */
+  public ImmutableList<String> getDexoptsSupportedInDexMerger() {
+    return dexoptsSupportedInDexMerger;
+  }
+
+  /**
    * Regardless of {@link #getIncrementalDexingBinaries}, incremental dexing must not be used for
    * binaries that list any of these flags in their {@code dexopts} attribute.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java
index b3f5cc55d5..91aac4f20f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java
@@ -23,6 +23,7 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1;
 import com.google.common.base.Function;
 import com.google.common.base.Functions;
 import com.google.common.base.Joiner;
+import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -60,9 +61,9 @@ import com.google.devtools.build.lib.rules.proto.ProtoLangToolchainProvider;
 import com.google.devtools.build.lib.rules.proto.ProtoSourcesProvider;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.TreeSet;
 
 /**
  * Aspect to {@link DexArchiveProvider build .dex Archives} from Jars.
@@ -442,9 +443,7 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu
 
   private static Set<Set<String>> aspectDexopts(RuleContext ruleContext) {
     return Sets.powerSet(
-        normalizeDexopts(
-            ruleContext,
-            getAndroidConfig(ruleContext).getDexoptsSupportedInIncrementalDexing()));
+        normalizeDexopts(getAndroidConfig(ruleContext).getDexoptsSupportedInIncrementalDexing()));
   }
 
   /**
@@ -455,25 +454,67 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu
    */
   static ImmutableSet<String> incrementalDexopts(RuleContext ruleContext,
       Iterable<String> tokenizedDexopts) {
+    if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
+      // TODO(bazel-team): Still needed? No longer done in AndroidCommon.createDexAction
+      tokenizedDexopts = Iterables.concat(tokenizedDexopts, ImmutableList.of("--no-locals"));
+    }
     return normalizeDexopts(
-        ruleContext,
         Iterables.filter(
             tokenizedDexopts,
+            // dexopts have to match exactly since aspect only creates archives for listed ones
             Predicates.in(getAndroidConfig(ruleContext).getDexoptsSupportedInIncrementalDexing())));
   }
 
-  private static ImmutableSet<String> normalizeDexopts(
-      RuleContext ruleContext, Iterable<String> tokenizedDexopts) {
+  /**
+   * Returns the subset of the given dexopts that are blacklisted from using incremental dexing
+   * by default.
+   */
+  static Iterable<String> blacklistedDexopts(
+      RuleContext ruleContext, List<String> dexopts) {
+    return Iterables.filter(
+        dexopts,
+        new FlagMatcher(
+            getAndroidConfig(ruleContext).getTargetDexoptsThatPreventIncrementalDexing()));
+  }
+
+  /**
+   * Derives options to use in DexFileMerger actions from the given context and dx flags, where the
+   * latter typically come from a {@code dexopts} attribute on a top-level target.
+   */
+  static ImmutableSet<String> mergerDexopts(RuleContext ruleContext,
+      Iterable<String> tokenizedDexopts) {
+    // We don't need an ordered set but might as well.  Note we don't need to worry about coverage
+    // builds since the merger doesn't use --no-locals.
+    return normalizeDexopts(
+        Iterables.filter(
+            tokenizedDexopts,
+            new FlagMatcher(getAndroidConfig(ruleContext).getDexoptsSupportedInDexMerger())));
+  }
+
+  private static ImmutableSet<String> normalizeDexopts(Iterable<String> tokenizedDexopts) {
     // Use TreeSet to drop duplicates and get fixed (sorted) order.  Fixed order is important so
     // we generate one dex archive per set of flag in create() method, regardless of how those flags
     // are listed in all the top-level targets being built.
-    Set<String> args = new TreeSet<>();
-    if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
-      // Match what we do in AndroidCommon.createDexAction
-      args.add("--nolocals"); // TODO(bazel-team): Still needed? See createDexAction
+    return ImmutableSet.copyOf(
+        Sets.newTreeSet(Iterables.transform(tokenizedDexopts, FlagConverter.DX_TO_DEXBUILDER)));
+  }
+
+  private static class FlagMatcher implements Predicate<String> {
+    private final ImmutableList<String> matching;
+
+    FlagMatcher(ImmutableList<String> matching) {
+      this.matching = matching;
+    }
+
+    @Override
+    public boolean apply(String input) {
+      for (String match : matching) {
+        if (input.contains(match)) {
+          return true;
+        }
+      }
+      return false;
     }
-    Iterables.addAll(args, Iterables.transform(tokenizedDexopts, FlagConverter.DX_TO_DEXBUILDER));
-    return ImmutableSet.copyOf(args);
   }
 
   private enum FlagConverter implements Function<String, String> {