Add --incompatible_filegroup_runfiles_for_data

If true, runfiles of targets listed in the srcs attribute of a filegroup are available to targets that consume the filegroup as a data dependency.

Reland of #25365 behind an incompatible flag

Closes #25978.

PiperOrigin-RevId: 772841978
Change-Id: Ia8f126bb33e02969eafdbd6972c83f5350d58be2

Author: fmeum

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

@@ -62,6 +62,17 @@
  */
 public class CoreOptions extends FragmentOptions implements Cloneable {
 
+  @Option(
+      name = "incompatible_filegroup_runfiles_for_data",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "If true, runfiles of targets listed in the srcs attribute are available to targets"
+              + " that consume the filegroup as a data dependency.")
+  public boolean filegroupRunfilesForData;
+
   @Option(
       name = "scl_config",
       defaultValue = "null",

src/main/java/com/google/devtools/build/lib/rules/BUILD

@@ -149,11 +149,10 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/instrumented_files_info",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
-        "//src/main/java/com/google/devtools/build/lib/analysis/config:build_configuration",
+        "//src/main/java/com/google/devtools/build/lib/analysis/config:core_options",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
-        "//third_party:guava",
         "//third_party:jsr305",
     ],
 )

src/main/java/com/google/devtools/build/lib/rules/filegroup/Filegroup.java

@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector.InstrumentationSpec;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
@@ -38,9 +39,7 @@
 import java.util.List;
 import javax.annotation.Nullable;
 
