From 36c70a68b75e2fe06fc78ddc493a802d09fcadb1 Mon Sep 17 00:00:00 2001 From: cparsons Date: Fri, 7 Jun 2019 09:31:14 -0700 Subject: [PATCH] Rollforward "Disable outputs param of rule function" with fix Original change broke //scripts/packages/debian:bazel-debian.deb This rollforward manually tested to not break that target. *** Original change description *** Disable outputs param of rule function This constitutes an incompatible change guarded by flag --incompatible_no_rule_outputs_param. See #7977 for further details. Implementation for #7977. RELNOTES: The `outputs` parameter of the `rule()` function is deprecated and attached to flag `--incompatible_no_rule_outputs_param`. Migrate rules to use `OutputGroupInfo` or `attr.output` instead. See https://github.com/bazelbuild/bazel/issues/7977 for more info. PiperOrigin-RevId: 252063297 --- .../packages/StarlarkSemanticsOptions.java | 13 ++++++ .../SkylarkRuleFunctionsApi.java | 5 ++- .../build/lib/syntax/StarlarkSemantics.java | 6 +++ .../SkylarkSemanticsConsistencyTest.java | 2 + .../lib/skylark/SkylarkIntegrationTest.java | 24 +++++++++++ .../lib/skylark/SkylarkRuleContextTest.java | 9 ++-- tools/build_defs/pkg/pkg.bzl | 42 ++++++++++++++----- tools/jdk/default_java_toolchain.bzl | 21 +++++++--- 8 files changed, 102 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index 4d52868c2d2b82..587e9593398ccf 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java @@ -448,6 +448,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "`attr.output_list` attribute definition functions.") public boolean incompatibleNoOutputAttrDefault; + @Option( + name = "incompatible_no_rule_outputs_param", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If set to true, disables the `outputs` parameter of the `rule()` Starlark function.") + public boolean incompatibleNoRuleOutputsParam; + @Option( name = "incompatible_no_support_tools_in_action_inputs", defaultValue = "true", @@ -662,6 +674,7 @@ public StarlarkSemantics toSkylarkSemantics() { .incompatibleNoAttrLicense(incompatibleNoAttrLicense) .incompatibleNoKwargsInBuildFiles(incompatibleNoKwargsInBuildFiles) .incompatibleNoOutputAttrDefault(incompatibleNoOutputAttrDefault) + .incompatibleNoRuleOutputsParam(incompatibleNoRuleOutputsParam) .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs) .incompatibleNoTargetOutputGroup(incompatibleNoTargetOutputGroup) .incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads) diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java index 20d280bfbf5fa4..a00b633b23b75f 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java @@ -149,9 +149,10 @@ public interface SkylarkRuleFunctionsApi { callbackEnabled = true, noneable = true, defaultValue = "None", + valueWhenDisabled = "None", + disableWithFlag = FlagIdentifier.INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM, doc = - "Experimental: This API is in the process of being redesigned." - + "

