Skip to content

Commit

Permalink
Always collect FileProvider's filesToBuild as data runfiles
Browse files Browse the repository at this point in the history
Guidelines for Starlark rules suggest that `data`-like attributes on
rules should always merge the default outputs of rule targets into the
transitive runfiles. See:
https://docs.bazel.build/versions/main/skylark/rules.html#runfiles-features-to-avoid

As a result, Starlark rules generally don't (and shouldn't) explicitly
include their default outputs into their runfiles. Before this commit,
native rules did not merge these outputs in the same way as idiomatic
Starlark rules, which led to unexpectedly absent runfiles.
  • Loading branch information
fmeum committed Sep 10, 2022
1 parent dcc5b22 commit ca229a4
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,13 @@ private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target,
return this;
}

if (mapping == RunfilesProvider.DATA_RUNFILES) {
FileProvider fileProvider = target.getProvider(FileProvider.class);
if (fileProvider != null) {
addTransitiveArtifacts(fileProvider.getFilesToBuild());
}
}

return addTargetExceptFileTargets(target, mapping);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,134 @@ public void testDefaultInfoWithRunfilesConstructor() throws Exception {
assertThat(getConfiguredTarget("//src:r_tools")).isNotNull();
}

@Test
public void testDefaultInfoFilesAddedToCcBinaryTargetRunfiles() throws Exception {
scratch.file(
"test/starlark/extension.bzl",
"def custom_rule_impl(ctx):",
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
" ctx.actions.write(out, 'foobar')",
" return [DefaultInfo(files = depset([out]))]",
"",
"custom_rule = rule(implementation = custom_rule_impl)");

scratch.file(
"test/starlark/BUILD",
"load('//test/starlark:extension.bzl', 'custom_rule')",
"",
"custom_rule(name = 'cr')",
"cc_binary(name = 'binary', data = [':cr'])"
);

ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");

assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
.contains("cr.out");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
.contains("cr.out");
}

@Test
public void testDefaultInfoFilesAddedToJavaBinaryTargetRunfiles() throws Exception {
scratch.file(
"test/starlark/extension.bzl",
"def custom_rule_impl(ctx):",
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
" ctx.actions.write(out, 'foobar')",
" return [DefaultInfo(files = depset([out]))]",
"",
"custom_rule = rule(implementation = custom_rule_impl)");

scratch.file(
"test/starlark/BUILD",
"load('//test/starlark:extension.bzl', 'custom_rule')",
"",
"custom_rule(name = 'cr')",
"java_binary(name = 'binary', data = [':cr'], srcs = ['Foo.java'], main_class = 'Foo')"
);

ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");

assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
.contains("cr.out");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
.contains("cr.out");
}

@Test
public void testDefaultInfoFilesAddedToPyBinaryTargetRunfiles() throws Exception {
scratch.file(
"test/starlark/extension.bzl",
"def custom_rule_impl(ctx):",
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
" ctx.actions.write(out, 'foobar')",
" return [DefaultInfo(files = depset([out]))]",
"",
"custom_rule = rule(implementation = custom_rule_impl)");

scratch.file(
"test/starlark/BUILD",
"load('//test/starlark:extension.bzl', 'custom_rule')",
"",
"custom_rule(name = 'cr')",
"py_binary(name = 'binary', data = [':cr'], srcs = ['binary.py'])"
);

ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");

assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
.contains("cr.out");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
.contains("cr.out");
}

@Test
public void testDefaultInfoFilesAddedToShBinaryTargetRunfiles() throws Exception {
scratch.file(
"test/starlark/extension.bzl",
"def custom_rule_impl(ctx):",
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
" ctx.actions.write(out, 'foobar')",
" return [DefaultInfo(files = depset([out]))]",
"",
"custom_rule = rule(implementation = custom_rule_impl)");

scratch.file(
"test/starlark/BUILD",
"load('//test/starlark:extension.bzl', 'custom_rule')",
"",
"custom_rule(name = 'cr')",
"sh_binary(name = 'binary', data = [':cr'], srcs = ['script.sh'])"
);

ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");

assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
.contains("cr.out");
assertThat(
ActionsTestUtil.baseArtifactNames(
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
.contains("cr.out");
}

@Test
public void testInstrumentedFilesProviderWithCodeCoverageDisabled() throws Exception {
setBuildLanguageOptions("--incompatible_disallow_struct_provider_syntax=false");
Expand Down

0 comments on commit ca229a4

Please sign in to comment.