Change the resource dependency handling to separate between the transitive and direct resources from libraries.

This slightly increases the complexity of resource propagation. The initial algorithm was to simply merge all transitive ResourceContainers together with any new ResourceContainer and propagate them via the AndroidResourcesProvider. The new algorithm is encapsulated inside a new ResourceDependencies class which works as follows:

1. Collect resources from the deps into transitive and direct NestedSets
2. If a rule provides a ResourceContainer, merge the transitive and direct into a new transitive set
3. Export the provider

This results having a rule without resources "forward" the merged sets of transitive and direct resources to the next rule.

--
MOS_MIGRATED_REVID=105515074

1 files changed, 133 insertions, 97 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java
index 0fa9ecd291..2e1c274004 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java
@@ -14,9 +14,11 @@
 package com.google.devtools.build.lib.rules.android;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
 import com.google.common.base.Functions;
 import com.google.common.base.Joiner;
 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.common.collect.Iterators;
@@ -24,7 +26,11 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
+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.AndroidResourcesProvider.ResourceContainer;
 import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceType;
 
@@ -32,13 +38,25 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
+import javax.annotation.Nullable;
+
 /**
  * Builder for creating resource processing action.
  */
 public class AndroidResourcesProcessorBuilder {
+  private static final ResourceContainerToArtifacts RESOURCE_CONTAINER_TO_ARTIFACTS =
+      new ResourceContainerToArtifacts(false);
+
+  private static final ResourceContainerToArtifacts RESOURCE_DEP_TO_ARTIFACTS =
+      new ResourceContainerToArtifacts(true);
+
+  private static final ResourceContainerToArg RESOURCE_CONTAINER_TO_ARG =
+      new ResourceContainerToArg(false);
 
+  private static final ResourceContainerToArg RESOURCE_DEP_TO_ARG =
+      new ResourceContainerToArg(true);
   private ResourceContainer primary;
-  private List<ResourceContainer> dependencies = Collections.emptyList();
+  private ResourceDependencies dependencies;
   private Artifact proguardOut;
   private Artifact rTxtOut;
   private Artifact sourceJarOut;
@@ -75,8 +93,8 @@ public class AndroidResourcesProcessorBuilder {
     return this;
   }
 
-  public AndroidResourcesProcessorBuilder withDependencies(Iterable<ResourceContainer> nestedSet) {
-    this.dependencies = ImmutableList.copyOf(nestedSet);
+  public AndroidResourcesProcessorBuilder withDependencies(ResourceDependencies resourceDeps) {
+    this.dependencies = resourceDeps;
     return this;
   }
 
@@ -131,160 +149,179 @@ public class AndroidResourcesProcessorBuilder {
     return this;
   }
 
-  private void addResourceContainer(List<Artifact> inputs, List<String> args,
-      ResourceContainer container) {
-    Iterables.addAll(inputs, container.getArtifacts());
-    inputs.add(container.getManifest());
-    inputs.add(container.getRTxt());
-
-    args.add(String.format("%s:%s:%s:%s:%s",
-        convertRoots(container, ResourceType.RESOURCES),
-        convertRoots(container, ResourceType.ASSETS),
-        container.getManifest().getExecPathString(),
-        container.getRTxt() == null ? "" : container.getRTxt().getExecPath(),
-        container.getSymbolsTxt() == null ? "" : container.getSymbolsTxt().getExecPath()
-    ));
+  private static class ResourceContainerToArg implements Function<ResourceContainer, String> {
+    private boolean includeSymbols;
+
+    public ResourceContainerToArg(boolean includeSymbols) {
+      this.includeSymbols = includeSymbols;
+    }
+
+    @Override
+    public String apply(ResourceContainer container) {
+      StringBuilder builder = new StringBuilder();
+      builder.append(convertRoots(container, ResourceType.RESOURCES))
+          .append(":")
+          .append(convertRoots(container, ResourceType.ASSETS))
+          .append(":")
+          .append(container.getManifest().getExecPathString());
+      if (includeSymbols) {
+        builder.append(":")
+            .append(container.getRTxt() == null ? "" : container.getRTxt().getExecPath())
+            .append(":")
+            .append(
+                container.getSymbolsTxt() == null ? "" : container.getSymbolsTxt().getExecPath());
+      }
+      return builder.toString();
+    }
   }
 
-  private void addPrimaryResourceContainer(List<Artifact> inputs, List<String> args,
-      ResourceContainer container) {
-    Iterables.addAll(inputs, container.getArtifacts());
-    inputs.add(container.getManifest());
-
-    // no R.txt, because it will be generated from this action.
-    args.add(String.format("%s:%s:%s",
-        convertRoots(container, ResourceType.RESOURCES),
-        convertRoots(container, ResourceType.ASSETS),
-        container.getManifest().getExecPathString()
-    ));
+  private static class ResourceContainerToArtifacts
+      implements Function<ResourceContainer, NestedSet<Artifact>> {
+
+    private boolean includeSymbols;
+
+    public ResourceContainerToArtifacts(boolean includeSymbols) {
+      this.includeSymbols = includeSymbols;
+    }
+
+    @Override
+    public NestedSet<Artifact> apply(ResourceContainer container) {
+      NestedSetBuilder<Artifact> artifacts = NestedSetBuilder.naiveLinkOrder();
+      addIfNotNull(container.getManifest(), artifacts);
+      if (includeSymbols) {
+        addIfNotNull(container.getRTxt(), artifacts);
+        addIfNotNull(container.getSymbolsTxt(), artifacts);
+      }
+      artifacts.addAll(container.getArtifacts());
+      return artifacts.build();
+    }
+
+    private void addIfNotNull(@Nullable Artifact artifact, NestedSetBuilder<Artifact> artifacts) {
+      if (artifact != null) {
+        artifacts.add(artifact);
+      }
+    }
   }
 
   @VisibleForTesting
   public static String convertRoots(ResourceContainer container, ResourceType resourceType) {
-    return Joiner.on("#").join(
-        Iterators.transform(
-            container.getRoots(resourceType).iterator(), Functions.toStringFunction()));
+    return Joiner.on("#").join(Iterators.transform(
+        container.getRoots(resourceType).iterator(), Functions.toStringFunction()));
   }
 
   public ResourceContainer build(ActionConstructionContext context) {
     List<Artifact> outs = new ArrayList<>();
-    List<Artifact> ins = new ArrayList<>();
-    List<String> args = new ArrayList<>();
-
-    args.add("--aapt");
-    args.add(sdk.getAapt().getExecutable().getExecPathString());
+    CustomCommandLine.Builder builder = new CustomCommandLine.Builder();
 
-    Iterables.addAll(ins,
-        ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST)
+    builder.addExecPath("--aapt", sdk.getAapt().getExecutable());
+    // Use a FluentIterable to avoid flattening the NestedSets
+    NestedSetBuilder<Artifact> inputs = NestedSetBuilder.naiveLinkOrder();
+    inputs.addAll(ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST)
             .getRunfilesSupport()
             .getRunfilesArtifactsWithoutMiddlemen());
 
-    args.add("--annotationJar");
-    args.add(sdk.getAnnotationsJar().getExecPathString());
-    ins.add(sdk.getAnnotationsJar());
-    args.add("--androidJar");
-    args.add(sdk.getAndroidJar().getExecPathString());
-    ins.add(sdk.getAndroidJar());
-
-    args.add("--primaryData");
-    addPrimaryResourceContainer(ins, args, primary);
-    if (!dependencies.isEmpty()) {
-      args.add("--data");
-      List<String> data = new ArrayList<>();
-      for (ResourceContainer container : dependencies) {
-        addResourceContainer(ins, data, container);
-      }
-      args.add(Joiner.on(",").join(data));
+    builder.addExecPath("--annotationJar", sdk.getAnnotationsJar());
+    inputs.add(sdk.getAnnotationsJar());
+
+    builder.addExecPath("--androidJar", sdk.getAndroidJar());
+    inputs.add(sdk.getAndroidJar());
+
+    builder.add("--primaryData").add(RESOURCE_CONTAINER_TO_ARG.apply(primary));
+    inputs.addTransitive(RESOURCE_CONTAINER_TO_ARTIFACTS.apply(primary));
+
+    if (dependencies != null) {
+      // add transitive data
+      builder.addJoinStrings("--data", ",",
+          Iterables.transform(dependencies.getTransitiveResources(), RESOURCE_DEP_TO_ARG));
+      // add direct data
+      builder.addJoinStrings("--directData", ",",
+          Iterables.transform(dependencies.getDirectResources(), RESOURCE_DEP_TO_ARG));
+
+      // This flattens the nested set. Since each ResourceContainer needs to be transformed into 
+      // Artifacts, and the NestedSetBuilder.wrap doesn't support lazy Iterator evaluation
+      // and SpawnActionBuilder.addInputs evaluates Iterables, it becomes necessary to make the
+      // best effort and let it get flattened.
+      inputs.addTransitive(
+          NestedSetBuilder.wrap(
+              Order.NAIVE_LINK_ORDER,
+              FluentIterable.from(dependencies.getResources())
+                  .transformAndConcat(RESOURCE_DEP_TO_ARTIFACTS)));
     }
 
     if (rTxtOut != null) {
-      args.add("--rOutput");
-      args.add(rTxtOut.getExecPathString());
+      builder.addExecPath("--rOutput", rTxtOut);
       outs.add(rTxtOut);
       // If R.txt is not null, dependency R.javas will not be regenerated from the R.txt found in
       // the deps, which means the resource processor needs to be told it is creating a library so
       // that it will generate the R.txt.
-      args.add("--packageType");
-      args.add("LIBRARY");
+      builder.add("--packageType").add("LIBRARY");
     }
+
     if (symbolsTxt != null) {
-      args.add("--symbolsTxtOut");
-      args.add(symbolsTxt.getExecPathString());
+      builder.addExecPath("--symbolsTxtOut", symbolsTxt);
       outs.add(symbolsTxt);
     }
     if (sourceJarOut != null) {
-      args.add("--srcJarOutput");
-      args.add(sourceJarOut.getExecPathString());
+      builder.addExecPath("--srcJarOutput", sourceJarOut);
       outs.add(sourceJarOut);
     }
     if (proguardOut != null) {
-      args.add("--proguardOutput");
-      args.add(proguardOut.getExecPathString());
+      builder.addExecPath("--proguardOutput", proguardOut);
       outs.add(proguardOut);
     }
     if (apkOut != null) {
-      args.add("--packagePath");
-      args.add(apkOut.getExecPathString());
+      builder.addExecPath("--packagePath", apkOut);
       outs.add(apkOut);
     }
     if (!resourceConfigs.isEmpty()) {
-      args.add("--resourceConfigs");
-      args.add(Joiner.on(',').join(resourceConfigs));
+      builder.addJoinStrings("--resourceConfigs", ",", resourceConfigs);
     }
     if (!densities.isEmpty()) {
-      args.add("--densities");
-      args.add(Joiner.on(',').join(densities));
+      builder.addJoinStrings("--densities", "'", densities);
     }
     if (!uncompressedExtensions.isEmpty()) {
-      args.add("--uncompressedExtensions");
-      args.add(Joiner.on(',').join(uncompressedExtensions));
+      builder.addJoinStrings("--uncompressedExtensions", ",", uncompressedExtensions);
     }
     if (!assetsToIgnore.isEmpty()) {
-      args.add("--assetsToIgnore");
-      args.add(
-          Joiner.on(',').join(assetsToIgnore));
+      builder.addJoinStrings("--assetsToIgnore", ",", assetsToIgnore);
     }
     if (debug) {
-      args.add("--debug");
+      builder.add("--debug");
     }
 
     if (versionCode != null) {
-      args.add("--versionCode");
-      args.add(versionCode);
+      builder.add("--versionCode").add(versionCode);
     }
 
     if (versionName != null) {
-      args.add("--versionName");
-      args.add(versionName);
+      builder.add("--versionName").add(versionName);
     }
 
     if (applicationId != null) {
-      args.add("--applicationId");
-      args.add(applicationId);
+      builder.add("--applicationId").add(applicationId);
     }
 
     if (!Strings.isNullOrEmpty(customJavaPackage)) {
       // Sets an alternative java package for the generated R.java
       // this is allows android rules to generate resources outside of the java{,tests} tree.
-      args.add("--packageForR");
-      args.add(customJavaPackage);
+      builder.add("--packageForR").add(customJavaPackage);
     }
 
     // Create the spawn action.
-    ruleContext.registerAction(this.spawnActionBuilder
-        .addTool(sdk.getAapt())
-        .addInputs(ImmutableList.<Artifact>copyOf(ins))
-        .addOutputs(ImmutableList.<Artifact>copyOf(outs))
-        .addArguments(ImmutableList.<String>copyOf(args))
-        .setExecutable(
-            ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST))
-        .setProgressMessage("Processing resources")
-        .setMnemonic("AndroidAapt")
-        .build(context));
+    ruleContext.registerAction(
+        this.spawnActionBuilder
+            .addTool(sdk.getAapt())
+            .addTransitiveInputs(inputs.build())
+            .addOutputs(ImmutableList.<Artifact>copyOf(outs))
+            .setCommandLine(builder.build())
+            .setExecutable(
+                ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST))
+            .setProgressMessage("Processing resources")
+            .setMnemonic("AndroidAapt")
+            .build(context));
 
     // Return the full set of processed transitive dependencies.
-    return new ResourceContainer(
-        primary.getLabel(),
+    return new ResourceContainer(primary.getLabel(),
         primary.getJavaPackage(),
         primary.getRenameManifestPackage(),
         primary.getConstantsInlined(),
@@ -293,8 +330,7 @@ public class AndroidResourcesProcessorBuilder {
         // for this resource processing action (in case of just creating an R.txt or
         // proguard merging), reuse the primary resource from the dependencies.
         apkOut != null ? apkOut : primary.getApk(),
-        primary.getManifest(),
-        sourceJarOut,
+        primary.getManifest(), sourceJarOut,
         primary.getArtifacts(ResourceType.ASSETS),
         primary.getArtifacts(ResourceType.RESOURCES),
         primary.getRoots(ResourceType.ASSETS),