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

[6.5.0] Fix tree file materialized as symlink to another file when building without the bytes. #20409

Merged
merged 1 commit into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1100,7 +1100,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
ImmutableList.builder();

boolean downloadOutputs = shouldDownloadOutputsFor(result);
boolean downloadOutputs = shouldDownloadOutputsFor(result, metadata);

// Download into temporary paths, then move everything at the end.
// This avoids holding the output lock while downloading, which would prevent the local branch
Expand Down Expand Up @@ -1251,13 +1251,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
return null;
}

private boolean shouldDownloadOutputsFor(RemoteActionResult result) {
private boolean shouldDownloadOutputsFor(RemoteActionResult result,
ActionResultMetadata metadata) {
if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
return true;
}
// An output materialized as a symlink might point to one of the other outputs.
if (!result.getOutputSymlinks().isEmpty() || !result.getOutputFileSymlinks().isEmpty()
|| !result.getOutputDirectorySymlinks().isEmpty()) {
if (resultHasSymlinks(result, metadata)) {
return true;
}
// In case the action failed, download all outputs. It might be helpful for debugging and there
Expand All @@ -1268,6 +1268,18 @@ private boolean shouldDownloadOutputsFor(RemoteActionResult result) {
return false;
}

private boolean resultHasSymlinks(RemoteActionResult result, ActionResultMetadata metadata) {
if (!metadata.symlinks().isEmpty()) {
return true;
}
for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
if (!entry.getValue().symlinks().isEmpty()) {
return true;
}
}
return false;
}

private ImmutableList<ListenableFuture<FileMetadata>> buildFilesToDownload(
RemoteActionExecutionContext context,
ProgressStatusListener progressStatusListener,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -713,13 +713,39 @@ public void downloadTopLevel_genruleSymlinkToOutput() throws Exception {

// Delete target, re-download it
getOutputPath("foo").delete();

buildTarget("//:gen");

assertSymlink("foo-link", PathFragment.create("foo"));
assertValidOutputFile("foo-link", "hello\n");
}

@Test
public void downloadTopLevel_treeContainingSymlink() throws Exception {
// Disable test on Windows.
assumeFalse(OS.getCurrent() == OS.WINDOWS);

setDownloadToplevel();
write("tree.bzl", "def _impl(ctx):",
" d = ctx.actions.declare_directory(ctx.label.name)",
" ctx.actions.run_shell(",
" outputs = [d],",
" command = 'cd {} && echo hello > file.txt && ln -s file.txt sym.txt'.format(d.path),",
" )",
" return DefaultInfo(files = depset([d]))",
"tree = rule(_impl)");
write(
"BUILD",
"load('tree.bzl', 'tree')",
"tree(",
" name = 'tree',",
")");

buildTarget("//:tree");

assertSymlink("tree/sym.txt", PathFragment.create("file.txt"));
assertValidOutputFile("tree/sym.txt", "hello\n");
}

protected void assertSymlink(String binRelativeLinkPath, PathFragment targetPath)
throws Exception {
Path output = getOutputPath(binRelativeLinkPath);
Expand Down