Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don't swallow runfiles of srcs in data runfiles of filegroup #25365

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 24, 2025

The data_runfiles of a filegroup should contain the data_runfiles of all deps in srcs, plus the data_runfiles of all deps in data. Previously, they didn't contain the former.

Related to #25329 (comment)

The `data_runfiles` of a `filegroup` should contain the `data_runfiles`
of all deps in `srcs`, plus the `data_runfiles` of all deps in `data`.
Previously, they didn't contain the former.
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 24, 2025

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? :-)

@fmeum fmeum marked this pull request as ready for review February 24, 2025 12:04
@fmeum fmeum requested a review from lberki as a code owner February 24, 2025 12:04
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Feb 24, 2025
@meteorcloudy
Copy link
Member

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 24, 2025

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

@meteorcloudy
Copy link
Member

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

@iancha1992 iancha1992 added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Feb 25, 2025
@lberki
Copy link
Contributor

lberki commented Feb 26, 2025

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?

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 26, 2025

Sounds good!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants