[]Rollback of commit 65425810207c9fd6892abfaa95da65e25db5e515.

*** Reason for rollback ***

Breaks [].


*** Original change description ***

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...

***

--
MOS_MIGRATED_REVID=105598448

1 files changed, 97 insertions, 133 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 2e1c274004..0fa9ecd291 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,11 +14,9 @@
 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;
@@ -26,11 +24,7 @@ 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;
 
@@ -38,25 +32,13 @@ 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 ResourceDependencies dependencies;
+  private List<ResourceContainer> dependencies = Collections.emptyList();
   private Artifact proguardOut;
   private Artifact rTxtOut;
   private Artifact sourceJarOut;
@@ -93,8 +75,8 @@ public class AndroidResourcesProcessorBuilder {
     return this;
   }
 
-  public AndroidResourcesProcessorBuilder withDependencies(ResourceDependencies resourceDeps) {
-    this.dependencies = resourceDeps;
+  public AndroidResourcesProcessorBuilder withDependencies(Iterable<ResourceContainer> nestedSet) {
+    this.dependencies = ImmutableList.copyOf(nestedSet);
     return this;
   }
 
@@ -149,179 +131,160 @@ public class AndroidResourcesProcessorBuilder {
     return this;
   }
 
-  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 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 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);
-      }
-    }
+  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()
+    ));
   }
 
   @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<>();
-    CustomCommandLine.Builder builder = new CustomCommandLine.Builder();
+    List<Artifact> ins = new ArrayList<>();
+    List<String> args = new ArrayList<>();
+
+    args.add("--aapt");
+    args.add(sdk.getAapt().getExecutable().getExecPathString());
 
-    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)
+    Iterables.addAll(ins,
+        ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST)
             .getRunfilesSupport()
             .getRunfilesArtifactsWithoutMiddlemen());
 
-    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)));
+    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));
     }
 
     if (rTxtOut != null) {
-      builder.addExecPath("--rOutput", rTxtOut);
+      args.add("--rOutput");
+      args.add(rTxtOut.getExecPathString());
       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.
-      builder.add("--packageType").add("LIBRARY");
+      args.add("--packageType");
+      args.add("LIBRARY");
     }
-
     if (symbolsTxt != null) {
-      builder.addExecPath("--symbolsTxtOut", symbolsTxt);
+      args.add("--symbolsTxtOut");
+      args.add(symbolsTxt.getExecPathString());
       outs.add(symbolsTxt);
     }
     if (sourceJarOut != null) {
-      builder.addExecPath("--srcJarOutput", sourceJarOut);
+      args.add("--srcJarOutput");
+      args.add(sourceJarOut.getExecPathString());
       outs.add(sourceJarOut);
     }
     if (proguardOut != null) {
-      builder.addExecPath("--proguardOutput", proguardOut);
+      args.add("--proguardOutput");
+      args.add(proguardOut.getExecPathString());
       outs.add(proguardOut);
     }
     if (apkOut != null) {
-      builder.addExecPath("--packagePath", apkOut);
+      args.add("--packagePath");
+      args.add(apkOut.getExecPathString());
       outs.add(apkOut);
     }
     if (!resourceConfigs.isEmpty()) {
-      builder.addJoinStrings("--resourceConfigs", ",", resourceConfigs);
+      args.add("--resourceConfigs");
+      args.add(Joiner.on(',').join(resourceConfigs));
     }
     if (!densities.isEmpty()) {
-      builder.addJoinStrings("--densities", "'", densities);
+      args.add("--densities");
+      args.add(Joiner.on(',').join(densities));
     }
     if (!uncompressedExtensions.isEmpty()) {
-      builder.addJoinStrings("--uncompressedExtensions", ",", uncompressedExtensions);
+      args.add("--uncompressedExtensions");
+      args.add(Joiner.on(',').join(uncompressedExtensions));
     }
     if (!assetsToIgnore.isEmpty()) {
-      builder.addJoinStrings("--assetsToIgnore", ",", assetsToIgnore);
+      args.add("--assetsToIgnore");
+      args.add(
+          Joiner.on(',').join(assetsToIgnore));
     }
     if (debug) {
-      builder.add("--debug");
+      args.add("--debug");
     }
 
     if (versionCode != null) {
-      builder.add("--versionCode").add(versionCode);
+      args.add("--versionCode");
+      args.add(versionCode);
     }
 
     if (versionName != null) {
-      builder.add("--versionName").add(versionName);
+      args.add("--versionName");
+      args.add(versionName);
     }
 
     if (applicationId != null) {
-      builder.add("--applicationId").add(applicationId);
+      args.add("--applicationId");
+      args.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.
-      builder.add("--packageForR").add(customJavaPackage);
+      args.add("--packageForR");
+      args.add(customJavaPackage);
     }
 
     // Create the spawn action.
-    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));
+    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));
 
     // Return the full set of processed transitive dependencies.
-    return new ResourceContainer(primary.getLabel(),
+    return new ResourceContainer(
+        primary.getLabel(),
         primary.getJavaPackage(),
         primary.getRenameManifestPackage(),
         primary.getConstantsInlined(),
@@ -330,7 +293,8 @@ 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),