Enable incremental dexing by default.

Fixes https://github.com/bazelbuild/bazel/issues/3045.

RELNOTES: Enable --incremental_dexing for Android builds by default. Note that some dexopts are incompatible with incremental dexing, including --force-jumbo.
PiperOrigin-RevId: 160650101

5 files changed, 96 insertions, 95 deletions

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 5cb88d5802..117545aca7 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
@@ -340,7 +340,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
 
     @Option(
       name = "incremental_dexing",
-      defaultValue = "false",
+      defaultValue = "true",
       category = "semantics",
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
       effectTags = {OptionEffectTag.UNKNOWN},
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
index 35f940f6b5..a52a1a60fb 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -103,6 +103,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testMainDexProguardSpecs() throws Exception {
+    useConfiguration("--noincremental_dexing");
     ConfiguredTarget ct = scratchConfiguredTarget("java/a", "a",
         "android_binary(",
         "    name = 'a',",
@@ -458,7 +459,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
     Action shardAction =
         getGeneratingAction(getBinArtifact("_dx/top/classes.jar", topTarget));
-    for (String basename : ActionsTestUtil.baseArtifactNames(shardAction.getInputs())) {
+    for (String basename : ActionsTestUtil.baseArtifactNames(getNonToolInputs(shardAction))) {
       // all jars are converted to dex archives
       assertThat(!basename.contains(".jar") || basename.endsWith(".jar.dex.zip"))
           .named(basename).isTrue();
@@ -516,7 +517,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
     Action shardAction =
         getGeneratingAction(getBinArtifact("_dx/top/classes.jar", topTarget));
-    for (String basename : ActionsTestUtil.baseArtifactNames(shardAction.getInputs())) {
+    for (String basename : ActionsTestUtil.baseArtifactNames(getNonToolInputs(shardAction))) {
       // all jars are converted to dex archives
       assertThat(!basename.contains(".jar") || basename.endsWith(".jar.dex.zip"))
           .named(basename).isTrue();
@@ -556,7 +557,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
     Action shardAction =
         getGeneratingAction(getBinArtifact("_dx/top/classes.jar", topTarget));
-    for (Artifact input : shardAction.getInputs()) {
+    for (Artifact input : getNonToolInputs(shardAction)) {
       String basename = input.getFilename();
       // all jars are converted to dex archives
       assertThat(!basename.contains(".jar") || basename.endsWith(".jar.dex.zip"))
@@ -573,7 +574,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
     // *all* expected input dex archives.
     assertThat(
             Iterables.filter(
-                ActionsTestUtil.baseArtifactNames(shardAction.getInputs()),
+                ActionsTestUtil.baseArtifactNames(getNonToolInputs(shardAction)),
                 Predicates.containsPattern("\\.jar")))
         .containsExactly(
             // top's dex archives
@@ -606,9 +607,9 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
     assertNoEvents();
     Action shardAction = getGeneratingAction(getBinArtifact("_dx/top/shard1.jar", topTarget));
     assertThat(
-        Iterables.filter(
-            ActionsTestUtil.baseArtifactNames(shardAction.getInputs()),
-            Predicates.containsPattern("\\.jar\\.dex\\.zip")))
+            Iterables.filter(
+                ActionsTestUtil.baseArtifactNames(getNonToolInputs(shardAction)),
+                Predicates.containsPattern("\\.jar\\.dex\\.zip")))
         .isEmpty(); // no dex archives are used
   }
 
@@ -934,17 +935,20 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testDexopts() throws Exception {
+    useConfiguration("--noincremental_dexing");
     checkDexopts("[ '--opt1', '--opt2' ]", ImmutableList.of("--opt1", "--opt2"));
   }
 
   @Test
   public void testDexoptsTokenization() throws Exception {
+    useConfiguration("--noincremental_dexing");
     checkDexopts("[ '--opt1', '--opt2 tokenized' ]",
         ImmutableList.of("--opt1", "--opt2", "tokenized"));
   }
 
   @Test
   public void testDexoptsMakeVariableSubstitution() throws Exception {
+    useConfiguration("--noincremental_dexing");
     checkDexopts("[ '--opt1', '$(COMPILATION_MODE)' ]", ImmutableList.of("--opt1", "fastbuild"));
   }
 
@@ -1992,6 +1996,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testDexShardingLegacyStructure() throws Exception {
+    useConfiguration("--noincremental_dexing");
     scratch.file("java/a/BUILD",
         "android_binary(",
         "    name='a',",
@@ -2005,6 +2010,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testDexShardingNativeStructure() throws Exception {
+    useConfiguration("--noincremental_dexing");
     scratch.file("java/a/BUILD",
         "android_binary(",
         "    name='a',",
@@ -2018,7 +2024,8 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testDexShardingNativeStructure_withDesugaring() throws Exception {
-    useConfiguration("--experimental_desugar_for_android");
+    useConfiguration("--experimental_desugar_for_android", "--noincremental_dexing");
+
     scratch.file("java/a/BUILD",
         "android_binary(",
         "    name='a',",
@@ -2179,6 +2186,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testMainDexAaptGenerationSupported() throws Exception {
+    useConfiguration("--android_sdk=//sdk:sdk", "--noincremental_dexing");
     scratch.file(
         "sdk/BUILD",
         "android_sdk(",
@@ -2205,7 +2213,6 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
         "    manifest = 'AndroidManifest.xml',",
         "    multidex = 'legacy')");
 
-    useConfiguration("--android_sdk=//sdk:sdk");
     ConfiguredTarget a = getConfiguredTarget("//java/a:a");
     Artifact intermediateJar = artifactByPath(ImmutableList.of(getCompressedUnsignedApk(a)),
         ".apk", ".dex.zip", ".dex.zip", "main_dex_list.txt", "_intermediate.jar");
@@ -2219,6 +2226,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testMainDexGenerationWithoutProguardMap() throws Exception {
+    useConfiguration("--noincremental_dexing");
     scratchConfiguredTarget("java/foo", "abin",
         "android_binary(",
         "    name = 'abin',",
@@ -2601,7 +2609,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testFeatureFlagsAttributeSetsSelectInDependency() throws Exception {
-    useConfiguration("--experimental_dynamic_configs=on");
+    useConfiguration("--experimental_dynamic_configs=notrim");
     scratch.file(
         "java/com/foo/BUILD",
         "config_feature_flag(",
@@ -2651,7 +2659,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testFeatureFlagsAttributeSetsSelectInBinary() throws Exception {
-    useConfiguration("--experimental_dynamic_configs=on");
+    useConfiguration("--experimental_dynamic_configs=notrim");
     scratch.file(
         "java/com/foo/BUILD",
         "config_feature_flag(",
@@ -2764,7 +2772,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
   public void testFeatureFlagsAttributeFailsAnalysisIfFlagIsAliased()
       throws Exception {
     reporter.removeHandler(failFastHandler);
-    useConfiguration("--experimental_dynamic_configs=on");
+    useConfiguration("--experimental_dynamic_configs=notrim");
     scratch.file(
         "java/com/foo/BUILD",
         "config_feature_flag(",
@@ -2792,7 +2800,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
 
   @Test
   public void testFeatureFlagsAttributeSetsFeatureFlagProviderValues() throws Exception {
-    useConfiguration("--experimental_dynamic_configs=on");
+    useConfiguration("--experimental_dynamic_configs=notrim");
     scratch.file(
         "java/com/foo/reader.bzl",
         "def _impl(ctx):",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java
index 4e9d40a300..3821b2d37b 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java
@@ -22,6 +22,7 @@ import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import com.google.common.truth.Truth;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
@@ -42,6 +43,7 @@ import com.google.devtools.build.lib.util.Preconditions;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 import javax.annotation.Nullable;
 
 /** Common methods shared between Android related {@link BuildViewTestCase}s. */
@@ -262,4 +264,9 @@ public abstract class AndroidBuildViewTestCase extends BuildViewTestCase {
       Truth.assertThat(actualPaths.build()).containsAllIn(expectedPaths);
     }
   }
+
+  protected static Set<Artifact> getNonToolInputs(Action action) {
+    return Sets.difference(
+        ImmutableSet.copyOf(action.getInputs()), ImmutableSet.copyOf(action.getTools()));
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidMultidexBaseTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidMultidexBaseTest.java
index 272e4a90e1..93e273c923 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidMultidexBaseTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidMultidexBaseTest.java
@@ -58,10 +58,6 @@ public class AndroidMultidexBaseTest extends BuildViewTestCase {
     ConfiguredTarget binary = getConfiguredTarget(ruleLabel);
     Set<Artifact> artifacts = actionsTestUtil().artifactClosureOf(getFilesToBuild(binary));
 
-    // Always created:
-    Artifact intermediateDexOutput =
-        getFirstArtifactEndingWith(artifacts, "intermediate_classes.dex.zip");
-    assertThat(intermediateDexOutput).isNotNull();
     Artifact finalDexOutput = getFirstArtifactEndingWith(artifacts, "/classes.dex.zip");
     assertThat(finalDexOutput).isNotNull();
 
@@ -73,13 +69,13 @@ public class AndroidMultidexBaseTest extends BuildViewTestCase {
       // First action: check that the stripped jar is generated through Proguard.
       AndroidSdkProvider sdk = AndroidSdkProvider.fromRuleContext(getRuleContext(binary));
       assertThat(strippedJar).isNotNull();
-      SpawnAction stripAction = (SpawnAction) getGeneratingAction(strippedJar);
+      SpawnAction stripAction = getGeneratingSpawnAction(strippedJar);
       assertThat(stripAction.getCommandFilename())
           .isEqualTo(sdk.getProguard().getExecutable().getExecPathString());
 
       // Second action: The dexer consumes the stripped jar to create the main dex class list.
       assertThat(mainDexList).isNotNull();
-      SpawnAction mainDexAction = (SpawnAction) getGeneratingAction(mainDexList);
+      SpawnAction mainDexAction = getGeneratingSpawnAction(mainDexList);
       assertThat(mainDexAction.getArguments()).containsAllOf(
           mainDexList.getExecPathString(),
           strippedJar.getExecPathString()).inOrder();
@@ -93,57 +89,45 @@ public class AndroidMultidexBaseTest extends BuildViewTestCase {
       assertThat(mainDexList).isNull();
     }
 
-    // Third action: the dexer command consumes the main dex class list to generate the intermediate
-    // dex output.
-    SpawnAction dexAction = (SpawnAction) getGeneratingAction(intermediateDexOutput);
-    ImmutableList.Builder<String> argBuilder = ImmutableList.builder();
-    argBuilder.add("--dex");
-    if (multidexMode == MultidexMode.OFF) {
-      argBuilder.add("--num-threads=5");
-    } else {
-      argBuilder.add("--multi-dex");
-
-      if (multidexMode == MultidexMode.LEGACY || multidexMode == MultidexMode.MANUAL_MAIN_DEX) {
-        argBuilder.add("--main-dex-list=" + mainDexList.getExecPathString());
-      }
-    }
-
-    argBuilder.add("--output=" + intermediateDexOutput.getExecPathString());
-
-    assertThat(dexAction.getArguments()).containsAllIn(argBuilder.build()).inOrder();
-
-    // Final action: the SingleJar command that consumes the intermediate dex out to produce the
-    // final dex out.
-    SpawnAction singleJarAction = getGeneratingSpawnAction(finalDexOutput);
-    assertThat(singleJarAction.getArguments())
-        .containsAllOf(
-            "--exclude_build_data",
-            "--dont_change_compression",
-            "--sources",
-            intermediateDexOutput.getExecPathString(),
+    Artifact dexMerger = getFirstArtifactEndingWith(artifacts, "dexmerger");
+    Artifact dexMergerInput = getFirstArtifactEndingWith(artifacts, "classes.jar");
+    SpawnAction dexMergerAction = getGeneratingSpawnAction(finalDexOutput);
+    ImmutableList.Builder<String> argsBuilder = ImmutableList.<String>builder()
+        .add(
+            dexMerger.getExecPathString(),
+            "--input",
+            dexMergerInput.getExecPathString(),
             "--output",
-            finalDexOutput.getExecPathString(),
-            "--include_prefixes",
-            "classes")
-        .inOrder();
+            finalDexOutput.getExecPathString());
+    if (multidexMode != MultidexMode.OFF) {
+      argsBuilder.add("--multidex=minimal");
+    }
+    if (multidexMode == MultidexMode.LEGACY || multidexMode == MultidexMode.MANUAL_MAIN_DEX) {
+      argsBuilder.add("--main-dex-list", mainDexList.getExecPathString());
+    }
+    assertThat(dexMergerAction.getArguments()).containsExactlyElementsIn(argsBuilder.build()).inOrder();
   }
-  
+
   /**
-   * Internal helper method: given an android_binary rule label, check that it builds
-   * in non-multidex mode.
+   * Internal helper method: given an android_binary rule label, check that the dex merger
+   * runs is invoked with {@code --multidex=off}.
    */
   protected void internalTestNonMultidexBuildStructure(String ruleLabel) throws Exception {
-
     ConfiguredTarget binary = getConfiguredTarget(ruleLabel);
     Set<Artifact> artifacts = actionsTestUtil().artifactClosureOf(getFilesToBuild(binary));
-    Artifact dexOutput = getFirstArtifactEndingWith(artifacts, "classes.dex");
-    SpawnAction dexAction = (SpawnAction) getGeneratingAction(dexOutput);
+    Artifact dexMerger = getFirstArtifactEndingWith(artifacts, "dexmerger");
+    Artifact dexInput = getFirstArtifactEndingWith(artifacts, "classes.jar");
+    Artifact dexOutput = getFirstArtifactEndingWith(artifacts, "classes.dex.zip");
+    SpawnAction dexAction = getGeneratingSpawnAction(dexOutput);
 
-    assertThat(dexAction.getArguments()).doesNotContain("--multi-dex");
-    assertThat(dexAction.getArguments()).containsAllOf(
-            "--dex",
-            "--num-threads=5",
-            "--output=" + dexOutput.getExecPathString()).inOrder();
+    assertThat(dexAction.getArguments())
+        .containsAllOf(
+            dexMerger.getExecPathString(),
+            "--input",
+            dexInput.getExecPathString(),
+            "--output",
+            dexOutput.getExecPathString(),
+            "--multidex=off").inOrder();
   }
 }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
index fa102db528..e8649a71f8 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
@@ -287,38 +287,6 @@ public class ConfigurationsForTargetsTest extends AnalysisTestCase {
     assertThat(toolDep.getConfiguration().isHostConfiguration()).isTrue();
   }
 
-  @Test
-  public void splitDeps() throws Exception {
-    scratch.file(
-        "java/a/BUILD",
-        "cc_library(name = 'lib', srcs = ['lib.cc'])",
-        "android_binary(name='a', manifest = 'AndroidManifest.xml', deps = [':lib'])");
-    useConfiguration("--fat_apk_cpu=k8,armeabi-v7a");
-    List<ConfiguredTarget> deps = getConfiguredDeps("//java/a:a", "deps");
-    assertThat(deps).hasSize(2);
-    ConfiguredTarget dep1 = deps.get(0);
-    ConfiguredTarget dep2 = deps.get(1);
-    assertThat(
-        ImmutableList.<String>of(
-            dep1.getConfiguration().getCpu(),
-            dep2.getConfiguration().getCpu()))
-        .containsExactly("armeabi-v7a", "k8");
-    // We don't care what order split deps are listed, but it must be deterministic. Static and
-    // dynamic configurations happen to apply different orders (static: same order as the split
-    // transition definition, dynamic: ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING). That's
-    // okay because of the "we don't care what order" principle. The primary value of this test is
-    // to check against the new dynamic code, which will soon replace the static code anyway. And
-    // the static code is already well-tested through all other Blaze tests. And checking its order
-    // would be a lot uglier. So we only worry about the dynamic case here.
-    if (getTargetConfiguration().useDynamicConfigurations()) {
-      assertThat(
-          ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING.compare(
-              Dependency.withConfiguration(dep1.getLabel(), dep1.getConfiguration()),
-              Dependency.withConfiguration(dep2.getLabel(), dep2.getConfiguration())))
-          .isLessThan(0);
-    }
-  }
-
   /** Runs the same test with untrimmed dynamic configurations. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)
@@ -327,5 +295,39 @@ public class ConfigurationsForTargetsTest extends AnalysisTestCase {
     protected FlagBuilder defaultFlags() {
       return super.defaultFlags().with(Flag.DYNAMIC_CONFIGURATIONS_NOTRIM);
     }
+
+    // This test does not pass with trimming because android_binary applies an aspect and aspects
+    // are not yet correctly supported with trimming.
+    @Test
+    public void splitDeps() throws Exception {
+      scratch.file(
+          "java/a/BUILD",
+          "cc_library(name = 'lib', srcs = ['lib.cc'])",
+          "android_binary(name='a', manifest = 'AndroidManifest.xml', deps = [':lib'])");
+      useConfiguration("--fat_apk_cpu=k8,armeabi-v7a");
+      List<ConfiguredTarget> deps = getConfiguredDeps("//java/a:a", "deps");
+      assertThat(deps).hasSize(2);
+      ConfiguredTarget dep1 = deps.get(0);
+      ConfiguredTarget dep2 = deps.get(1);
+      assertThat(
+          ImmutableList.<String>of(
+              dep1.getConfiguration().getCpu(),
+              dep2.getConfiguration().getCpu()))
+          .containsExactly("armeabi-v7a", "k8");
+      // We don't care what order split deps are listed, but it must be deterministic. Static and
+      // dynamic configurations happen to apply different orders (static: same order as the split
+      // transition definition, dynamic: ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING).
+      // That's okay because of the "we don't care what order" principle. The primary value of this
+      // test is to check against the new dynamic code, which will soon replace the static code
+      // anyway. And the static code is already well-tested through all other Blaze tests. And
+      // checking its order would be a lot uglier. So we only worry about the dynamic case here.
+      if (getTargetConfiguration().useDynamicConfigurations()) {
+        assertThat(
+            ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING.compare(
+                Dependency.withConfiguration(dep1.getLabel(), dep1.getConfiguration()),
+                Dependency.withConfiguration(dep2.getLabel(), dep2.getConfiguration())))
+            .isLessThan(0);
+      }
+    }
   }
 }