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

Ensure runfiles output manifest is gone before making it a link. #19382

Closed
wants to merge 1 commit into from

Conversation

benjaminp
Copy link
Collaborator

After 0763dd0, the test included in this CL fails like this:

** test_switch_runfiles_from_enabled_to_disabled *******************************
Computing main repo mapping:
Loading:
Loading: 0 packages loaded
Analyzing: target //:out (1 packages loaded, 0 targets configured)
INFO: Analyzed target //:out (42 packages loaded, 172 targets configured).
INFO: Found 1 target...
[0 / 2] [Prepa] BazelWorkspaceStatusAction stable-status.txt
Target //:out up-to-date:
  bazel-bin/out
INFO: Elapsed time: 0.391s, Critical Path: 0.02s
INFO: 5 processes: 4 internal, 1 local.
INFO: Build completed successfully, 5 total actions
Computing main repo mapping:
Loading:
Loading: 0 packages loaded
WARNING: Build option --enable_runfiles has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
Analyzing: target //:out (1 packages loaded, 0 targets configured)
INFO: Analyzed target //:out (1 packages loaded, 173 targets configured).
INFO: Found 1 target...
[0 / 3] [Prepa] BazelWorkspaceStatusAction stable-status.txt
ERROR: workspace/BUILD:6:8: Executing genrule //:g failed: java.io.IOException: execroot/main/bazel-out/k8-opt-exec-ST-e846b08c7501/bin/cmd.runfiles/MANIFEST (File exists)
Target //:out failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.177s, Critical Path: 0.01s
INFO: 4 processes: 4 internal.
ERROR: Build did NOT complete successfully

This is because FileSystemUtils.copyFile ensured that the target was removed before writing to the target while Path.createSymbolicLink does not. Insert a delete() call to fix the problem.

Also, rename copyManifest to linkManifest for accuracy.

…lic link.

After bazelbuild@0763dd0, the test included in this CL fails like this:
```
** test_switch_runfiles_from_enabled_to_disabled *******************************
Computing main repo mapping:
Loading:
Loading: 0 packages loaded
Analyzing: target //:out (1 packages loaded, 0 targets configured)
INFO: Analyzed target //:out (42 packages loaded, 172 targets configured).
INFO: Found 1 target...
[0 / 2] [Prepa] BazelWorkspaceStatusAction stable-status.txt
Target //:out up-to-date:
  bazel-bin/out
INFO: Elapsed time: 0.391s, Critical Path: 0.02s
INFO: 5 processes: 4 internal, 1 local.
INFO: Build completed successfully, 5 total actions
Computing main repo mapping:
Loading:
Loading: 0 packages loaded
WARNING: Build option --enable_runfiles has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
Analyzing: target //:out (1 packages loaded, 0 targets configured)
INFO: Analyzed target //:out (1 packages loaded, 173 targets configured).
INFO: Found 1 target...
[0 / 3] [Prepa] BazelWorkspaceStatusAction stable-status.txt
ERROR: workspace/BUILD:6:8: Executing genrule //:g failed: java.io.IOException: execroot/main/bazel-out/k8-opt-exec-ST-e846b08c7501/bin/cmd.runfiles/MANIFEST (File exists)
Target //:out failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.177s, Critical Path: 0.01s
INFO: 4 processes: 4 internal.
ERROR: Build did NOT complete successfully
```
This is because `FileSystemUtils.copyFile` ensured that the target was removed before writing to the target while `Path.createSymbolicLink` does not. Insert a `delete()` call to fix the problem.

Also, rename `copyManifest` to `linkManifest` for accuracy.
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 31, 2023
@benjaminp benjaminp changed the title Ensure runfiles output manifest is gone before trying make it a link. Ensure runfiles output manifest is gone before trying to make it a link. Aug 31, 2023
@benjaminp benjaminp changed the title Ensure runfiles output manifest is gone before trying to make it a link. Ensure runfiles output manifest is gone before making it a link. Aug 31, 2023
Copy link
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

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

What a tricky bug! Thanks for the fix.

@tjgq tjgq 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 Aug 31, 2023
@iancha1992 iancha1992 added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Sep 1, 2023
@copybara-service copybara-service bot closed this in 695d747 Sep 4, 2023
@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 Sep 4, 2023
@benjaminp benjaminp deleted the link-deletion branch September 4, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants