Automated rollback of commit 9bfbefc13f2b6ae9a86fd46a8470e3b4cd8efd1a.

*** Reason for rollback ***

Roll forward with fix and test - turns out I didn't distinguish properly
between list and item seperators.

*** Original change description ***

Rollback "Allow Merge action to take an interface as primary, not just ResourceContainer", as it breaks some android rule integration tests.

RELNOTES: none
PiperOrigin-RevId: 191322706

6 files changed, 235 insertions, 26 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
index f23146b3e1..1fa50a6628 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
@@ -208,7 +208,7 @@ public final class CustomCommandLine extends CommandLine {
       }
 
       /** Each argument is mapped using the supplied map function */
-      public MappedVectorArg<T> mapped(CommandLineItem.MapFn<T> mapFn) {
+      public MappedVectorArg<T> mapped(CommandLineItem.MapFn<? super T> mapFn) {
         return new MappedVectorArg<>(this, mapFn);
       }
     }
@@ -216,9 +216,9 @@ public final class CustomCommandLine extends CommandLine {
     /** A vector arg that maps some type T to strings. */
     static class MappedVectorArg<T> extends VectorArg<String> {
       private final Iterable<T> values;
-      private final CommandLineItem.MapFn<T> mapFn;
+      private final CommandLineItem.MapFn<? super T> mapFn;
 
-      private MappedVectorArg(SimpleVectorArg<T> other, CommandLineItem.MapFn<T> mapFn) {
+      private MappedVectorArg(SimpleVectorArg<T> other, CommandLineItem.MapFn<? super T> mapFn) {
         super(
             other.isNestedSet,
             other.isEmpty,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataConverter.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataConverter.java
new file mode 100644
index 0000000000..a7ecccd2e2
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataConverter.java
@@ -0,0 +1,151 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.rules.android;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.CommandLineItem.ParametrizedMapFn;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.Objects;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+/**
+ * Factory for functions to convert a {@code T} to a commandline argument. Uses a certain convention
+ * for commandline arguments (e.g., separators, and ordering of container elements).
+ */
+public class AndroidDataConverter<T> extends ParametrizedMapFn<T> {
+
+  /** Indicates the type of joiner between options expected by the command line. */
+  public enum JoinerType {
+    COLON_COMMA(":", ","),
+    SEMICOLON_AMPERSAND(";", "&");
+
+    private final String itemSeparator;
+    private final String listSeparator;
+
+    JoinerType(String itemSeparator, String listSeparator) {
+      this.itemSeparator = itemSeparator;
+      this.listSeparator = listSeparator;
+    }
+
+    private String escape(String string) {
+      return string
+          .replace(itemSeparator, "\\" + itemSeparator)
+          .replace(listSeparator, "\\" + listSeparator);
+    }
+  }
+
+  private final ImmutableList<Function<T, String>> suppliers;
+  private final JoinerType joinerType;
+
+  private AndroidDataConverter(
+      ImmutableList<Function<T, String>> suppliers, JoinerType joinerType) {
+    this.suppliers = suppliers;
+    this.joinerType = joinerType;
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (!(obj instanceof AndroidDataConverter)) {
+      return false;
+    }
+
+    AndroidDataConverter<?> other = (AndroidDataConverter) obj;
+    return suppliers.equals(other.suppliers) && joinerType.equals(other.joinerType);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(suppliers, joinerType);
+  }
+
+  @Override
+  public int maxInstancesAllowed() {
+    // This is the max number of resource converters we expect to statically
+    // construct for any given blaze instance.
+    // Do not increase recklessly.
+    return 10;
+  }
+
+  @Override
+  public void expandToCommandLine(T t, Consumer<String> args) {
+    args.accept(map(t));
+  }
+
+  public String map(T t) {
+    return suppliers
+        .stream()
+        .map(s -> (s.apply(t)))
+        .collect(Collectors.joining(joinerType.itemSeparator));
+  }
+
+  public static <T> Builder<T> builder(JoinerType joinerType) {
+    return new Builder<>(joinerType);
+  }
+
+  public void addDepsToCommandLine(
+      CustomCommandLine.Builder cmdBuilder,
+      NestedSet<? extends T> direct,
+      NestedSet<? extends T> transitive) {
+    cmdBuilder.addAll("--data", getVectorArg(transitive));
+    cmdBuilder.addAll("--directData", getVectorArg(direct));
+  }
+
+  public VectorArg<String> getVectorArg(NestedSet<? extends T> values) {
+    return VectorArg.join(joinerType.listSeparator).each(values).mapped(this);
+  }
+
+  static class Builder<T> {
+    private final ImmutableList.Builder<Function<T, String>> inner = ImmutableList.builder();
+    private final JoinerType joinerType;
+
+    private Builder(JoinerType joinerType) {
+      this.joinerType = joinerType;
+    }
+
+    Builder<T> withRoots(Function<T, ImmutableList<PathFragment>> rootsFunction) {
+      return with(
+          t ->
+              rootsFunction
+                  .apply(t)
+                  .stream()
+                  .map(PathFragment::toString)
+                  .collect(Collectors.joining("#")));
+    }
+
+    Builder<T> withArtifact(Function<T, Artifact> artifactFunction) {
+      return with(t -> artifactFunction.apply(t).getExecPathString());
+    }
+
+    Builder<T> withLabel(Function<T, Label> labelFunction) {
+      // Escape labels, since they are known to contain separating characters (specifically, ':').
+      return with(t -> joinerType.escape(labelFunction.apply(t).toString()));
+    }
+
+    Builder<T> with(Function<T, String> stringFunction) {
+      inner.add(stringFunction);
+      return this;
+    }
+
+    AndroidDataConverter<T> build() {
+      return new AndroidDataConverter<>(inner.build(), joinerType);
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourceMergingActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourceMergingActionBuilder.java
index 65afc20314..70d70aca56 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourceMergingActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourceMergingActionBuilder.java
@@ -25,8 +25,7 @@ import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 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.NestedSetBuilder;
-import com.google.devtools.build.lib.rules.android.ResourceContainerConverter.ToArg;
-import com.google.devtools.build.lib.rules.android.ResourceContainerConverter.ToArg.Includes;
+import com.google.devtools.build.lib.rules.android.AndroidDataConverter.JoinerType;
 import com.google.devtools.build.lib.util.OS;
 import java.util.ArrayList;
 import java.util.List;
@@ -40,21 +39,22 @@ import java.util.List;
  */
 public class AndroidResourceMergingActionBuilder {
 
-  private static final ResourceContainerConverter.ToArg RESOURCE_CONTAINER_TO_ARG =
-      ResourceContainerConverter.builder()
-          .include(Includes.ResourceRoots)
-          .include(Includes.Label)
-          .include(Includes.SymbolsBin)
-          .withSeparator(ToArg.SeparatorType.SEMICOLON_AMPERSAND)
-          .toArgConverter();
-
-  private static final ResourceContainerConverter.ToArg RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED =
-      ResourceContainerConverter.builder()
-          .include(Includes.ResourceRoots)
-          .include(Includes.Label)
-          .include(Includes.CompiledSymbols)
-          .withSeparator(ToArg.SeparatorType.SEMICOLON_AMPERSAND)
-          .toArgConverter();
+  private static final AndroidDataConverter<MergableAndroidData> RESOURCE_CONTAINER_TO_ARG =
+      AndroidDataConverter.<MergableAndroidData>builder(JoinerType.SEMICOLON_AMPERSAND)
+          .withRoots(MergableAndroidData::getResourceRoots)
+          .withRoots(MergableAndroidData::getAssetRoots)
+          .withLabel(MergableAndroidData::getLabel)
+          .withArtifact(MergableAndroidData::getSymbols)
+          .build();
+
+  private static final AndroidDataConverter<ResourceContainer>
+      RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED =
+          AndroidDataConverter.<ResourceContainer>builder(JoinerType.SEMICOLON_AMPERSAND)
+              .withRoots(ResourceContainer::getResourceRoots)
+              .withRoots(ResourceContainer::getAssetRoots)
+              .withLabel(ResourceContainer::getLabel)
+              .withArtifact(ResourceContainer::getCompiledSymbols)
+              .build();
 
   private final RuleContext ruleContext;
   private final AndroidSdkProvider sdk;
@@ -172,8 +172,10 @@ public class AndroidResourceMergingActionBuilder {
     inputs.add(primary.getCompiledSymbols());
 
     if (dependencies != null) {
-      ResourceContainerConverter.addToCommandLine(
-          dependencies, builder, RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED);
+      RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED.addDepsToCommandLine(
+          builder,
+          dependencies.getDirectResourceContainers(),
+          dependencies.getTransitiveResourceContainers());
       inputs.addTransitive(dependencies.getTransitiveResources());
       inputs.addTransitive(dependencies.getTransitiveAssets());
       inputs.addTransitive(dependencies.getTransitiveCompiledSymbols());
@@ -208,7 +210,10 @@ public class AndroidResourceMergingActionBuilder {
     inputs.add(primary.getSymbols());
 
     if (dependencies != null) {
-      ResourceContainerConverter.addToCommandLine(dependencies, builder, RESOURCE_CONTAINER_TO_ARG);
+      RESOURCE_CONTAINER_TO_ARG.addDepsToCommandLine(
+          builder,
+          dependencies.getDirectResourceContainers(),
+          dependencies.getTransitiveResourceContainers());
       inputs.addTransitive(dependencies.getTransitiveResources());
       inputs.addTransitive(dependencies.getTransitiveAssets());
       inputs.addTransitive(dependencies.getTransitiveSymbolsBin());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/MergableAndroidData.java b/src/main/java/com/google/devtools/build/lib/rules/android/MergableAndroidData.java
new file mode 100644
index 0000000000..47e162067a
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/MergableAndroidData.java
@@ -0,0 +1,42 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.rules.android;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.vfs.PathFragment;
+
+/**
+ * Interface used to indicate a container for resources, assets, or both can be merged.
+ *
+ * <p>Currently, resources and assets invoke the same action for merging, so virtually all the code
+ * to generate that action is identical. Implementing this interface allows it to be reused.
+ */
+public interface MergableAndroidData {
+
+  /** @return the roots of all resources to be merged */
+  default ImmutableList<PathFragment> getResourceRoots() {
+    return ImmutableList.of();
+  }
+
+  /** @return the roots of all assets to be merged */
+  default ImmutableList<PathFragment> getAssetRoots() {
+    return ImmutableList.of();
+  }
+
+  Label getLabel();
+
+  Artifact getSymbols();
+}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java
index 564597a5bb..2bb66189a7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java
@@ -40,7 +40,7 @@ import javax.annotation.Nullable;
   category = SkylarkModuleCategory.NONE,
   doc = "The Android resources contributed by a single target."
 )
-public abstract class ResourceContainer {
+public abstract class ResourceContainer implements MergableAndroidData {
   /** The type of resource in question: either asset or a resource. */
   public enum ResourceType {
     ASSETS("assets"),
@@ -92,12 +92,20 @@ public abstract class ResourceContainer {
         : getResources().getResources();
   }
 
-  /** @deprecated We are moving towards decoupling assets and resources */
-  @Deprecated
   public Iterable<Artifact> getArtifacts() {
     return Iterables.concat(getAssets().getAssets(), getResources().getResources());
   }
 
+  @Override
+  public ImmutableList<PathFragment> getResourceRoots() {
+    return getResources().getResourceRoots();
+  }
+
+  @Override
+  public ImmutableList<PathFragment> getAssetRoots() {
+    return getAssets().getAssetRoots();
+  }
+
   /**
    * Gets the directories containing the resources of a specific type.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainerConverter.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainerConverter.java
index 102abfc0c2..df15063d50 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainerConverter.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainerConverter.java
@@ -35,8 +35,11 @@ import java.util.function.Consumer;
  * Factory for functions to convert a {@link ResourceContainer} to a commandline argument, or a
  * collection of artifacts. Uses a certain convention for commandline arguments (e.g., separators,
  * and ordering of container elements).
+ *
+ * @deprecated Use {@link AndroidDataConverter} instead.
  */
 @VisibleForTesting
+@Deprecated
 public class ResourceContainerConverter {
 
   static Builder builder() {