Skip to content

Commit 95f9adc

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Always collect FileProvider's filesToBuild as data runfiles
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. Fixes bazelbuild#15043 Closes bazelbuild#15052. PiperOrigin-RevId: 486663801 Change-Id: I4a30b8b8a4dfc84bf4de27fd7894dd48e795c081
1 parent d645104 commit 95f9adc

10 files changed

Lines changed: 213 additions & 19 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -935,7 +935,10 @@ public Builder add(
935935
/** Collects runfiles from data dependencies of a target. */
936936
@CanIgnoreReturnValue
937937
public Builder addDataDeps(RuleContext ruleContext) {
938-
addTargets(getPrerequisites(ruleContext, "data"), RunfilesProvider.DATA_RUNFILES);
938+
addTargets(
939+
getPrerequisites(ruleContext, "data"),
940+
RunfilesProvider.DATA_RUNFILES,
941+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
939942
return this;
940943
}
941944

@@ -952,16 +955,20 @@ public Builder addNonDataDeps(
952955
@CanIgnoreReturnValue
953956
public Builder addTargets(
954957
Iterable<? extends TransitiveInfoCollection> targets,
955-
Function<TransitiveInfoCollection, Runfiles> mapping) {
958+
Function<TransitiveInfoCollection, Runfiles> mapping,
959+
boolean alwaysIncludeFilesToBuildInData) {
956960
for (TransitiveInfoCollection target : targets) {
957-
addTarget(target, mapping);
961+
addTarget(target, mapping, alwaysIncludeFilesToBuildInData);
958962
}
959963
return this;
960964
}
961965

962-
public Builder addTarget(TransitiveInfoCollection target,
963-
Function<TransitiveInfoCollection, Runfiles> mapping) {
964-
return addTargetIncludingFileTargets(target, mapping);
966+
@CanIgnoreReturnValue
967+
public Builder addTarget(
968+
TransitiveInfoCollection target,
969+
Function<TransitiveInfoCollection, Runfiles> mapping,
970+
boolean alwaysIncludeFilesToBuildInData) {
971+
return addTargetIncludingFileTargets(target, mapping, alwaysIncludeFilesToBuildInData);
965972
}
966973

967974
@CanIgnoreReturnValue
@@ -975,8 +982,10 @@ private Builder addTargetExceptFileTargets(
975982
return this;
976983
}
977984

978-
private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target,
979-
Function<TransitiveInfoCollection, Runfiles> mapping) {
985+
private Builder addTargetIncludingFileTargets(
986+
TransitiveInfoCollection target,
987+
Function<TransitiveInfoCollection, Runfiles> mapping,
988+
boolean alwaysIncludeFilesToBuildInData) {
980989
if (target.getProvider(RunfilesProvider.class) == null
981990
&& mapping == RunfilesProvider.DATA_RUNFILES) {
982991
// RuleConfiguredTarget implements RunfilesProvider, so this will only be called on
@@ -988,6 +997,17 @@ private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target,
988997
return this;
989998
}
990999

1000+
if (alwaysIncludeFilesToBuildInData && mapping == RunfilesProvider.DATA_RUNFILES) {
1001+
// Ensure that `DefaultInfo.files` of Starlark rules is merged in so that native rules
1002+
// interoperate well with idiomatic Starlark rules..
1003+
// https://bazel.build/extending/rules#runfiles_features_to_avoid
1004+
// Internal tests fail if the order of filesToBuild is preserved.
1005+
addTransitiveArtifacts(
1006+
NestedSetBuilder.<Artifact>stableOrder()
1007+
.addTransitive(target.getProvider(FileProvider.class).getFilesToBuild())
1008+
.build());
1009+
}
1010+
9911011
return addTargetExceptFileTargets(target, mapping);
9921012
}
9931013

‎src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -661,6 +661,13 @@ public boolean legacyExternalRunfiles() {
661661
return options.legacyExternalRunfiles;
662662
}
663663

664+
/**
665+
* Returns true if Runfiles should merge in FilesToBuild from deps when collecting data runfiles.
666+
*/
667+
public boolean alwaysIncludeFilesToBuildInData() {
668+
return options.alwaysIncludeFilesToBuildInData;
669+
}
670+
664671
/**
665672
* Returns user-specified test environment variables and their values, as set by the --test_env
666673
* options.

‎src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,18 @@ public ExecConfigurationDistinguisherSchemeConverter() {
433433
+ ".runfiles/wsname/external/repo (in addition to .runfiles/repo).")
434434
public boolean legacyExternalRunfiles;
435435

436+
@Option(
437+
name = "incompatible_always_include_files_in_data",
438+
defaultValue = "false",
439+
documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
440+
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
441+
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
442+
help =
443+
"If true, native rules add <code>DefaultInfo.files</code> of data dependencies to "
444+
+ "their runfiles, which matches the recommended behavior for Starlark rules ("
445+
+ "https://bazel.build/extending/rules#runfiles_features_to_avoid).")
446+
public boolean alwaysIncludeFilesToBuildInData;
447+
436448
@Option(
437449
name = "check_fileset_dependencies_recursively",
438450
defaultValue = "true",
@@ -931,6 +943,7 @@ public FragmentOptions getHost() {
931943
host.legacyExternalRunfiles = legacyExternalRunfiles;
932944
host.remotableSourceManifestActions = remotableSourceManifestActions;
933945
host.skipRunfilesManifests = skipRunfilesManifests;
946+
host.alwaysIncludeFilesToBuildInData = alwaysIncludeFilesToBuildInData;
934947

935948
// === Filesets ===
936949
host.strictFilesetOutput = strictFilesetOutput;

‎src/main/java/com/google/devtools/build/lib/rules/android/AndroidInstrumentationTestBase.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
8989
.addArtifact(testExecutable)
9090
.addArtifact(getInstrumentationApk(ruleContext))
9191
.addArtifact(getTargetApk(ruleContext))
92-
.addTargets(runfilesDeps, RunfilesProvider.DEFAULT_RUNFILES)
92+
.addTargets(
93+
runfilesDeps,
94+
RunfilesProvider.DEFAULT_RUNFILES,
95+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData())
9396
.addTransitiveArtifacts(AndroidCommon.getSupportApks(ruleContext))
9497
.addTransitiveArtifacts(getAdb(ruleContext).getFilesToRun())
9598
.merge(getAapt(ruleContext).getRunfilesSupport())

‎src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,10 @@ private Runfiles collectDefaultRunfiles(
469469
// runtime jars always in naive link order, incompatible with compile order runfiles.
470470
builder.addArtifacts(getRuntimeJarsForTargets(getAndCheckTestSupport(ruleContext)).toList());
471471

472-
builder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES);
472+
builder.addTargets(
473+
depsForRunfiles,
474+
RunfilesProvider.DEFAULT_RUNFILES,
475+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
473476

474477
// We assume that the runtime jars will not have conflicting artifacts
475478
// with the same root relative path

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -739,7 +739,10 @@ private void collectDefaultRunfiles(
739739
builder.addSymlinks(runfiles.getSymlinks());
740740
builder.addRootSymlinks(runfiles.getRootSymlinks());
741741
} else {
742-
builder.addTarget(defaultLauncher, RunfilesProvider.DEFAULT_RUNFILES);
742+
builder.addTarget(
743+
defaultLauncher,
744+
RunfilesProvider.DEFAULT_RUNFILES,
745+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
743746
}
744747
}
745748

@@ -748,7 +751,10 @@ private void collectDefaultRunfiles(
748751

749752
List<? extends TransitiveInfoCollection> runtimeDeps =
750753
ruleContext.getPrerequisites("runtime_deps");
751-
builder.addTargets(runtimeDeps, RunfilesProvider.DEFAULT_RUNFILES);
754+
builder.addTargets(
755+
runtimeDeps,
756+
RunfilesProvider.DEFAULT_RUNFILES,
757+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
752758

753759
builder.addTransitiveArtifactsWrappedInStableOrder(common.getRuntimeClasspath());
754760

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -886,11 +886,17 @@ public static Runfiles getRunfiles(
886886
depsForRunfiles.addAll(ruleContext.getPrerequisites("exports"));
887887
}
888888

889-
runfilesBuilder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES);
889+
runfilesBuilder.addTargets(
890+
depsForRunfiles,
891+
RunfilesProvider.DEFAULT_RUNFILES,
892+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
890893

891894
TransitiveInfoCollection launcher = JavaHelper.launcherForTarget(semantics, ruleContext);
892895
if (launcher != null) {
893-
runfilesBuilder.addTarget(launcher, RunfilesProvider.DATA_RUNFILES);
896+
runfilesBuilder.addTarget(
897+
launcher,
898+
RunfilesProvider.DATA_RUNFILES,
899+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
894900
}
895901

896902
semantics.addRunfilesForLibrary(ruleContext, runfilesBuilder);

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
140140
ruleContext.getConfiguration().legacyExternalRunfiles())
141141
// add the jars to the runfiles
142142
.addArtifacts(javaArtifacts.getRuntimeJars())
143-
.addTargets(targets, RunfilesProvider.DEFAULT_RUNFILES)
143+
.addTargets(
144+
targets,
145+
RunfilesProvider.DEFAULT_RUNFILES,
146+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData())
144147
.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES)
145148
.build();
146149

‎src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,15 @@ public ConfiguredTarget create(RuleContext ruleContext)
7979
directTestsAndSuitesBuilder.add(dep);
8080
}
8181

82-
Runfiles runfiles = new Runfiles.Builder(
83-
ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles())
84-
.addTargets(directTestsAndSuitesBuilder, RunfilesProvider.DATA_RUNFILES)
85-
.build();
82+
Runfiles runfiles =
83+
new Runfiles.Builder(
84+
ruleContext.getWorkspaceName(),
85+
ruleContext.getConfiguration().legacyExternalRunfiles())
86+
.addTargets(
87+
directTestsAndSuitesBuilder,
88+
RunfilesProvider.DATA_RUNFILES,
89+
ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData())
90+
.build();
8691

8792
return new RuleConfiguredTargetBuilder(ruleContext)
8893
.add(RunfilesProvider.class,

‎src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,134 @@ public void testDefaultInfoWithRunfilesConstructor() throws Exception {
713713
assertThat(getConfiguredTarget("//src:r_tools")).isNotNull();
714714
}
715715

716+
@Test
717+
public void testDefaultInfoFilesAddedToCcBinaryTargetRunfiles() throws Exception {
718+
scratch.file(
719+
"test/starlark/extension.bzl",
720+
"def custom_rule_impl(ctx):",
721+
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
722+
" ctx.actions.write(out, 'foobar')",
723+
" return [DefaultInfo(files = depset([out]))]",
724+
"",
725+
"custom_rule = rule(implementation = custom_rule_impl)");
726+
727+
scratch.file(
728+
"test/starlark/BUILD",
729+
"load('//test/starlark:extension.bzl', 'custom_rule')",
730+
"",
731+
"custom_rule(name = 'cr')",
732+
"cc_binary(name = 'binary', data = [':cr'])");
733+
734+
useConfiguration("--incompatible_always_include_files_in_data");
735+
ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
736+
737+
assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
738+
assertThat(
739+
ActionsTestUtil.baseArtifactNames(
740+
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
741+
.contains("cr.out");
742+
assertThat(
743+
ActionsTestUtil.baseArtifactNames(
744+
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
745+
.contains("cr.out");
746+
}
747+
748+
@Test
749+
public void testDefaultInfoFilesAddedToJavaBinaryTargetRunfiles() throws Exception {
750+
scratch.file(
751+
"test/starlark/extension.bzl",
752+
"def custom_rule_impl(ctx):",
753+
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
754+
" ctx.actions.write(out, 'foobar')",
755+
" return [DefaultInfo(files = depset([out]))]",
756+
"",
757+
"custom_rule = rule(implementation = custom_rule_impl)");
758+
759+
scratch.file(
760+
"test/starlark/BUILD",
761+
"load('//test/starlark:extension.bzl', 'custom_rule')",
762+
"",
763+
"custom_rule(name = 'cr')",
764+
"java_binary(name = 'binary', data = [':cr'], srcs = ['Foo.java'], main_class = 'Foo')");
765+
766+
useConfiguration("--incompatible_always_include_files_in_data");
767+
ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
768+
769+
assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
770+
assertThat(
771+
ActionsTestUtil.baseArtifactNames(
772+
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
773+
.contains("cr.out");
774+
assertThat(
775+
ActionsTestUtil.baseArtifactNames(
776+
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
777+
.contains("cr.out");
778+
}
779+
780+
@Test
781+
public void testDefaultInfoFilesAddedToPyBinaryTargetRunfiles() throws Exception {
782+
scratch.file(
783+
"test/starlark/extension.bzl",
784+
"def custom_rule_impl(ctx):",
785+
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
786+
" ctx.actions.write(out, 'foobar')",
787+
" return [DefaultInfo(files = depset([out]))]",
788+
"",
789+
"custom_rule = rule(implementation = custom_rule_impl)");
790+
791+
scratch.file(
792+
"test/starlark/BUILD",
793+
"load('//test/starlark:extension.bzl', 'custom_rule')",
794+
"",
795+
"custom_rule(name = 'cr')",
796+
"py_binary(name = 'binary', data = [':cr'], srcs = ['binary.py'])");
797+
798+
useConfiguration("--incompatible_always_include_files_in_data");
799+
ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
800+
801+
assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
802+
assertThat(
803+
ActionsTestUtil.baseArtifactNames(
804+
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
805+
.contains("cr.out");
806+
assertThat(
807+
ActionsTestUtil.baseArtifactNames(
808+
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
809+
.contains("cr.out");
810+
}
811+
812+
@Test
813+
public void testDefaultInfoFilesAddedToShBinaryTargetRunfiles() throws Exception {
814+
scratch.file(
815+
"test/starlark/extension.bzl",
816+
"def custom_rule_impl(ctx):",
817+
" out = ctx.actions.declare_file(ctx.attr.name + '.out')",
818+
" ctx.actions.write(out, 'foobar')",
819+
" return [DefaultInfo(files = depset([out]))]",
820+
"",
821+
"custom_rule = rule(implementation = custom_rule_impl)");
822+
823+
scratch.file(
824+
"test/starlark/BUILD",
825+
"load('//test/starlark:extension.bzl', 'custom_rule')",
826+
"",
827+
"custom_rule(name = 'cr')",
828+
"sh_binary(name = 'binary', data = [':cr'], srcs = ['script.sh'])");
829+
830+
useConfiguration("--incompatible_always_include_files_in_data");
831+
ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
832+
833+
assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
834+
assertThat(
835+
ActionsTestUtil.baseArtifactNames(
836+
target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
837+
.contains("cr.out");
838+
assertThat(
839+
ActionsTestUtil.baseArtifactNames(
840+
target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
841+
.contains("cr.out");
842+
}
843+
716844
@Test
717845
public void testInstrumentedFilesProviderWithCodeCoverageDisabled() throws Exception {
718846
setBuildLanguageOptions("--incompatible_disallow_struct_provider_syntax=false");

0 commit comments

Comments
 (0)