-/**
- * ConfiguredTarget for "filegroup".
- */
+/** ConfiguredTarget for "filegroup". */
 public class Filegroup implements RuleConfiguredTargetFactory {
 
   /** Error message for output groups that are explicitly forbidden from filegroup reference. */
@@ -81,16 +80,25 @@ public ConfiguredTarget create(RuleContext ruleContext)
             new InstrumentationSpec(FileTypeSet.ANY_FILE).withDependencyAttributes("srcs", "data"),
             /* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER));
 
+    // If you're visiting a filegroup as data, then we also visit its data as data.
+    var dataRunfilesBuilder =
+        new Runfiles.Builder(ruleContext.getWorkspaceName()).addTransitiveArtifacts(filesToBuild);
+    if (ruleContext
+        .getConfiguration()
+        .getOptions()
+        .get(CoreOptions.class)
+        .filegroupRunfilesForData) {
+      // If you're visiting a filegroup as data, then we also visit its data as data.
+      dataRunfilesBuilder.addRunfiles(ruleContext, RunfilesProvider.DATA_RUNFILES);
+    } else {
+      dataRunfilesBuilder.addDataDeps(ruleContext);
+    }
     RunfilesProvider runfilesProvider =
         RunfilesProvider.withData(
             new Runfiles.Builder(ruleContext.getWorkspaceName())
                 .addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES)
                 .build(),
-            // If you're visiting a filegroup as data, then we also visit its data as data.
-            new Runfiles.Builder(ruleContext.getWorkspaceName())
-                .addTransitiveArtifacts(filesToBuild)
-                .addDataDeps(ruleContext)
-                .build());
+            dataRunfilesBuilder.build());
 
     RuleConfiguredTargetBuilder builder =
         new RuleConfiguredTargetBuilder(ruleContext)

src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl

@@ -273,6 +273,7 @@ bazel_fragments["CoreOptions"] = fragment(
         "//command_line_option:experimental_inprocess_symlink_creation",
         "//command_line_option:experimental_throttle_action_cache_check",
         "//command_line_option:experimental_use_platforms_in_output_dir_legacy_heuristic",
+        "//command_line_option:incompatible_filegroup_runfiles_for_data",
     ],
     inputs = ["//command_line_option:features"],
     outputs = [

src/test/java/com/google/devtools/build/lib/rules/filegroup/BUILD

@@ -24,8 +24,10 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/test/java/com/google/devtools/build/lib/actions/util",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
+        "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
+        "@maven//:com_google_testparameterinjector_test_parameter_injector",
     ],
 )
 

src/test/java/com/google/devtools/build/lib/rules/filegroup/FilegroupConfiguredTargetTest.java

@@ -16,22 +16,22 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
 import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.rules.java.JavaSemantics;
 import com.google.devtools.build.lib.util.FileType;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.io.IOException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
-/**
- * Tests for {@link Filegroup}.
- */
-@RunWith(JUnit4.class)
+/** Tests for {@link Filegroup}. */
+@RunWith(TestParameterInjector.class)
 public class FilegroupConfiguredTargetTest extends BuildViewTestCase {
 
   @Test
@@ -219,4 +219,87 @@ public void testErrorForIllegalOutputGroup() throws Exception {
         .contains(
             String.format(Filegroup.ILLEGAL_OUTPUT_GROUP_ERROR, OutputGroupInfo.HIDDEN_TOP_LEVEL));
   }
+
+  @Test
+  public void testDefaultInfo(@TestParameter boolean filegroupRunfilesForData) throws Exception {
+    scratch.file(
+        "x/defs.bzl",
+        """
+        def _default_info_impl(ctx):
+            files = depset(transitive = [t[DefaultInfo].files for t in ctx.attr.files])
+            default_runfiles = ctx.runfiles(transitive_files = depset(transitive = [t[DefaultInfo].files for t in ctx.attr.default_runfiles]))
+            data_runfiles = ctx.runfiles(transitive_files = depset(transitive = [t[DefaultInfo].files for t in ctx.attr.data_runfiles]))
+            return [
+                DefaultInfo(
+                    files = files,
+                    default_runfiles = default_runfiles,
+                    data_runfiles = data_runfiles,
+                )
+            ]
+        default_info = rule(
+            implementation = _default_info_impl,
+            attrs = {
+                "files": attr.label_list(allow_files=True),
+                "default_runfiles": attr.label_list(allow_files=True),
+                "data_runfiles": attr.label_list(allow_files=True),
+            },
+        )
+        """);
+    scratch.file(
+        "x/BUILD",
+        """
+        load(":defs.bzl", "default_info")
+
+        default_info(
+            name = "default_info_srcs",
+            files = ["srcs_files_file"],
+            default_runfiles = ["srcs_default_runfiles_file"],
+            data_runfiles = ["srcs_data_runfiles_file"],
+        )
+
+        default_info(
+            name = "default_info_data",
+            files = ["data_files"],
+            default_runfiles = ["data_default_runfiles_file"],
+            data_runfiles = ["data_data_runfiles_file"],
+        )
+
+        filegroup(
+            name = "filegroup",
+            srcs = [
+                ":default_info_srcs",
+                "srcs_file",
+            ],
+            data = [
+                ":default_info_data",
+                "data_file",
+            ],
+        )
+        """);
+
+    useConfiguration("--incompatible_filegroup_runfiles_for_data=" + filegroupRunfilesForData);
+    var filegroup = getConfiguredTarget("//x:filegroup");
+
+    assertThat(ActionsTestUtil.prettyArtifactNames(getFilesToBuild(filegroup)))
+        .containsExactly("x/srcs_file", "x/srcs_files_file");
+    assertThat(ActionsTestUtil.prettyArtifactNames(getDefaultRunfiles(filegroup).getArtifacts()))
+        .containsExactly(
+            "x/srcs_default_runfiles_file",
+            "x/data_file",
+            "x/data_files",
+            "x/data_data_runfiles_file");
+    var expectedDataRunfiles =
+        ImmutableSet.<String>builder()
+            .add(
+                "x/srcs_file",
+                "x/srcs_files_file",
+                "x/data_file",
+                "x/data_files",
+                "x/data_data_runfiles_file");
+    if (filegroupRunfilesForData) {
+      expectedDataRunfiles.add("x/srcs_data_runfiles_file");
+    }
+    assertThat(ActionsTestUtil.prettyArtifactNames(getDataRunfiles(filegroup).getArtifacts()))
+        .containsExactlyElementsIn(expectedDataRunfiles.build());
+  }
 }