From bbe47a1564a832e1a175206f2dfbc92af94c120b Mon Sep 17 00:00:00 2001 From: cparsons Date: Wed, 17 Apr 2019 11:20:41 -0700 Subject: [PATCH] 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: 244032985 --- .../packages/StarlarkSemanticsOptions.java | 13 +++++ .../SkylarkRuleFunctionsApi.java | 5 +- .../build/lib/syntax/StarlarkSemantics.java | 6 +++ .../SkylarkSemanticsConsistencyTest.java | 2 + .../lib/skylark/SkylarkIntegrationTest.java | 24 ++++++++++ tools/build_defs/pkg/pkg.bzl | 48 +++++++++++++++---- tools/jdk/default_java_toolchain.bzl | 21 ++++++-- 7 files changed, 102 insertions(+), 17 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 3bb2b55c02ef7a..58208a47979a7f 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 @@ -416,6 +416,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 = "false", @@ -587,6 +599,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 c2b82d1a9efbde..c66e70814a2ea4 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 @@ -153,9 +153,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 aaddb98e5047d3..5d78a8839ec263 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 @@ -48,6 +48,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), @@ -167,6 +168,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleNoOutputAttrDefault(); + public abstract boolean incompatibleNoRuleOutputsParam(); + public abstract boolean incompatibleNoSupportToolsInActionInputs(); public abstract boolean incompatibleNoTargetOutputGroup(); @@ -229,6 +232,7 @@ public static Builder builderWithDefaults() { .incompatibleNoAttrLicense(true) .incompatibleNoKwargsInBuildFiles(false) .incompatibleNoOutputAttrDefault(true) + .incompatibleNoRuleOutputsParam(false) .incompatibleNoSupportToolsInActionInputs(false) .incompatibleNoTargetOutputGroup(false) .incompatibleNoTransitiveLoads(true) @@ -300,6 +304,8 @@ public abstract static class Builder { 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 294d329b165f8b..6b6e84b7d4aefb 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 @@ -151,6 +151,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(), @@ -199,6 +200,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 43b5c4b06080fc..cb7f3d80565441 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 @@ -2893,6 +2893,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"); + } + /** * Skylark integration test that forces inlining. */ diff --git a/tools/build_defs/pkg/pkg.bzl b/tools/build_defs/pkg/pkg.bzl index 8ef8c1af6a01ce..1e1391cff24cf8 100644 --- a/tools/build_defs/pkg/pkg.bzl +++ b/tools/build_defs/pkg/pkg.bzl @@ -212,6 +212,12 @@ def _pkg_deb_impl(ctx): inputs = [ctx.outputs.deb], outputs = [ctx.outputs.out], ) + output_groups = {"out": [ctx.outputs.out]} + if hasattr(ctx.outputs, "out_deb"): + output_groups["deb"] = ctx.outputs.deb + if hasattr(ctx.outputs, "out_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( @@ -226,6 +232,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(), @@ -244,9 +251,6 @@ _real_pkg_tar = rule( allow_files = True, ), }, - outputs = { - "out": "%{name}.%{extension}", - }, ) def pkg_tar(**kwargs): @@ -260,10 +264,17 @@ 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) + if "extension" in kwargs and kwargs["extension"]: + extension = kwargs["extension"] + else: + extension = "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), @@ -300,10 +311,27 @@ 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(), + "changes": attr.output(), }, ) + +def pkg_deb(name, package, **kwargs): + """Creates a deb file. See README.md.""" + version = kwargs.get("version", None) + architecture = kwargs.get("architecture", "all") + out_deb = None + out_changes = None + if version and architecture: + 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 3fdcfc1c7b540c..182419952bff11 100644 --- a/tools/jdk/default_java_toolchain.bzl +++ b/tools/jdk/default_java_toolchain.bzl @@ -108,7 +108,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 @@ -141,7 +141,7 @@ def _bootclasspath(ctx): arguments = [args], ) - bootclasspath = ctx.outputs.jar + bootclasspath = ctx.outputs.output_jar inputs = class_outputs + ctx.files.host_javabase @@ -166,9 +166,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", @@ -181,6 +185,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 + )