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

Jump to bottom

Add missing deps to integration test setup #25329

Closed
wants to merge 1 commit into from
Closed

Add missing deps to integration test setup #25329

wants to merge 1 commit into from

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

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
Open
# for free to join this conversation on GitHub. Already have an account? # 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
5 participants
@fmeum @mai93 @meteorcloudy @Wyverald @sgowroji