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.

Sign up for GitHub

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

Jump to bottom

Gracefully handle output symlinks with BwoB #18075

Closed
wants to merge 2 commits into from
Closed

Gracefully handle output symlinks with BwoB #18075

wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 13, 2023

If an action creates output symlinks, --remote_download_minimal now falls back to downloading all its outputs rather than failing with an exception, which most of the time led to a broken build.

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 13, 2023

@meteorcloudy I assume that this would fix a breakage in rules_go caused by bazelbuild/continuous-integration#1579: https://buildkite.com/bazel/rules-go-golang/builds/5049#018777f3-a4a6-4396-aecc-a7abe4de7ae8/256-294

@coeuvre Could you review this?

@fmeum fmeum marked this pull request as ready for review April 13, 2023 09:37
@fmeum fmeum requested a review from a team as a code owner April 13, 2023 09:37
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Apr 13, 2023
@coeuvre
Copy link
Member

coeuvre commented Apr 13, 2023

cc @tjgq

@fmeum fmeum force-pushed the download-minimal-symlinks branch from 8cc1a74 to eff963a Compare April 13, 2023 10:05
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 13, 2023

@bazel-io flag

@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 Apr 13, 2023
@fmeum fmeum force-pushed the download-minimal-symlinks branch from eff963a to 180927a Compare April 13, 2023 10:13
@fmeum
Gracefully handle output symlinks with BwoB
0c7c67a 
If an action creates output symlinks, `--remote_download_minimal` now
falls back to downloading all its outputs rather than failing with an
exception, which most of the time led to a broken build.
@fmeum fmeum force-pushed the download-minimal-symlinks branch from 180927a to 0c7c67a Compare April 13, 2023 10:20
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 13, 2023

@coeuvre I fixed the test on Windows (by skipping it), but test_symlink_outputs_not_allowed_with_minimial now fails because the warning is no longer printed. Is the warning something that should be preserved? I couldn't figure out how to access EventHandler from RemoteExecutionService to show a warning without throwing an exception.

@tjgq
Copy link
Contributor

tjgq commented Apr 13, 2023

@coeuvre I fixed the test on Windows (by skipping it), but test_symlink_outputs_not_allowed_with_minimial now fails because the warning is no longer printed. Is the warning something that should be preserved? I couldn't figure out how to access EventHandler from RemoteExecutionService to show a warning without throwing an exception.

Can you do something similar to RemoteExecutionService#reportUploadError? I think the warning is warranted because we're degrading performance in a non-obvious way, but if it's too annoying to plumb it through, I think it's also fine to just delete the test.

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 13, 2023

@tjgq I added a warning and updated the test.

@coeuvre coeuvre 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 Apr 13, 2023
@keertk
Copy link
Member

keertk commented Apr 13, 2023

@bazel-io fork 6.2.0

@bazel-io bazel-io mentioned this pull request Apr 13, 2023
Closed
@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 Apr 13, 2023
@Pavank1992 Pavank1992 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 14, 2023
@fmeum fmeum deleted the download-minimal-symlinks branch April 14, 2023 12:29
@keertk keertk mentioned this pull request Apr 14, 2023
Merged
keertk added a commit that referenced this pull request Apr 17, 2023
@keertk @fmeum
Gracefully handle output symlinks with BwoB (#18106)
3687b56 
If an action creates output symlinks, `--remote_download_minimal` now falls back to downloading all its outputs rather than failing with an exception, which most of the time led to a broken build.

Closes #18075.

PiperOrigin-RevId: 524264497
Change-Id: Id0dc77baf1f2c7c54553cf4d7f2d52ce889d1b8b

Co-authored-by: Fabian Meumertzheim <[email protected]>
@fmeum fmeum mentioned this pull request Apr 25, 2023
Closed
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@fmeum @fweikert
Gracefully handle output symlinks with BwoB
693756f 
If an action creates output symlinks, `--remote_download_minimal` now falls back to downloading all its outputs rather than failing with an exception, which most of the time led to a broken build.

Closes bazelbuild#18075.

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

@coeuvre coeuvre coeuvre approved these changes

Assignees
No one assigned
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fmeum @coeuvre @tjgq @keertk @bazel-io @Pavank1992