Skip to content

Add missing deps to integration test setup#25329

Closed
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:25323-fix-integration-test-setup
Closed

Add missing deps to integration test setup#25329
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:25323-fix-integration-test-setup

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 19, 2025

Fixes #25323
Fixes #25324
Fixes #25325
Fixes #25306

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 19, 2025

@sgowroji

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Feb 19, 2025
@sgowroji sgowroji added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Feb 19, 2025
@mai93
Copy link
Contributor

mai93 commented Feb 19, 2025

@fmeum will this also fix bazelbuild/examples#556?

Wyverald
Wyverald approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this'll cause any difficulties during import, but I guess we can't know without trying :)

@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Feb 19, 2025
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 20, 2025
@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 24, 2025

https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/4492#01953622-7720-4201-a255-1fb3970ec121
Still many tests failed with:

ERROR: cannot find bazel_tools/tools/bash/runfiles/runfiles.bash

@fmeum fmeum deleted the 25323-fix-integration-test-setup branch February 24, 2025 11:10
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 24, 2025

@meteorcloudy I can turn more filegroups into sh_librarys to fix this, but it's actually a bug in filegroup that swallows runfiles:

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

Lines 93 to 98 in d5e62f1

// If you're visiting a filegroup as data, then we also visit its data as data.
new Runfiles.Builder(
ruleContext.getWorkspaceName(), configuration.legacyExternalRunfiles())
.addTransitiveArtifacts(filesToBuild)
.addDataDeps(ruleContext)
.build());

The comment says "also visit its data as data", but then only visits its data as data, ignoring the runfiles of srcs. I don't think it's correct for a data edge to merge strictly fewer runfiles than a non-data edge. I can send a fix so that we know how much this would break.

@comius Are there any thoughts on Starlarkifying filegroup to make its exact semantics more obvious?

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 24, 2025 •
edited
Loading

#25365 fixes this particular issue, but changing filegroup logic that has been unchanged since at least 2015 is scary.

CC @lberki

@fmeum fmeum mentioned this pull request Feb 24, 2025
Closed
copybara-service bot pushed a commit that referenced this pull request Mar 1, 2025
@fmeum @copybara-github
Don't swallow runfiles of srcs in data runfiles of filegroup
15a3526 
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)

Closes #25365.

PiperOrigin-RevId: 732362664
Change-Id: I00786a362bd61b56d951e9e2bedb4ded04786994
@lberki
Copy link
Contributor

lberki commented Mar 5, 2025

Ow, I'll have to roll this back -- this actually broke things at Google in a highly non-trivial way and it was hidden behind some confusing UI. Stay tuned!

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 5, 2025

Now I want to know what that UI is. :⁠-⁠)

copybara-service bot pushed a commit that referenced this pull request Mar 5, 2025
@pzembrod @copybara-github
Automated rollback of commit 15a3526.
a4f6814 
*** Reason for rollback ***

b/400902205, breaks blaze nightly build 2025-03-01 ff.

*** Original change description ***

Don't swallow runfiles of `srcs` in data runfiles of `filegroup`

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)

Closes #25365.

PiperOrigin-RevId: 733682081
Change-Id: Ibdc06c70e585758fe19ff8666aa4a02adaa0d348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Wyverald Wyverald Wyverald approved these changes

@meteorcloudy meteorcloudy Awaiting requested review from meteorcloudy

Assignees

No one assigned

Labels

team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website

Projects

None yet

Milestone

No milestone

6 participants

@fmeum @mai93 @meteorcloudy @lberki @Wyverald @sgowroji