Make it possible to pass a FilesToRunProvider to ctx.actions.run() in…

… tools= and executable=.

This makes sense because FilesToRunProvider is the data structure that actually represents an executable. Previously, we had to resort to fishing out the runfiles of binaries using a back channel that knew only about the direct inputs of the rule being analyzed.

Fixes #5463.

RELNOTES: None.
PiperOrigin-RevId: 229699575

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

@@ -242,13 +242,11 @@ public void run(
       }
     } else if (executableUnchecked instanceof String) {
       builder.setExecutable(PathFragment.create((String) executableUnchecked));
+    } else if (executableUnchecked instanceof FilesToRunProvider) {
+      builder.setExecutable((FilesToRunProvider) executableUnchecked);
     } else {
-      throw new EvalException(
-          null,
-          "expected file or string for "
-              + "executable but got "
-              + EvalUtils.getDataTypeName(executableUnchecked)
-              + " instead");
+      // Should have been verified by Starlark before this function is called
+      throw new IllegalStateException();
     }
     registerSpawnAction(
         outputs,
@@ -434,17 +432,30 @@ private void registerSpawnAction(
     builder.addOutputs(outputs.getContents(Artifact.class, "outputs"));
 
     if (toolsUnchecked != Runtime.UNBOUND) {
-      final Iterable<Artifact> toolsIterable;
+      @SuppressWarnings("unchecked")
+      Iterable<Object> toolsIterable;
       if (toolsUnchecked instanceof SkylarkList) {
-        toolsIterable = ((SkylarkList) toolsUnchecked).getContents(Artifact.class, "tools");
+        toolsIterable = ((SkylarkList<Object>) toolsUnchecked).getContents(Object.class, "tools");
       } else {
-        toolsIterable = ((SkylarkNestedSet) toolsUnchecked).getSet(Artifact.class);
+        toolsIterable = ((SkylarkNestedSet) toolsUnchecked).getSet(Object.class);
       }
-      for (Artifact artifact : toolsIterable) {
-        builder.addInput(artifact);
-        FilesToRunProvider provider = context.getExecutableRunfiles(artifact);
-        if (provider != null) {
-          builder.addTool(provider);
+      for (Object toolUnchecked : toolsIterable) {
+        if (toolUnchecked instanceof Artifact) {
+          Artifact artifact = (Artifact) toolUnchecked;
+          builder.addInput(artifact);
+          FilesToRunProvider provider = context.getExecutableRunfiles(artifact);
+          if (provider != null) {
+            builder.addTool(provider);
+          }
+        } else if (toolUnchecked instanceof FilesToRunProvider) {
+          builder.addTool((FilesToRunProvider) toolUnchecked);
+        } else {
+          throw new EvalException(
+              null,
+              "expected value of type 'File or FilesToRunProvider' for "
+                  + "a member of parameter 'tools' but got "
+                  + EvalUtils.getDataTypeName(toolUnchecked)
+                  + " instead");
         }
       }
     } else {

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java

@@ -189,6 +189,7 @@ public void write(FileApi output, Object content, Boolean isExecutable, Location
             allowedTypes = {
               @ParamType(type = FileApi.class),
               @ParamType(type = String.class),
+              @ParamType(type = FilesToRunProviderApi.class),
             },
             named = true,
             positional = false,
@@ -199,7 +200,6 @@ public void write(FileApi output, Object content, Boolean isExecutable, Location
               @ParamType(type = SkylarkList.class),
               @ParamType(type = SkylarkNestedSet.class),
             },
-            generic1 = FileApi.class,
             defaultValue = "unbound",
             named = true,
             positional = false,
@@ -331,8 +331,8 @@ public void run(
             positional = false,
             doc =
                 "List or depset of any tools needed by the action. Tools are inputs with "
-                    + "additional "
-                    + "runfiles that are automatically made available to the action."),
+                    + "additional runfiles that are automatically made available to the action. "
+                    + "The list can contain Files or FilesToRunProvider instances."),
         @Param(
             name = "arguments",
             allowedTypes = {

src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java

@@ -23,6 +23,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -35,6 +36,7 @@
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.DefaultInfo;
+import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
@@ -310,7 +312,7 @@ public void testCreateActionWithDepsetInput() throws Exception {
   public void testCreateSpawnActionArgumentsBadExecutable() throws Exception {
     checkErrorContains(
         createRuleContext("//foo:foo"),
-        "expected value of type 'File or string' for parameter 'executable', "
+        "expected value of type 'File or string or FilesToRunProvider' for parameter 'executable', "
             + "for call to method run(",
         "ruleContext.actions.run(",
         "  inputs = ruleContext.files.srcs,",
@@ -2793,6 +2795,61 @@ public void testConfigurationField_invalidVisibility() throws Exception {
             + "the attribute must be private (i.e. start with '_')");
   }
 
+  @Test
+  public void testFilesToRunInActionsRun() throws Exception {
+    scratch.file(
+        "a/a.bzl",
+        "def _impl(ctx):",
+        "    f = ctx.actions.declare_file('output')",
+        "    ctx.actions.run(",
+        "        inputs = [],",
+        "        outputs = [f],",
+        "        executable = ctx.attr._tool[DefaultInfo].files_to_run)",
+        "    return [DefaultInfo(files=depset([f]))]",
+        "r = rule(implementation=_impl, attrs = {'_tool': attr.label(default='//a:tool')})");
+
+    scratch.file(
+        "a/BUILD",
+        "load(':a.bzl', 'r')",
+        "r(name='r')",
+        "sh_binary(name='tool', srcs=['tool.sh'], data=['data'])");
+
+    ConfiguredTarget r = getConfiguredTarget("//a:r");
+    Action action =
+        getGeneratingAction(
+            Iterables.getOnlyElement(r.getProvider(FileProvider.class).getFilesToBuild()));
+    assertThat(ActionsTestUtil.baseArtifactNames(action.getRunfilesSupplier().getArtifacts()))
+        .containsAllOf("tool", "tool.sh", "data");
+  }
+
+  @Test
+  public void testFilesToRunInActionsTools() throws Exception {
+    scratch.file(
+        "a/a.bzl",
+        "def _impl(ctx):",
+        "    f = ctx.actions.declare_file('output')",
+        "    ctx.actions.run(",
+        "        inputs = [],",
+        "        outputs = [f],",
+        "        tools = [ctx.attr._tool[DefaultInfo].files_to_run],",
+        "        executable = 'a/tool')",
+        "    return [DefaultInfo(files=depset([f]))]",
+        "r = rule(implementation=_impl, attrs = {'_tool': attr.label(default='//a:tool')})");
+
+    scratch.file(
+        "a/BUILD",
+        "load(':a.bzl', 'r')",
+        "r(name='r')",
+        "sh_binary(name='tool', srcs=['tool.sh'], data=['data'])");
+
+    ConfiguredTarget r = getConfiguredTarget("//a:r");
+    Action action =
+        getGeneratingAction(
+            Iterables.getOnlyElement(r.getProvider(FileProvider.class).getFilesToBuild()));
+    assertThat(ActionsTestUtil.baseArtifactNames(action.getRunfilesSupplier().getArtifacts()))
+        .containsAllOf("tool", "tool.sh", "data");
+  }
+
   // Verifies that configuration_field can only be used on 'label' attributes.
   @Test
   public void testConfigurationField_invalidAttributeType() throws Exception {