diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java index 9d8b10955fa52d..d9933d60b19561 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java @@ -935,7 +935,10 @@ public Builder add( /** Collects runfiles from data dependencies of a target. */ @CanIgnoreReturnValue public Builder addDataDeps(RuleContext ruleContext) { - addTargets(getPrerequisites(ruleContext, "data"), RunfilesProvider.DATA_RUNFILES); + addTargets( + getPrerequisites(ruleContext, "data"), + RunfilesProvider.DATA_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()); return this; } @@ -952,16 +955,20 @@ public Builder addNonDataDeps( @CanIgnoreReturnValue public Builder addTargets( Iterable targets, - Function mapping) { + Function mapping, + boolean alwaysIncludeFilesToBuildInData) { for (TransitiveInfoCollection target : targets) { - addTarget(target, mapping); + addTarget(target, mapping, alwaysIncludeFilesToBuildInData); } return this; } - public Builder addTarget(TransitiveInfoCollection target, - Function mapping) { - return addTargetIncludingFileTargets(target, mapping); + @CanIgnoreReturnValue + public Builder addTarget( + TransitiveInfoCollection target, + Function mapping, + boolean alwaysIncludeFilesToBuildInData) { + return addTargetIncludingFileTargets(target, mapping, alwaysIncludeFilesToBuildInData); } @CanIgnoreReturnValue @@ -975,8 +982,10 @@ private Builder addTargetExceptFileTargets( return this; } - private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target, - Function mapping) { + private Builder addTargetIncludingFileTargets( + TransitiveInfoCollection target, + Function mapping, + boolean alwaysIncludeFilesToBuildInData) { if (target.getProvider(RunfilesProvider.class) == null && mapping == RunfilesProvider.DATA_RUNFILES) { // RuleConfiguredTarget implements RunfilesProvider, so this will only be called on @@ -988,6 +997,17 @@ private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target, return this; } + if (alwaysIncludeFilesToBuildInData && mapping == RunfilesProvider.DATA_RUNFILES) { + // Ensure that `DefaultInfo.files` of Starlark rules is merged in so that native rules + // interoperate well with idiomatic Starlark rules.. + // https://bazel.build/extending/rules#runfiles_features_to_avoid + // Internal tests fail if the order of filesToBuild is preserved. + addTransitiveArtifacts( + NestedSetBuilder.stableOrder() + .addTransitive(target.getProvider(FileProvider.class).getFilesToBuild()) + .build()); + } + return addTargetExceptFileTargets(target, mapping); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 6adb56718af482..f2770c01f111a2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -661,6 +661,13 @@ public boolean legacyExternalRunfiles() { return options.legacyExternalRunfiles; } + /** + * Returns true if Runfiles should merge in FilesToBuild from deps when collecting data runfiles. + */ + public boolean alwaysIncludeFilesToBuildInData() { + return options.alwaysIncludeFilesToBuildInData; + } + /** * Returns user-specified test environment variables and their values, as set by the --test_env * options. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 711ca92c626551..7d4aeef0580a1d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -433,6 +433,18 @@ public ExecConfigurationDistinguisherSchemeConverter() { + ".runfiles/wsname/external/repo (in addition to .runfiles/repo).") public boolean legacyExternalRunfiles; + @Option( + name = "incompatible_always_include_files_in_data", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If true, native rules add DefaultInfo.files of data dependencies to " + + "their runfiles, which matches the recommended behavior for Starlark rules (" + + "https://bazel.build/extending/rules#runfiles_features_to_avoid).") + public boolean alwaysIncludeFilesToBuildInData; + @Option( name = "check_fileset_dependencies_recursively", defaultValue = "true", @@ -931,6 +943,7 @@ public FragmentOptions getHost() { host.legacyExternalRunfiles = legacyExternalRunfiles; host.remotableSourceManifestActions = remotableSourceManifestActions; host.skipRunfilesManifests = skipRunfilesManifests; + host.alwaysIncludeFilesToBuildInData = alwaysIncludeFilesToBuildInData; // === Filesets === host.strictFilesetOutput = strictFilesetOutput; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidInstrumentationTestBase.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidInstrumentationTestBase.java index 45720b13462f39..344ebf23861505 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidInstrumentationTestBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidInstrumentationTestBase.java @@ -89,7 +89,10 @@ public ConfiguredTarget create(RuleContext ruleContext) .addArtifact(testExecutable) .addArtifact(getInstrumentationApk(ruleContext)) .addArtifact(getTargetApk(ruleContext)) - .addTargets(runfilesDeps, RunfilesProvider.DEFAULT_RUNFILES) + .addTargets( + runfilesDeps, + RunfilesProvider.DEFAULT_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()) .addTransitiveArtifacts(AndroidCommon.getSupportApks(ruleContext)) .addTransitiveArtifacts(getAdb(ruleContext).getFilesToRun()) .merge(getAapt(ruleContext).getRunfilesSupport()) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java index 809fc927736960..33905dd3b0a9bf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java @@ -469,7 +469,10 @@ private Runfiles collectDefaultRunfiles( // runtime jars always in naive link order, incompatible with compile order runfiles. builder.addArtifacts(getRuntimeJarsForTargets(getAndCheckTestSupport(ruleContext)).toList()); - builder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES); + builder.addTargets( + depsForRunfiles, + RunfilesProvider.DEFAULT_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()); // We assume that the runtime jars will not have conflicting artifacts // with the same root relative path diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java index dfb04a255166f7..4734c3a1754af8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java @@ -739,7 +739,10 @@ private void collectDefaultRunfiles( builder.addSymlinks(runfiles.getSymlinks()); builder.addRootSymlinks(runfiles.getRootSymlinks()); } else { - builder.addTarget(defaultLauncher, RunfilesProvider.DEFAULT_RUNFILES); + builder.addTarget( + defaultLauncher, + RunfilesProvider.DEFAULT_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()); } } @@ -748,7 +751,10 @@ private void collectDefaultRunfiles( List runtimeDeps = ruleContext.getPrerequisites("runtime_deps"); - builder.addTargets(runtimeDeps, RunfilesProvider.DEFAULT_RUNFILES); + builder.addTargets( + runtimeDeps, + RunfilesProvider.DEFAULT_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()); builder.addTransitiveArtifactsWrappedInStableOrder(common.getRuntimeClasspath()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java index b6e2245c82e379..b382b1f39f7a36 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java @@ -886,11 +886,17 @@ public static Runfiles getRunfiles( depsForRunfiles.addAll(ruleContext.getPrerequisites("exports")); } - runfilesBuilder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES); + runfilesBuilder.addTargets( + depsForRunfiles, + RunfilesProvider.DEFAULT_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()); TransitiveInfoCollection launcher = JavaHelper.launcherForTarget(semantics, ruleContext); if (launcher != null) { - runfilesBuilder.addTarget(launcher, RunfilesProvider.DATA_RUNFILES); + runfilesBuilder.addTarget( + launcher, + RunfilesProvider.DATA_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()); } semantics.addRunfilesForLibrary(ruleContext, runfilesBuilder); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java index a4d1d0556d3529..d3ed1d7f97a47f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java @@ -140,7 +140,10 @@ public ConfiguredTarget create(RuleContext ruleContext) ruleContext.getConfiguration().legacyExternalRunfiles()) // add the jars to the runfiles .addArtifacts(javaArtifacts.getRuntimeJars()) - .addTargets(targets, RunfilesProvider.DEFAULT_RUNFILES) + .addTargets( + targets, + RunfilesProvider.DEFAULT_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()) .addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java index 40eeb359852495..07ad56196be619 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java @@ -79,10 +79,15 @@ public ConfiguredTarget create(RuleContext ruleContext) directTestsAndSuitesBuilder.add(dep); } - Runfiles runfiles = new Runfiles.Builder( - ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles()) - .addTargets(directTestsAndSuitesBuilder, RunfilesProvider.DATA_RUNFILES) - .build(); + Runfiles runfiles = + new Runfiles.Builder( + ruleContext.getWorkspaceName(), + ruleContext.getConfiguration().legacyExternalRunfiles()) + .addTargets( + directTestsAndSuitesBuilder, + RunfilesProvider.DATA_RUNFILES, + ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData()) + .build(); return new RuleConfiguredTargetBuilder(ruleContext) .add(RunfilesProvider.class, diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java index 8ba84526fd64b9..0fad5fa2d6cb1e 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java @@ -713,6 +713,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'])"); + + useConfiguration("--incompatible_always_include_files_in_data"); + 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')"); + + useConfiguration("--incompatible_always_include_files_in_data"); + 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'])"); + + useConfiguration("--incompatible_always_include_files_in_data"); + 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'])"); + + useConfiguration("--incompatible_always_include_files_in_data"); + 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");