A schema for defining predeclared outputs. Unlike " + "A schema for defining predeclared outputs. Unlike " + "output and " + "output_list attributes, " + "the user does not specify the labels for these files. " diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index 9f3e9607bfa95c..987de0a9d95603 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -53,6 +53,7 @@ public enum FlagIdentifier { INCOMPATIBLE_DISABLE_OBJC_PROVIDER_RESOURCES( StarlarkSemantics::incompatibleDisableObjcProviderResources), INCOMPATIBLE_NO_OUTPUT_ATTR_DEFAULT(StarlarkSemantics::incompatibleNoOutputAttrDefault), + INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM(StarlarkSemantics::incompatibleNoRuleOutputsParam), INCOMPATIBLE_NO_TARGET_OUTPUT_GROUP(StarlarkSemantics::incompatibleNoTargetOutputGroup), INCOMPATIBLE_NO_ATTR_LICENSE(StarlarkSemantics::incompatibleNoAttrLicense), INCOMPATIBLE_OBJC_FRAMEWORK_CLEANUP(StarlarkSemantics::incompatibleObjcFrameworkCleanup), @@ -180,6 +181,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleNoOutputAttrDefault(); + public abstract boolean incompatibleNoRuleOutputsParam(); + public abstract boolean incompatibleNoSupportToolsInActionInputs(); public abstract boolean incompatibleNoTargetOutputGroup(); @@ -265,6 +268,7 @@ public static Builder builderWithDefaults() { .incompatibleNoAttrLicense(true) .incompatibleNoKwargsInBuildFiles(true) .incompatibleNoOutputAttrDefault(true) + .incompatibleNoRuleOutputsParam(false) .incompatibleNoSupportToolsInActionInputs(true) .incompatibleNoTargetOutputGroup(false) .incompatibleNoTransitiveLoads(true) @@ -346,6 +350,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo public abstract Builder incompatibleNoOutputAttrDefault(boolean value); + public abstract Builder incompatibleNoRuleOutputsParam(boolean value); + public abstract Builder incompatibleNoSupportToolsInActionInputs(boolean value); public abstract Builder incompatibleNoTargetOutputGroup(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 01ebea97756244..50ff3410bdb97f 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 @@ -156,6 +156,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_no_attr_license=" + rand.nextBoolean(), "--incompatible_no_kwargs_in_build_files=" + rand.nextBoolean(), "--incompatible_no_output_attr_default=" + rand.nextBoolean(), + "--incompatible_no_rule_outputs_param=" + rand.nextBoolean(), "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(), "--incompatible_no_target_output_group=" + rand.nextBoolean(), "--incompatible_no_transitive_loads=" + rand.nextBoolean(), @@ -210,6 +211,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleNoAttrLicense(rand.nextBoolean()) .incompatibleNoKwargsInBuildFiles(rand.nextBoolean()) .incompatibleNoOutputAttrDefault(rand.nextBoolean()) + .incompatibleNoRuleOutputsParam(rand.nextBoolean()) .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean()) .incompatibleNoTargetOutputGroup(rand.nextBoolean()) .incompatibleNoTransitiveLoads(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index f07fc4fd6d8181..556453102ec769 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -2909,6 +2909,30 @@ public void testDisallowStructProviderSyntax() throws Exception { + "--incompatible_disallow_struct_provider_syntax=false"); } + @Test + public void testNoRuleOutputsParam() throws Exception { + setSkylarkSemanticsOptions("--incompatible_no_rule_outputs_param=true"); + scratch.file( + "test/skylark/test_rule.bzl", + "def _impl(ctx):", + " output = ctx.outputs.out", + " ctx.actions.write(output = output, content = 'hello')", + "", + "my_rule = rule(", + " implementation = _impl,", + " outputs = {\"out\": \"%{name}.txt\"})"); + scratch.file( + "test/skylark/BUILD", + "load('//test/skylark:test_rule.bzl', 'my_rule')", + "my_rule(name = 'target')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test/skylark:target"); + assertContainsEvent( + "parameter 'outputs' is deprecated and will be removed soon. It may be temporarily " + + "re-enabled by setting --incompatible_no_rule_outputs_param=false"); + } + @Test public void testExecutableNotInRunfiles() throws Exception { setSkylarkSemanticsOptions("--incompatible_disallow_struct_provider_syntax=false"); diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java index 1b54e1cd726864..054d5a8e42f311 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java @@ -1925,7 +1925,8 @@ public void testNoAccessToDependencyActionsWithoutSkylarkTest() throws Exception @Test public void testAbstractActionInterface() throws Exception { setSkylarkSemanticsOptions( - "--incompatible_disallow_struct_provider_syntax=false"); + "--incompatible_disallow_struct_provider_syntax=false", + "--incompatible_no_rule_outputs_param=false"); scratch.file("test/rules.bzl", "def _undertest_impl(ctx):", " out1 = ctx.outputs.out1", @@ -1969,7 +1970,8 @@ public void testAbstractActionInterface() throws Exception { @Test public void testCreatedActions() throws Exception { setSkylarkSemanticsOptions( - "--incompatible_disallow_struct_provider_syntax=false"); + "--incompatible_disallow_struct_provider_syntax=false", + "--incompatible_no_rule_outputs_param=false"); // createRuleContext() gives us the context for a rule upon entry into its analysis function. // But we need to inspect the result of calling created_actions() after the rule context has // been modified by creating actions. So we'll call created_actions() from within the analysis @@ -2052,7 +2054,8 @@ public void testSpawnActionInterface() throws Exception { @Test public void testRunShellUsesHelperScriptForLongCommand() throws Exception { setSkylarkSemanticsOptions( - "--incompatible_disallow_struct_provider_syntax=false"); + "--incompatible_disallow_struct_provider_syntax=false", + "--incompatible_no_rule_outputs_param=false"); // createRuleContext() gives us the context for a rule upon entry into its analysis function. // But we need to inspect the result of calling created_actions() after the rule context has // been modified by creating actions. So we'll call created_actions() from within the analysis diff --git a/tools/build_defs/pkg/pkg.bzl b/tools/build_defs/pkg/pkg.bzl index 9074865c137c95..c2deaeece5cc77 100644 --- a/tools/build_defs/pkg/pkg.bzl +++ b/tools/build_defs/pkg/pkg.bzl @@ -225,6 +225,12 @@ def _pkg_deb_impl(ctx): inputs = [ctx.outputs.deb], outputs = [ctx.outputs.out], ) + output_groups = {"out": [ctx.outputs.out]} + if hasattr(ctx.outputs, "deb"): + output_groups["deb"] = [ctx.outputs.deb] + if hasattr(ctx.outputs, "changes"): + output_groups["changes"] = [ctx.outputs.changes] + return OutputGroupInfo(**output_groups) # A rule for creating a tar file, see README.md _real_pkg_tar = rule( @@ -239,6 +245,7 @@ _real_pkg_tar = rule( "modes": attr.string_dict(), "mtime": attr.int(default = -1), "portable_mtime": attr.bool(default = True), + "out": attr.output(), "owner": attr.string(default = "0.0"), "ownername": attr.string(default = "."), "owners": attr.string_dict(), @@ -257,9 +264,6 @@ _real_pkg_tar = rule( allow_files = True, ), }, - outputs = { - "out": "%{name}.%{extension}", - }, ) def pkg_tar(**kwargs): @@ -273,10 +277,14 @@ def pkg_tar(**kwargs): "This attribute was renamed to `srcs`. " + "Consider renaming it in your BUILD file.") kwargs["srcs"] = kwargs.pop("files") - _real_pkg_tar(**kwargs) + extension = kwargs.get("extension") or "tar" + _real_pkg_tar( + out = kwargs["name"] + "." + extension, + **kwargs + ) # A rule for creating a deb file, see README.md -pkg_deb = rule( +_pkg_deb = rule( implementation = _pkg_deb_impl, attrs = { "data": attr.label(mandatory = True, allow_single_file = tar_filetype), @@ -316,10 +324,24 @@ pkg_deb = rule( executable = True, allow_files = True, ), - }, - outputs = { - "out": "%{name}.deb", - "deb": "%{package}_%{version}_%{architecture}.deb", - "changes": "%{package}_%{version}_%{architecture}.changes", + # Outputs. + "out": attr.output(mandatory = True), + "deb": attr.output(mandatory = True), + "changes": attr.output(mandatory = True), }, ) + +def pkg_deb(name, package, **kwargs): + """Creates a deb file. See README.md.""" + version = kwargs.get("version", "") + architecture = kwargs.get("architecture", "all") + out_deb = "%s_%s_%s.deb" % (package, version, architecture) + out_changes = "%s_%s_%s.changes" % (package, version, architecture) + _pkg_deb( + name = name, + package = package, + out = name + ".deb", + deb = out_deb, + changes = out_changes, + **kwargs + ) diff --git a/tools/jdk/default_java_toolchain.bzl b/tools/jdk/default_java_toolchain.bzl index da8f1182644038..308b42a086f8b4 100644 --- a/tools/jdk/default_java_toolchain.bzl +++ b/tools/jdk/default_java_toolchain.bzl @@ -110,7 +110,7 @@ def java_runtime_files(name, srcs): outs = [src], ) -def _bootclasspath(ctx): +def _bootclasspath_impl(ctx): host_javabase = ctx.attr.host_javabase[java_common.JavaRuntimeInfo] # explicitly list output files instead of using TreeArtifact to work around @@ -143,7 +143,7 @@ def _bootclasspath(ctx): arguments = [args], ) - bootclasspath = ctx.outputs.jar + bootclasspath = ctx.outputs.output_jar inputs = class_outputs + ctx.files.host_javabase @@ -168,9 +168,13 @@ def _bootclasspath(ctx): outputs = [bootclasspath], arguments = [args], ) + return [ + DefaultInfo(files = depset([bootclasspath])), + OutputGroupInfo(jar = [bootclasspath]), + ] -bootclasspath = rule( - implementation = _bootclasspath, +_bootclasspath = rule( + implementation = _bootclasspath_impl, attrs = { "host_javabase": attr.label( cfg = "host", @@ -183,6 +187,13 @@ bootclasspath = rule( "target_javabase": attr.label( providers = [java_common.JavaRuntimeInfo], ), + "output_jar": attr.output(mandatory = True), }, - outputs = {"jar": "%{name}.jar"}, ) + +def bootclasspath(name, **kwargs): + _bootclasspath( + name = name, + output_jar = name + ".jar", + **kwargs + )