Always collect FileProvider's filesToBuild as data runfiles

fmeum · copybara-github · commit 95f9adc5ac54 · 2022-11-07T08:08:09.000-08:00
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
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
@@ -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<? extends TransitiveInfoCollection> targets,
-        Function<TransitiveInfoCollection, Runfiles> mapping) {
+        Function<TransitiveInfoCollection, Runfiles> mapping,
+        boolean alwaysIncludeFilesToBuildInData) {
       for (TransitiveInfoCollection target : targets) {
-        addTarget(target, mapping);
+        addTarget(target, mapping, alwaysIncludeFilesToBuildInData);
       }
       return this;
     }
 
-    public Builder addTarget(TransitiveInfoCollection target,
-        Function<TransitiveInfoCollection, Runfiles> mapping) {
-      return addTargetIncludingFileTargets(target, mapping);
+    @CanIgnoreReturnValue
+    public Builder addTarget(
+        TransitiveInfoCollection target,
+        Function<TransitiveInfoCollection, Runfiles> mapping,
+        boolean alwaysIncludeFilesToBuildInData) {
+      return addTargetIncludingFileTargets(target, mapping, alwaysIncludeFilesToBuildInData);
     }
 
     @CanIgnoreReturnValue
@@ -975,8 +982,10 @@ private Builder addTargetExceptFileTargets(
       return this;
     }
 
-    private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target,
-        Function<TransitiveInfoCollection, Runfiles> mapping) {
+    private Builder addTargetIncludingFileTargets(
+        TransitiveInfoCollection target,
+        Function<TransitiveInfoCollection, Runfiles> 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.<Artifact>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
@@ -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
@@ -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 <code>DefaultInfo.files</code> 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
@@ -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
@@ -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
@@ -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<? extends TransitiveInfoCollection> 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
@@ -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
@@ -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
@@ -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
@@ -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");
