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

Inject metadata when creating a filesystem symlink for a non-symlink artifact. #16283

Closed
wants to merge 1 commit into from

Conversation

tjgq
Copy link
Contributor

@tjgq tjgq commented Sep 15, 2022

A ctx.actions.symlink whose output is a declare_file or declare_directory
(as opposed to a declare_symlink) has "copy" semantics, i.e., the output
artifact is indistinguishable from the referent except for its name; the
symlink is just a filesystem-level optimization to avoid an actual copy,
and is transparently resolved when collecting the action output metadata.

When the symlink points to an artifact that was built remotely and without the
bytes, we currently must download it before executing the symlink action, so
that the output metadata can be constructed from the local filesystem. This
change short-circuits the filesystem traversal by injecting output metadata,
which is identical to the input plus a pointer to the original path. This is
used by the prefetcher to avoid downloading the same files multiple times when
they're symlinked more than once.

(An alternative would have been to teach all of the RemoteActionFileSystem
methods to resolve symlinks by patching together the local and remote metadata,
but that would have resulted in an awful lot of complexity.)

Fixes #15678.

@tjgq tjgq force-pushed the symlink-to-dir branch 2 times, most recently from 06c9399 to 9bdf1c1 Compare September 15, 2022 13:18
@tjgq tjgq marked this pull request as ready for review September 15, 2022 13:20
@tjgq tjgq requested a review from a team as a code owner September 15, 2022 13:20
@ShreeM01 ShreeM01 added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-user-response Awaiting a response from the author labels Sep 16, 2022
@tjgq tjgq force-pushed the symlink-to-dir branch 4 times, most recently from 2bcd265 to 1396247 Compare September 21, 2022 08:46
@tjgq tjgq changed the title Inject metadata when calling RemoteActionFileSystem#createSymbolicLink for a non-symlink artifact. Inject metadata when creating a filesystem symlink for a non-symlink artifact. Sep 22, 2022
@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Sep 26, 2022
@tjgq tjgq force-pushed the symlink-to-dir branch 5 times, most recently from de0baf8 to 7f88ca5 Compare September 28, 2022 23:27
copybara-service bot pushed a commit that referenced this pull request Sep 29, 2022
This makes it easier to later return different specializations depending on which fields are present, which will be required by #16283.

PiperOrigin-RevId: 477674504
Change-Id: Ieec385e923ab4ce0546f8853811df18316a8d884
@tjgq tjgq force-pushed the symlink-to-dir branch 2 times, most recently from a4d53a7 to c7bb56e Compare September 29, 2022 10:10
@tjgq tjgq force-pushed the symlink-to-dir branch 3 times, most recently from 4d55045 to 9f84f7e Compare September 29, 2022 19:22
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
This makes it easier to later return different specializations depending on which fields are present, which will be required by bazelbuild#16283.

PiperOrigin-RevId: 477674504
Change-Id: Ieec385e923ab4ce0546f8853811df18316a8d884
copybara-service bot pushed a commit that referenced this pull request Oct 18, 2022
In #16283, I will add a new data member to this class, which won't be shared with other uses of FileArtifactValue#createProxy. This is a preliminary no-op CL to make the subsequent change simpler.

As noted in the PR discussion, the getType() method on the proxy object currently returns REGULAR_FILE instead of DIRECTORY. I intend to fix this in a separate CL since it's unclear whether any callers rely on this.

PiperOrigin-RevId: 481913076
Change-Id: I44f721f10230ec782f7a583cdf372972376959c0
copybara-service bot pushed a commit that referenced this pull request Oct 18, 2022
This will be needed by #16283.

PiperOrigin-RevId: 481921235
Change-Id: I619bdbee25b7465ce2e6ad3e384e1ce414d2e770
@tjgq tjgq force-pushed the symlink-to-dir branch 2 times, most recently from f09c1dd to ba654cc Compare October 18, 2022 15:40
@tjgq tjgq force-pushed the symlink-to-dir branch 4 times, most recently from e2217af to 7e31a48 Compare October 20, 2022 08:35
…artifact.

A ctx.actions.symlink whose output is a declare_file or declare_directory
(as opposed to a declare_symlink) has "copy" semantics, i.e., the output
artifact is indistinguishable from the referent except for its name; the
symlink is just a filesystem-level optimization to avoid an actual copy,
and is transparently resolved when collecting the action output metadata.

When the symlink points to an artifact that was built remotely and without the
bytes, we currently must download it before executing the symlink action, so
that the output metadata can be constructed from the local filesystem. This
change short-circuits the filesystem traversal by injecting output metadata,
which is identical to the input plus a pointer to the original path. This is
used by the prefetcher to avoid downloading the same files multiple times when
they're symlinked more than once.

(An alternative would have been to teach all of the RemoteActionFileSystem
methods to resolve symlinks by patching together the local and remote metadata,
but that would have resulted in an awful lot of complexity.)

Fixes bazelbuild#15678.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Action input symlink into directory doesn't work with --remote_download_minimal
5 participants