I'm not entirely sure whether the correct behavior would be to merge in the data_runfiles or the default_runfiles of srcs as these have been deprecated for a while and Starlark rules rarely distinguish between the two. @lberki Are you able to make the call? :-)

Testing in downstream: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/4493

Thanks, the run looks good, all failures are caused by known issues.

Nice, @comius @lberki please double check this change is safe.

Ow. This is definitely a change that I'd love to land (DATA_RUNFILES should die) but also has a chance that it'd break things at Google. How about this: I'll kick off a test run for Google's tests tomorrow morning (Europe) (it's already too late for that today) and report back. If it comes back clean, let's submit it, if not, depending on the amount of fallout, either we'll fix it at Google or you go with the sh_library approach.

Hm?

Sounds good!

Update: the overnight test sampling run (all I could do overnight) came back clean and I kicked off the full test run. Results should be in in a few hours.

I ran our internal test battery and it looks fine. Let's merge this and thanks for your patience!

@lberki Can you share more details on the reason for the rollback?

It's a few failing tests with quite possible a single root cause that I keep not finding enough time to debug :(

I did some debugging (for Googlers, link is b/405063764) and it looks like there are three different root causes, all because this change adds some extra files to the runfiles trees of some tests, which tips two tests over the maximum size of nput files and confuses one test.

I'm afraid this'll require quite some debugging on our part.

Looking at this more deeply, I'm not entirely sure that this is the right fix. I do realize that there is a comment that says that filegroups should visit their data dependencies as data and their srcs as default, but one could argue that semantically, not doing the latter works better.

Is there an alternative fix? I'm currently elbow deep into some hairy Starlark rules to figure out where the extra files come from and I'm not sure if I can fix these without causing collateral damage.

I tracked down one of the failures and it's indeed as I suspected. The structure is like this:

cc_library(name="lib", data=[":one"]
filegroup(name="one", data=[":big"])
filegroup(name="big", srcs=[<big files>])

And this change adds <big files> to the data runfiles of :big, therefore, to the regular runfiles of :lib. Which is not unreasonable.

I would go as far as calling that the only reasonable behavior, but I can see how that would require some cleanup. :⁠-⁠)

I would go as far as calling that the only reasonable behavior, but I can see how that would require some cleanup. :⁠-⁠)

Why's that? My mental model is that the concept of "data runfiles" is not very well defined and if you accept that as a fact, it's difficult to argue for any sort of behavior based on principles.

Did you mean to write "to the data runfiles of :one" above? My mental model always has been that a data dependency edge translates data runfiles into ordinary runfiles.

A different perspective: filegroup(..., srcs = [...]) should probably be referentially transparent in the sense that

cc_library(name="lib", data=[":one"]
filegroup(name="one", data=[":big"])
filegroup(name="big", srcs=["big1", "big2"])

behaves just like

cc_library(name="lib", data=[":one"]
filegroup(name="one", data=["big1", "big2"])

whatever data semantics are precisely.

@fmeum that's a sound line of reasoning, although if you think a step deeper, the inevitable conclusion is that since filegroup is just, well, a group of files, the "data" attribute on it doesn't make sense at all. But there we are...

Did you mean to write "to the data runfiles of :one" above?

Technically, yes, to the data runfiles of :one, but those eventually land in the regular runfiles of ``:lib` also.

@lberki I took another look at the motivating issue #25513, which doesn't use the data attribute on filegroup. The dependency structure is equivalent to:

sh_test(name = "test", srcs = ["test.sh"], data = [":filegroup"])
filegroup(name = "filegroup", srcs = [":binary"])
sh_binary(name = "binary", srcs = ["binary.sh"], data = ["foo.txt"])

I find it extremely surprising that the runfile foo.txt is lost in this case. As a result, filegroup practically isn't safe to use in data attributes of any rule as long as it references binary targets. This is particularly bad for Bazel, where the filegroup may be authored by a first party user while the binary target comes from a ruleset.

I'm ultimately fine with any kind of semantics of filegroup(data = ...) as long as we can preserve existing runfiles for filegroup(srcs = ...). We could also guard the new behavior behind a flag that is never flipped for Blaze.

@lberki Do you think if it's feasible to just fix the internal tests? If so, I can also help with that.

It's at least not easy. The obvious fix would be to merge the runfiles so that what filegroup() does is the same regardless of which attribute the files are under, but it's been experimentally proven that that causes "too many input files" errors.

@fmeum Sorry, it looks like it'll take a significant amount of work to fix google internal projects, is there any other workaround to make Bazel's downstream green again?

I'll send a targeted fix.