Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Move Bash and Python runfiles libraries out of Bazel #24219

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Nov 6, 2024

The Bash and Python runfiles libraries in @bazel_tools are replaced with aliases to @rules_<lang>//<lang>/runfiles.

This requires skipping load visibility checks for resolved WORKSPACE files, which are going away anyway.

@fmeum fmeum changed the title Move Bash runfiles library out of Bazel Move runfiles libraries out of Bazel Nov 6, 2024
@fmeum fmeum force-pushed the drop-shell-runfiles-library branch 3 times, most recently from 973d791 to 46db68e Compare November 6, 2024 11:16
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 6, 2024

FYI @comius, let me know if you want to merge this as is (Bash + Python).

@comius
Copy link
Contributor

comius commented Nov 19, 2024

FYI @comius, let me know if you want to merge this as is (Bash + Python).

It looks ok to me.

@fmeum fmeum marked this pull request as ready for review November 19, 2024 16:05
@fmeum fmeum changed the title Move runfiles libraries out of Bazel Move Bash and Python runfiles libraries out of Bazel Nov 19, 2024
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Nov 19, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 19, 2024

@comius I updated the description, feel free to merge this. I will open a new PR for Java and C++.

@fmeum fmeum requested a review from comius November 19, 2024 18:12
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 19, 2024

@bazel-io fork 8.0.0

@comius comius 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 Nov 20, 2024
@iancha1992
Copy link
Member

@fmeum Can you please resolve the conflicts? Thanks!

@fmeum fmeum force-pushed the drop-shell-runfiles-library branch from 46db68e to 006cf31 Compare November 20, 2024 22:25
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 20, 2024

@iancha1992 Done!

@iancha1992
Copy link
Member

@fmeum Looks like one of the tests is failing

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 21, 2024

@iancha1992 Looks like CI has trouble connecting to github.com.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 21, 2024

@iancha1992 I sent #24425 to fix this, I guess that the failing tests just aren't supposed to hit anything other than the mirror.

@fmeum fmeum force-pushed the drop-shell-runfiles-library branch from 006cf31 to 16818bb Compare November 21, 2024 13:21
@fmeum fmeum force-pushed the drop-shell-runfiles-library branch from 4378c2f to 30f2982 Compare November 22, 2024 15:10
@fmeum fmeum force-pushed the drop-shell-runfiles-library branch from 30f2982 to c7d4613 Compare November 22, 2024 15:14
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 22, 2024

@meteorcloudy The remaining failure is due to --experimental_repository_resolved_file not working when a resolved repo rule has limited load visibility. Since this functionality is WORKSPACE only, should I fix it?

@meteorcloudy
Copy link
Member

Since this functionality is WORKSPACE only, should I fix it?

I don't think so, can we work around this in the test? This feature will just go way when we delete WORKSPACE.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 26, 2024

@meteorcloudy I fixed the root cause, it just took one line.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 29, 2024

@bazel-io flag

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 29, 2024

(for 8.1.0)

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 29, 2024
@iancha1992
Copy link
Member

@bazel-io fork 8.0.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 2, 2024
@alexeagle
Copy link
Contributor

@iancha1992 @fmeum I just noticed maybe a mistake here, Fabian intended it for 8.1.0 but it was flagged for 8.0.0?

@iancha1992
Copy link
Member

@alexeagle We'll move it to 8.1.0 milestone if it doesn't make it in time.

@meteorcloudy
Copy link
Member

@fmeum Is this change actually backwards-compatible? Wouldn't users need to update the runfiles boilerplate like?
https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/integration/bazel_query_test.sh;l=23-34

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 3, 2024

@fmeum Is this change actually backwards-compatible? Wouldn't users need to update the runfiles boilerplate like?
https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/integration/bazel_query_test.sh;l=23-34

It is meant to be backwards compatible, due to some trickery with root symlinks: https://github.com/bazelbuild/rules_shell/blob/104505faea10e1fe50bb7f7a5b3142b63181e644/shell/runfiles/BUILD#L19

@meteorcloudy
Copy link
Member

I see, we were having some trouble importing this change, I guess internal we still have to keep a copy of runfiles.bash at the old location.

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 3, 2024

I see, we were having some trouble importing this change, I guess internal we still have to keep a copy of runfiles.bash at the old location.

I suspect that's because your copybara rewrite for the runfiles path doesn't apply to the BUILD file line I linked. That should be the only place that needs changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally team-Rules-Python Native rules for Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants