Allow Merge action to take an interface as primary, not just ResourceContainer

In future reviews, we will use this to be able to pass assets and resources
individually.

Introduce replacement for ResourceContainerConverter that can handle generics
and should be about as flexible.

To support that replacement, slightly improve how CustomCommandLine handles
generics.

RELNOTES: none
PiperOrigin-RevId: 190970298

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..cb367d45a2
--- /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 listSeparator;
+    private final String itemSeparator;
+
+    JoinerType(String listSeparator, String itemSeparator) {
+      this.listSeparator = listSeparator;
+      this.itemSeparator = itemSeparator;
+    }
+
+    private String escape(String string) {
+      return string
+          .replace(listSeparator, "\\" + listSeparator)
+          .replace(itemSeparator, "\\" + itemSeparator);
+    }
+  }
+
+  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.listSeparator));
+  }
+
+  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() {