From e98e04b07f02ce1aa613b02e7675acd94bcaaba4 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 7 Nov 2022 08:06:52 -0800 Subject: [PATCH] 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 #15043 Closes #15052. PiperOrigin-RevId: 486663801 Change-Id: I4a30b8b8a4dfc84bf4de27fd7894dd48e795c081 --- .../devtools/build/lib/analysis/Runfiles.java | 36 +++-- .../config/BuildConfigurationValue.java | 7 + .../lib/analysis/config/CoreOptions.java | 13 ++ .../AndroidInstrumentationTestBase.java | 5 +- .../rules/android/AndroidLocalTestBase.java | 5 +- .../build/lib/rules/java/JavaBinary.java | 10 +- .../build/lib/rules/java/JavaCommon.java | 10 +- .../build/lib/rules/java/JavaImport.java | 5 +- .../build/lib/rules/test/TestSuite.java | 13 +- .../lib/starlark/StarlarkIntegrationTest.java | 128 ++++++++++++++++++ 10 files changed, 213 insertions(+), 19 deletions(-) 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 9a59733973261b..19597cc44c23d8 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 @@ -636,6 +636,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 f4f83bb319ff78..a7fbb8be55f51a 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", @@ -930,6 +942,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");