Skip to content

Commit

Permalink
Rollforward "Disable outputs param of rule function" with fix
Browse files Browse the repository at this point in the history
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 #7977 for more info.
PiperOrigin-RevId: 252063297
  • Loading branch information
c-parsons authored and Copybara-Service committed Jun 7, 2019
1 parent 4a24f5f commit 36c70a6
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -662,6 +674,7 @@ public StarlarkSemantics toSkylarkSemantics() {
.incompatibleNoAttrLicense(incompatibleNoAttrLicense)
.incompatibleNoKwargsInBuildFiles(incompatibleNoKwargsInBuildFiles)
.incompatibleNoOutputAttrDefault(incompatibleNoOutputAttrDefault)
.incompatibleNoRuleOutputsParam(incompatibleNoRuleOutputsParam)
.incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs)
.incompatibleNoTargetOutputGroup(incompatibleNoTargetOutputGroup)
.incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,10 @@ public interface SkylarkRuleFunctionsApi<FileApiT extends FileApi> {
callbackEnabled = true,
noneable = true,
defaultValue = "None",
valueWhenDisabled = "None",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM,
doc =
"<b>Experimental:</b> This API is in the process of being redesigned."
+ "<p>A schema for defining predeclared outputs. Unlike "
"A schema for defining predeclared outputs. Unlike "
+ "<a href='attr.html#output'><code>output</code></a> and "
+ "<a href='attr.html#output_list'><code>output_list</code></a> attributes, "
+ "the user does not specify the labels for these files. "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -265,6 +268,7 @@ public static Builder builderWithDefaults() {
.incompatibleNoAttrLicense(true)
.incompatibleNoKwargsInBuildFiles(true)
.incompatibleNoOutputAttrDefault(true)
.incompatibleNoRuleOutputsParam(false)
.incompatibleNoSupportToolsInActionInputs(true)
.incompatibleNoTargetOutputGroup(false)
.incompatibleNoTransitiveLoads(true)
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
42 changes: 32 additions & 10 deletions tools/build_defs/pkg/pkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(),
Expand All @@ -257,9 +264,6 @@ _real_pkg_tar = rule(
allow_files = True,
),
},
outputs = {
"out": "%{name}.%{extension}",
},
)

def pkg_tar(**kwargs):
Expand All @@ -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),
Expand Down Expand Up @@ -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
)
21 changes: 16 additions & 5 deletions tools/jdk/default_java_toolchain.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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",
Expand All @@ -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
)

0 comments on commit 36c70a6

Please sign in to comment.