diff options
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, |