Improve documentation for resolve_command

--
MOS_MIGRATED_REVID=105875176

2 files changed, 94 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
index 56f4c2090e..4a42997c0e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
@@ -130,7 +130,7 @@ public class SkylarkRuleImplementationFunctions {
         @Param(name = "input_manifests", type = Map.class, noneable = true,
             defaultValue = "None",
             doc = "sets the map of input manifests files; "
-            + "they are typically generated by the command_helper")},
+            + "they are typically generated by resolve_command")},
       useLocation = true)
   private static final BuiltinFunction createSpawnAction = new BuiltinFunction("action") {
     public Runtime.NoneType invoke(
@@ -519,12 +519,16 @@ public class SkylarkRuleImplementationFunctions {
     return (Map<Label, Iterable<Artifact>>) labelDict;
   }
 
+  /** suffix of script to be used in case the command is too long to fit on a single line */
+  private static final String SCRIPT_SUFFIX = ".script.sh";
+
   @SkylarkSignature(
     name = "resolve_command",
     doc = "Experimental."
-    + "Returns a tuple of the list of resolved inputs, "
+    + "Returns a tuple (inputs, command, input_manifests) of the list of resolved inputs, "
     + "the argv list for the resolved command, and "
-    + "the dict mapping locations to runfiles required to run the command",
+    + "the dict mapping locations to runfiles required to run the command, "
+    + "all of them suitable for passing as the same-named arguments of the ctx.action method.",
     objectType = SkylarkRuleContext.class,
     returnType = Tuple.class,
     mandatoryPositionals = {
@@ -544,12 +548,6 @@ public class SkylarkRuleImplementationFunctions {
         doc = "name of the associated attribute for which to issue an error, or None"
       ),
       @Param(
-        name = "suffix",
-        type = String.class, // string
-        defaultValue = "'.script.sh'",
-        doc = "suffix of script if command line too long"
-      ),
-      @Param(
         name = "expand_locations",
         type = Boolean.class,
         defaultValue = "False",
@@ -586,7 +584,6 @@ public class SkylarkRuleImplementationFunctions {
             SkylarkRuleContext ctx,
             String command,
             Object attributeO,
-            String suffix,
             Boolean expandLocations,
             Object makeVariablesO,
             SkylarkList tools,
@@ -606,7 +603,6 @@ public class SkylarkRuleImplementationFunctions {
             command = helper.resolveCommandAndExpandLabels(
                 command, attribute, false, false);
           }
-          // TODO(bazel_team): Implement heuristic label expansion
           if (!EvalUtils.isNullOrNone(makeVariablesO)) {
             Map<String, String> makeVariables = Type.STRING_DICT.convert(
                 makeVariablesO, "make_variables", ruleLabel);
@@ -614,7 +610,7 @@ public class SkylarkRuleImplementationFunctions {
           }
           List<Artifact> inputs = new ArrayList<>();
           inputs.addAll(helper.getResolvedTools());
-          List<String> argv = helper.buildCommandLine(command, inputs, suffix);
+          List<String> argv = helper.buildCommandLine(command, inputs, SCRIPT_SUFFIX);
           return Tuple.of(
               new MutableList(inputs, env),
               new MutableList(argv, env),
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index f97c452291..e64e74cfcb 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
 import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
@@ -90,6 +91,17 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
         "  srcs = ['a.go'],",
         "  outs = [ 'gl.a', 'gl.gcgox', ],",
         "  output_to_bindir = 1,",
+        ")",
+        // The two below are used by testResolveCommand
+        "sh_binary(name = 'mytool',",
+        "  srcs = ['mytool.sh'],",
+        "  data = ['file1.dat', 'file2.dat'],",
+        ")",
+        "genrule(name = 'resolve_me',",
+        "  cmd = 'aa',",
+        "  tools = [':mytool', 't.exe'],",
+        "  srcs = ['file3.dat', 'file4.dat'],",
+        "  outs = ['r1.txt', 'r2.txt'],",
         ")");
   }
 
@@ -435,14 +447,82 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
    */
   private void runExpansion(SkylarkRuleContext ruleContext, String command, String expectedPattern)
       throws Exception {
-    String expanded =
-        (String)
-            evalRuleContextCode(
-                ruleContext, String.format("ruleContext.expand_location('$(%s)')", command));
+    assertMatches(
+        "Expanded string",
+        expectedPattern,
+        (String) evalRuleContextCode(
+            ruleContext, String.format("ruleContext.expand_location('$(%s)')", command)));
+  }
 
+  private void assertMatches(String description, String expectedPattern, String computedValue)
+      throws Exception {
     assertTrue(
-        String.format("Expanded string '%s' did not match pattern '%s'", expanded, expectedPattern),
-        Pattern.matches(expectedPattern, expanded));
+        Printer.format("%s %r did not match pattern '%s'",
+            description, computedValue, expectedPattern),
+        Pattern.matches(expectedPattern, computedValue));
+  }
+
+  public void testResolveCommandMakeVariables() throws Exception {
+    evalRuleContextCode(
+        createRuleContext("//foo:resolve_me"),
+        "inputs, argv, manifests = ruleContext.resolve_command(",
+        "  command='I got the $(HELLO) on a $(DAVE)', ",
+        "  make_variables={'HELLO': 'World', 'DAVE': type('')})");
+    @SuppressWarnings("unchecked")
+    List<String> argv = (List<String>) (List<?>) ((MutableList) lookup("argv")).getList();
+    assertThat(argv).hasSize(3);
+    assertMatches("argv[0]", "^.*/bash$", argv.get(0));
+    assertThat(argv.get(1)).isEqualTo("-c");
+    assertThat(argv.get(2)).isEqualTo("I got the World on a string");
+  }
+
+  public void testResolveCommandInputs() throws Exception {
+    evalRuleContextCode(
+        createRuleContext("//foo:resolve_me"),
+        "inputs, argv, manifests = ruleContext.resolve_command(",
+        "   tools=ruleContext.attr.tools)");
+    @SuppressWarnings("unchecked")
+    List<Artifact> inputs = (List<Artifact>) (List<?>) ((MutableList) lookup("inputs")).getList();
+    assertArtifactFilenames(inputs, "mytool.sh", "mytool", "foo_Smytool-runfiles", "t.exe");
+    Map<?, ?> manifests = (Map<?, ?>) lookup("manifests");
+    assertThat(manifests).hasSize(1);
+  }
+
+  public void testResolveCommandExpandLocations() throws Exception {
+    evalRuleContextCode(
+        createRuleContext("//foo:resolve_me"),
+        "def foo():", // no for loops at top-level
+        "  label_dict = {}",
+        "  all = []",
+        "  for dep in ruleContext.attr.srcs + ruleContext.attr.tools:",
+        "    all.extend(list(dep.files))",
+        "    label_dict[dep.label] = list(dep.files)",
+        "  return ruleContext.resolve_command(",
+        "    command='A$(locations //foo:mytool) B$(location //foo:file3.dat)',",
+        "    attribute='cmd', expand_locations=True, label_dict=label_dict)",
+        "inputs, argv, manifests = foo()");
+    @SuppressWarnings("unchecked")
+    List<String> argv = (List<String>) (List<?>) ((MutableList) lookup("argv")).getList();
+    assertThat(argv).hasSize(3);
+    assertMatches("argv[0]", "^.*/bash$", argv.get(0));
+    assertThat(argv.get(1)).isEqualTo("-c");
+    assertMatches("argv[2]", "A.*/mytool .*/mytool.sh B.*file3.dat", argv.get(2));
+  }
+
+  public void testResolveCommandScript() throws Exception {
+    evalRuleContextCode(
+        createRuleContext("//foo:resolve_me"),
+        "def foo():", // no for loops at top-level
+        "  s = 'a'",
+        "  for i in range(1,17): s = s + s", // 2**17 > CommandHelper.maxCommandLength (=64000)
+        "  return ruleContext.resolve_command(",
+        "    command=s)",
+        "argv = foo()[1]");
+    @SuppressWarnings("unchecked")
+    List<String> argv = (List<String>) (List<?>) ((MutableList) lookup("argv")).getList();
+    assertThat(argv).hasSize(2);
+    assertMatches("argv[0]", "^.*/bash$", argv.get(0));
+    assertMatches("argv[1]", "^.*/resolve_me[.]script[.]sh$", argv.get(1));
   }
 
   public void testBadParamTypeErrorMessage() throws Exception {