Skip to content

Commit

Permalink
Skip empty directories instead of throwing in prefetcher.
Browse files Browse the repository at this point in the history
While non-empty tree artifacts in the inputs to an action are expanded into the files they contain and omitted from the input mapping, empty tree artifacts are still present, as they signal the need to create the directory. Thus, the check added in 763f966 is incorrect.

I'm explicitly skipping the empty tree artifacts in prefetchFiles() as otherwise they get skipped as a result of FileArtifactValue#isRemote() returning false for the FileArtifactValue associated with an empty tree artifact (even if it was produced remotely!), which is extremely subtle.

Closes #17183.

PiperOrigin-RevId: 501207095
Change-Id: Ib52727d6fdc6b7a291a61fba33914e57531fb1f4
  • Loading branch information
tjgq authored and copybara-github committed Jan 11, 2023
1 parent 2ae409b commit 4c57def
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.remote;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable;
Expand Down Expand Up @@ -183,13 +182,16 @@ protected ListenableFuture<Void> prefetchFiles(
Map<SpecialArtifact, List<TreeFileArtifact>> trees = new HashMap<>();
List<ActionInput> files = new ArrayList<>();
for (ActionInput input : inputs) {
checkArgument(!input.isDirectory(), "cannot prefetch a directory");

// Source artifacts don't need to be fetched.
if (input instanceof Artifact && ((Artifact) input).isSourceArtifact()) {
continue;
}

// Skip empty tree artifacts (non-empty tree artifacts should have already been expanded).
if (input.isDirectory()) {
continue;
}

if (input instanceof TreeFileArtifact) {
TreeFileArtifact treeFile = (TreeFileArtifact) input;
SpecialArtifact treeArtifact = treeFile.getParent();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,31 @@ public void treeOutputsFromLocalFileSystem_works() throws Exception {
assertValidOutputFile("out/foobar.txt", "file-1\nbar\n");
}

@Test
public void emptyTreeConsumedByLocalAction() throws Exception {
// Disable remote execution so that the empty tree artifact is prefetched.
addOptions("--modify_execution_info=Genrule=+no-remote-exec");
setDownloadToplevel();
writeOutputDirRule();
write(
"BUILD",
"load(':output_dir.bzl', 'output_dir')",
"output_dir(",
" name = 'foo',",
" manifest = ':manifest',",
")",
"genrule(",
" name = 'foobar',",
" srcs = [':foo'],",
" outs = ['foobar.txt'],",
" cmd = 'touch $@',",
")");
write("manifest"); // no files

buildTarget("//:foobar");
waitDownloads();
}

@Test
public void multiplePackagePaths_buildsSuccessfully() throws Exception {
write(
Expand Down

0 comments on commit 4c57def

Please sign in to comment.