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

Remote executor used in tests does not support dangling symlinks #16289

Closed
tjgq opened this issue Sep 16, 2022 · 6 comments
Closed

Remote executor used in tests does not support dangling symlinks #16289

tjgq opened this issue Sep 16, 2022 · 6 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team
Milestone
6.0.0 release blo...

Comments

@tjgq
Copy link
Contributor

tjgq commented Sep 16, 2022

When attempting to remotely execute an action that produces a dangling symlink, the remote executor throws an exception here: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java;l=194;drc=93029d8f1f01be440d8e06cce9585ad912f9169b

This must be fixed before integration tests for dangling symlinks can be added. This may be a blocker for #10298.
@tjgq tjgq mentioned this issue Sep 16, 2022
Closed
tjgq added a commit to tjgq/bazel that referenced this issue Sep 16, 2022
@tjgq
Add tests for unresolved symlinks + remote execution.
3313be9 
The tests don't currently pass due to bazelbuild#16290 and bazelbuild#16289. Making them pass might
be considered a blocker for bazelbuild#10298 (declaring unresolved symlinks stable).
@tjgq tjgq mentioned this issue Sep 16, 2022
Closed
@lberki
Copy link
Contributor

lberki commented Sep 16, 2022

I think the more important point is whether RBE itself supports unresolved symlinks; to my best understanding, yes. I would be okay with releasing Bazel 6.0 without that bit of test infrastructure, if necessary, but it would be better not to do so.

So let's make this, P2 and a release blocker, with maybe revisiting the latter it if we are in a time crunch?

@lberki lberki added release blocker team-Remote-Exec Issues and PRs for the Execution (Remote) team P2 We'll consider working on this in future. (Assignee optional) labels Sep 16, 2022
@tjgq
Copy link
Contributor Author

tjgq commented Sep 16, 2022

The remote execution spec definitely supports actions that create a symlink, but it doesn't say whether the symlink must point to an existing file or directory or not (at least according to my reading). The relevant protocol fields are output_file_symlinks, output_directory_symlinks and output_symlinks. Note that the former two are deprecated in favor of the latter, but Bazel follows the older spec.

The Google Cloud RBE implementation, as far as I can tell, silently fails to report a dangling output symlink. I don't know about other implementations. A minimal repro is at https://github.com/tjgq/bazel-rbe-unresolved-symlink (but note that, even if the RBE implementation worked, the build would still fail on the Bazel side due to #16290).

@meisterT
Copy link
Member

cc @bazelbuild/remote-execution

@lberki
Copy link
Contributor

lberki commented Sep 16, 2022

Oof. For some reason, I thought that the Google's version of remote execution does handle both input and output symlinks. Then there is no alternative than to implement it in our mock backend if we want to test the functionality and we should definitely do so.

@tjgq
Copy link
Contributor Author

tjgq commented Sep 20, 2022

I've confirmed with folks who work on Google RBE that output symlinks are not currently implemented (specifically, symlinks that resolve successfully are reported as the regular files or directories that they resolve to, and dangling symlinks are omitted from the outputs entirely). So we do need to test against the mock backend.

@meteorcloudy meteorcloudy added this to the 6.0.0 release blockers milestone Sep 27, 2022
tjgq added a commit to tjgq/bazel that referenced this issue Sep 29, 2022
@tjgq
Upload dangling symlinks found in the outputs of local actions.
89e795a 
Since we don't know whether a dangling symlink is supposed to point to a file
or a directory, we always report it as a symlink to a file. The distinction
only matters in V2 of the protocol, with V2.1 reporting them uniformly, so it
doesn't seem to be worth the effort to match up the input and output types.

Dangling symlinks to absolute paths are unconditionally uploaded. A followup
PR will gate their upload on the presence of the respective server capability
(SymlinkAbsolutePathStrategy.ALLOW).

Progress towards bazelbuild#16289.
@tjgq tjgq mentioned this issue Sep 29, 2022
Closed
copybara-service bot pushed a commit that referenced this issue Sep 29, 2022
@tjgq @copybara-github
Upload dangling symlinks found in the outputs of local actions.
5b46a48 
Since we don't know whether a dangling symlink is supposed to point to a file or a directory, we always report it as a symlink to a file. The distinction only matters in V2 of the protocol, with V2.1 reporting them uniformly, so it doesn't seem to be worth the effort to match up the input and output types.

Dangling symlinks to absolute paths are unconditionally uploaded. A followup PR will gate their upload on the presence of the respective server capability (SymlinkAbsolutePathStrategy.ALLOW).

The change is behind a new --incompatible_remote_dangling_symlinks flag, which will be introduced in the flipped state. See #16353.

Progress towards #16289.

Closes #16352.

PiperOrigin-RevId: 477735370
Change-Id: I6210d152529e4603a49377a9e72e8e116ced6b85
@tjgq
Copy link
Contributor Author

tjgq commented Sep 29, 2022

This was fixed by 5b46a48.

@tjgq tjgq closed this as completed Sep 29, 2022
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
@tjgq @aiuto
Upload dangling symlinks found in the outputs of local actions.
cabda0f 
Since we don't know whether a dangling symlink is supposed to point to a file or a directory, we always report it as a symlink to a file. The distinction only matters in V2 of the protocol, with V2.1 reporting them uniformly, so it doesn't seem to be worth the effort to match up the input and output types.

Dangling symlinks to absolute paths are unconditionally uploaded. A followup PR will gate their upload on the presence of the respective server capability (SymlinkAbsolutePathStrategy.ALLOW).

The change is behind a new --incompatible_remote_dangling_symlinks flag, which will be introduced in the flipped state. See bazelbuild#16353.

Progress towards bazelbuild#16289.

Closes bazelbuild#16352.

PiperOrigin-RevId: 477735370
Change-Id: I6210d152529e4603a49377a9e72e8e116ced6b85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Milestone
6.0.0 release blockers
Development

No branches or pull requests
4 participants
@meisterT @lberki @meteorcloudy @tjgq