Stop collapsing NestedSet of deps in RClassGenerator

This collapsing should be avoided when possible, and punted on until execution when needed.

RELNOTES: none
PiperOrigin-RevId: 195695290

2 files changed, 66 insertions, 50 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java
index 016aee5866..01dc6f549b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java
@@ -13,26 +13,22 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.android;
 
-import com.google.common.base.Function;
 import com.google.common.base.Strings;
-import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ParamFileInfo;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.Builder;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.rules.android.AndroidConfiguration.AndroidAaptVersion;
+import com.google.devtools.build.lib.rules.android.AndroidDataConverter.JoinerType;
 import java.util.ArrayList;
 import java.util.List;
-import javax.annotation.Nullable;
+import java.util.function.Function;
 
 /** Builds up the spawn action for $android_rclass_generator. */
 public class RClassGeneratorActionBuilder {
@@ -77,7 +73,7 @@ public class RClassGeneratorActionBuilder {
   }
 
   private void build(Artifact rTxt, ProcessedAndroidManifest manifest) {
-    CustomCommandLine.Builder builder = new CustomCommandLine.Builder();
+    Builder builder = new Builder();
 
     // Set the busybox tool.
     builder.add("--tool").add("GENERATE_BINARY_R").add("--");
@@ -98,19 +94,20 @@ public class RClassGeneratorActionBuilder {
       builder.add("--packageForR", manifest.getPackage());
     }
     if (dependencies != null) {
-      // TODO(corysmith): Remove NestedSet as we are already flattening it.
-      Iterable<ValidatedAndroidData> depResources = dependencies.getResourceContainers();
-      if (!Iterables.isEmpty(depResources)) {
+      if (!dependencies.getResourceContainers().isEmpty()) {
         builder.addAll(
             VectorArg.addBefore("--library")
-                .each(
-                    ImmutableList.copyOf(
-                        Iterables.transform(depResources, chooseDepsToArg(version)))));
-        inputs.addTransitive(
-            NestedSetBuilder.wrap(
-                Order.NAIVE_LINK_ORDER,
-                FluentIterable.from(depResources)
-                    .transformAndConcat(chooseDepsToArtifacts(version))));
+                .each(dependencies.getResourceContainers())
+                .mapped(
+                    AndroidDataConverter.<ValidatedAndroidData>builder(JoinerType.COLON_COMMA)
+                        .with(chooseDepsToArg(version))
+                        .build()));
+        if (version == AndroidAaptVersion.AAPT2) {
+          inputs.addTransitive(dependencies.getTransitiveAapt2RTxt());
+        } else {
+          inputs.addTransitive(dependencies.getTransitiveRTxt());
+        }
+        inputs.addTransitive(dependencies.getTransitiveManifests());
       }
     }
     builder.addExecPath("--classJarOutput", classJarOut);
@@ -134,39 +131,14 @@ public class RClassGeneratorActionBuilder {
             .build(ruleContext));
   }
 
-  private static Artifact chooseRTxt(ValidatedAndroidData container, AndroidAaptVersion version) {
-    return version == AndroidAaptVersion.AAPT2 ? container.getAapt2RTxt() : container.getRTxt();
-  }
-
-  private static Function<ValidatedAndroidData, NestedSet<Artifact>> chooseDepsToArtifacts(
-      final AndroidAaptVersion version) {
-    return new Function<ValidatedAndroidData, NestedSet<Artifact>>() {
-      @Override
-      public NestedSet<Artifact> apply(ValidatedAndroidData container) {
-        NestedSetBuilder<Artifact> artifacts = NestedSetBuilder.naiveLinkOrder();
-        addIfNotNull(chooseRTxt(container, version), artifacts);
-        addIfNotNull(container.getManifest(), artifacts);
-        return artifacts.build();
-      }
-
-      private void addIfNotNull(@Nullable Artifact artifact, NestedSetBuilder<Artifact> artifacts) {
-        if (artifact != null) {
-          artifacts.add(artifact);
-        }
-      }
-    };
-  }
-
   private static Function<ValidatedAndroidData, String> chooseDepsToArg(
       final AndroidAaptVersion version) {
-    return new Function<ValidatedAndroidData, String>() {
-      @Override
-      public String apply(ValidatedAndroidData container) {
-        Artifact rTxt = chooseRTxt(container, version);
-        return (rTxt != null ? rTxt.getExecPath() : "")
-            + ","
-            + (container.getManifest() != null ? container.getManifest().getExecPath() : "");
-      }
+    return container -> {
+      Artifact rTxt =
+          version == AndroidAaptVersion.AAPT2 ? container.getAapt2RTxt() : container.getRTxt();
+      return (rTxt != null ? rTxt.getExecPath() : "")
+          + ","
+          + (container.getManifest() != null ? container.getManifest().getExecPath() : "");
     };
   }
 }
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 1dd3e80eed..3a9bb90190 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
@@ -1959,6 +1959,50 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
   }
 
   @Test
+  public void testUseRClassGeneratorMultipleDeps() throws Exception {
+    scratch.file(
+        "java/r/android/BUILD",
+        "android_library(name = 'lib1',",
+        "                manifest = 'AndroidManifest.xml',",
+        "                resource_files = glob(['res1/**']),",
+        "                )",
+        "android_library(name = 'lib2',",
+        "                manifest = 'AndroidManifest.xml',",
+        "                resource_files = glob(['res2/**']),",
+        "                )",
+        "android_binary(name = 'r',",
+        "               manifest = 'AndroidManifest.xml',",
+        "               resource_files = glob(['res/**']),",
+        "               deps = [':lib1', ':lib2'],",
+        "               )");
+    ConfiguredTargetAndData binary = getConfiguredTargetAndData("//java/r/android:r");
+    Artifact jar = getResourceClassJar(binary);
+    assertThat(getGeneratingAction(jar).getMnemonic()).isEqualTo("RClassGenerator");
+    List<String> args = getGeneratingSpawnActionArgs(jar);
+
+    AndroidResourcesInfo resourcesInfo =
+        binary.getConfiguredTarget().get(AndroidResourcesInfo.PROVIDER);
+    assertThat(resourcesInfo.getTransitiveAndroidResources()).hasSize(2);
+    ValidatedAndroidData firstDep = resourcesInfo.getTransitiveAndroidResources().toList().get(0);
+    ValidatedAndroidData secondDep = resourcesInfo.getTransitiveAndroidResources().toList().get(1);
+
+    assertThat(args)
+        .containsAllOf(
+            "--primaryRTxt",
+            "--primaryManifest",
+            "--library",
+            firstDep.getRTxt().getExecPathString()
+                + ","
+                + firstDep.getManifest().getExecPathString(),
+            "--library",
+            secondDep.getRTxt().getExecPathString()
+                + ","
+                + secondDep.getManifest().getExecPathString(),
+            "--classJarOutput")
+        .inOrder();
+  }
+
+  @Test
   public void testNoCrunchBinaryOnly() throws Exception {
     scratch.file("java/r/android/BUILD",
         "android_binary(name = 'r',",