Make tools in action inputs an error.

Supporting tools in inputs introduces a slow linear scan. Such tools should be moved to the 'tools' attribute. If --incompatible_no_support_tools_in_action_inputs is set the inputs are scanned, but a helpful error message is issued to the user.

Eventually we will remove the slow scanning. Errors will surface in the execution phase instead of during analysis, and the resulting error messages will be less obvious.

RELNOTES: None
RELNOTES[INC]: With --incompatible_no_support_tools_in_action_inputs enabled, Skylark action inputs are no longer scanned for tools. Move any such inputs to the newly introduced 'tools' attribute.
PiperOrigin-RevId: 198996093

8 files changed, 132 insertions, 6 deletions

diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md
index f6fe1e51f8..aac35278ce 100644
--- a/site/docs/skylark/backward-compatibility.md
+++ b/site/docs/skylark/backward-compatibility.md
@@ -45,6 +45,7 @@ guarded behind flags in the current release:
 *   [Remove native git repository](#remove-native-git-repository)
 *   [Remove native http archive](#remove-native-http-archive)
 *   [New-style JavaInfo constructor](#new-style-java_info)
+*   [Disallow tools in action inputs](#disallow-tools-in-action-inputs)
 
 
 ### Dictionary concatenation
@@ -349,4 +350,42 @@ provider = JavaInfo(
 )
 ```
 
+### Disallow tools in action inputs
+
+A tool is an input coming from an attribute of type `label`
+where the attribute has been marked `executable = True`. In order for an action
+to run a tool, it needs access to its runfiles.
+
+Under the old API, tools are passed to `ctx.actions.run()` and
+`ctx.actions.run_shell()` via their `inputs` parameter. Bazel scans this
+argument (which may be a large depset) to find all the inputs that are tools,
+and adds their runfiles automatically.
+
+In the new API, tools are instead passed to a dedicated `tools` parameter. The
+`inputs` are not scanned. If a tool is accidentally put in `inputs` instead of
+`tools`, the action will fail during the execution phase with an error due to
+missing runfiles. This may be somewhat cryptic.
+
+To support a gradual transition, all actions with a `tools` argument are opted
+into the new API, while all actions without a `tools` argument still follow the
+old one. In the future (when this flag is removed), all actions will use the new
+API unconditionally.
+
+This flag turns on a safety check that is useful for migrating existing code.
+The safety check applies to all actions that do not have a `tools` argument. It
+scans the `inputs` looking for tools, and if it finds any, it raises an error
+during the analysis phase that clearly identifies the offending tools.
+
+In the rare case that your action requires a tool as input, but does not
+actually run the tool and therefore does not need its runfiles, the safety check
+will fail even though the action would have succeeded. In this case, you can
+bypass the check by adding a (possibly empty) `tools` argument to your action.
+Note that once an action has been modified to take a `tools` argument, you will
+no longer get helpful analysis-time errors for any remaining tools that should
+have been migrated from `inputs`.
+
+
+*   Flag: `--incompatible_no_support_tools_in_action_inputs`
+*   Default: `false`
+
 <!-- Add new options here -->
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
index a13b2818f0..bb39ec5ee2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
@@ -13,7 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.skylark;
 
+import static java.util.stream.Collectors.toList;
+
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Action;
@@ -392,11 +395,45 @@ public class SkylarkActionFactory implements SkylarkActionFactoryApi {
       }
     } else {
       // Users didn't pass 'tools', kick in compatibility modes
-      // Full legacy support -- add tools from inputs
-      for (Artifact artifact : inputArtifacts) {
-        FilesToRunProvider provider = context.getExecutableRunfiles(artifact);
-        if (provider != null) {
-          builder.addTool(provider);
+      if (skylarkSemantics.incompatibleNoSupportToolsInActionInputs()) {
+        // In this mode we error out if we find any tools among the inputs
+        List<Artifact> tools = null;
+        for (Artifact artifact : inputArtifacts) {
+          FilesToRunProvider provider = context.getExecutableRunfiles(artifact);
+          if (provider != null) {
+            tools = tools != null ? tools : new ArrayList<>(1);
+            tools.add(artifact);
+          }
+        }
+        if (tools != null) {
+          String toolsAsString =
+              Joiner.on(", ")
+                  .join(
+                      tools
+                          .stream()
+                          .map(Artifact::getExecPathString)
+                          .map(s -> "'" + s + "'")
+                          .collect(toList()));
+          throw new EvalException(
+              location,
+              String.format(
+                  "Found tool(s) %s in inputs. "
+                      + "A tool is an input with executable=True set. "
+                      + "All tools should be passed using the 'tools' "
+                      + "argument instead of 'inputs' in order to make their runfiles available "
+                      + "to the action. This safety check will not be performed once the action "
+                      + "is modified to take a 'tools' argument. "
+                      + "To temporarily disable this check, "
+                      + "set --incompatible_no_support_tools_in_action_inputs=false.",
+                  toolsAsString));
+        }
+      } else {
+        // Full legacy support -- add tools from inputs
+        for (Artifact artifact : inputArtifacts) {
+          FilesToRunProvider provider = context.getExecutableRunfiles(artifact);
+          if (provider != null) {
+            builder.addTool(provider);
+          }
         }
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
index 28b3c6747a..758eb39155 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
@@ -53,6 +53,7 @@ public final class SkylarkSemanticsCodec implements ObjectCodec<SkylarkSemantics
     codedOut.writeBoolNoTag(semantics.incompatibleDisallowOldStyleArgsAdd());
     codedOut.writeBoolNoTag(semantics.incompatibleDisallowSlashOperator());
     codedOut.writeBoolNoTag(semantics.incompatibleNewActionsApi());
+    codedOut.writeBoolNoTag(semantics.incompatibleNoSupportToolsInActionInputs());
     codedOut.writeBoolNoTag(semantics.incompatiblePackageNameIsAFunction());
     codedOut.writeBoolNoTag(semantics.incompatibleRemoveNativeGitRepository());
     codedOut.writeBoolNoTag(semantics.incompatibleRemoveNativeHttpArchive());
@@ -76,6 +77,7 @@ public final class SkylarkSemanticsCodec implements ObjectCodec<SkylarkSemantics
     builder.incompatibleDisallowOldStyleArgsAdd(codedIn.readBool());
     builder.incompatibleDisallowSlashOperator(codedIn.readBool());
     builder.incompatibleNewActionsApi(codedIn.readBool());
+    builder.incompatibleNoSupportToolsInActionInputs(codedIn.readBool());
     builder.incompatiblePackageNameIsAFunction(codedIn.readBool());
     builder.incompatibleRemoveNativeGitRepository(codedIn.readBool());
     builder.incompatibleRemoveNativeHttpArchive(codedIn.readBool());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
index 82c89a025a..bc8a1390f0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -200,6 +200,22 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
   public boolean incompatibleNewActionsApi;
 
   @Option(
+      name = "incompatible_no_support_tools_in_action_inputs",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help =
+          "If set to true, tools should be passed to `ctx.actions.run()` and "
+              + "`ctx.actions.run_shell()` using the `tools` parameter instead of the `inputs` "
+              + "parameter. Furthermore, if this flag is set and a `tools` parameter is not "
+              + "passed to the action, it is an error for any tools to appear in the `inputs`.")
+  public boolean incompatibleNoSupportToolsInActionInputs;
+
+  @Option(
     name = "incompatible_package_name_is_a_function",
     defaultValue = "false",
     documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
@@ -282,6 +298,7 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
         .incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd)
         .incompatibleDisallowSlashOperator(incompatibleDisallowSlashOperator)
         .incompatibleNewActionsApi(incompatibleNewActionsApi)
+        .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs)
         .incompatiblePackageNameIsAFunction(incompatiblePackageNameIsAFunction)
         .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository)
         .incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
index d19b2db21c..1a5cc170b6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -59,6 +59,8 @@ public abstract class SkylarkSemantics {
 
   public abstract boolean incompatibleNewActionsApi();
 
+  public abstract boolean incompatibleNoSupportToolsInActionInputs();
+
   public abstract boolean incompatiblePackageNameIsAFunction();
 
   public abstract boolean incompatibleRemoveNativeGitRepository();
@@ -94,6 +96,7 @@ public abstract class SkylarkSemantics {
           .incompatibleDisallowOldStyleArgsAdd(false)
           .incompatibleDisallowSlashOperator(false)
           .incompatibleNewActionsApi(false)
+          .incompatibleNoSupportToolsInActionInputs(false)
           .incompatiblePackageNameIsAFunction(false)
           .incompatibleRemoveNativeGitRepository(false)
           .incompatibleRemoveNativeHttpArchive(false)
@@ -126,6 +129,8 @@ public abstract class SkylarkSemantics {
 
     public abstract Builder incompatibleNewActionsApi(boolean value);
 
+    public abstract Builder incompatibleNoSupportToolsInActionInputs(boolean value);
+
     public abstract Builder incompatiblePackageNameIsAFunction(boolean value);
 
     public abstract Builder incompatibleRemoveNativeGitRepository(boolean value);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index 6ef0459657..0edd71e549 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -129,6 +129,7 @@ public class SkylarkSemanticsConsistencyTest {
         "--incompatible_disallow_old_style_args_add=" + rand.nextBoolean(),
         "--incompatible_disallow_slash_operator=" + rand.nextBoolean(),
         "--incompatible_new_actions_api=" + rand.nextBoolean(),
+        "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(),
         "--incompatible_package_name_is_a_function=" + rand.nextBoolean(),
         "--incompatible_remove_native_git_repository=" + rand.nextBoolean(),
         "--incompatible_remove_native_http_archive=" + rand.nextBoolean(),
@@ -153,6 +154,7 @@ public class SkylarkSemanticsConsistencyTest {
         .incompatibleDisallowOldStyleArgsAdd(rand.nextBoolean())
         .incompatibleDisallowSlashOperator(rand.nextBoolean())
         .incompatibleNewActionsApi(rand.nextBoolean())
+        .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean())
         .incompatiblePackageNameIsAFunction(rand.nextBoolean())
         .incompatibleRemoveNativeGitRepository(rand.nextBoolean())
         .incompatibleRemoveNativeHttpArchive(rand.nextBoolean())
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 22f0d9f9f6..0312144c85 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
@@ -439,6 +439,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
 
   @Test
   public void testCreateSpawnActionWithToolInInputsLegacy() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_no_support_tools_in_action_inputs=false");
     setupToolInInputsTest(
         "output = ctx.actions.declare_file('bar.out')",
         "ctx.actions.run_shell(",
@@ -453,6 +454,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
 
   @Test
   public void testCreateSpawnActionWithToolAttribute() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_no_support_tools_in_action_inputs=true");
     setupToolInInputsTest(
         "output = ctx.actions.declare_file('bar.out')",
         "ctx.actions.run_shell(",
@@ -468,6 +470,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
 
   @Test
   public void testCreateSpawnActionWithToolAttributeIgnoresToolsInInputs() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_no_support_tools_in_action_inputs=true");
     setupToolInInputsTest(
         "output = ctx.actions.declare_file('bar.out')",
         "ctx.actions.run_shell(",
@@ -482,6 +485,26 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
   }
 
   @Test
+  public void testCreateSpawnActionWithToolInInputsFailAtAnalysisTime() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_no_support_tools_in_action_inputs=true");
+    setupToolInInputsTest(
+        "output = ctx.actions.declare_file('bar.out')",
+        "ctx.actions.run_shell(",
+        "  inputs = ctx.attr.exe.files,",
+        "  outputs = [output],",
+        "  command = 'boo bar baz',",
+        ")");
+    try {
+      getConfiguredTarget("//bar:my_rule");
+    } catch (Throwable t) {
+      // Expected
+    }
+    assertThat(eventCollector).hasSize(1);
+    assertThat(eventCollector.iterator().next().getMessage())
+        .containsMatch("Found tool\\(s\\) '.*' in inputs");
+  }
+
+  @Test
   public void testCreateFileAction() throws Exception {
     SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
     evalRuleContextCode(
diff --git a/tools/build_defs/pkg/pkg.bzl b/tools/build_defs/pkg/pkg.bzl
index f18a736d87..459161785b 100644
--- a/tools/build_defs/pkg/pkg.bzl
+++ b/tools/build_defs/pkg/pkg.bzl
@@ -81,7 +81,8 @@ def _pkg_tar_impl(ctx):
 
     ctx.actions.run_shell(
         command = "%s --flagfile=%s" % (build_tar.path, arg_file.path),
-        inputs = file_inputs + ctx.files.deps + [arg_file, build_tar],
+        inputs = file_inputs + ctx.files.deps + [arg_file],
+        tools = [build_tar],
         outputs = [ctx.outputs.out],
         mnemonic = "PackageTar",
         use_default_shell_env = True,