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

Add missing deps to integration test setup #25329

Closed

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?

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:

// 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

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

CC @lberki

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
5 participants