diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index e274afcac6e410..bcdb553a1d4016 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -116,6 +116,7 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; import com.google.protobuf.ByteString; import com.google.protobuf.ExtensionRegistry; import com.google.protobuf.Message; @@ -910,6 +911,60 @@ ActionResultMetadata parseActionResultMetadata(RemoteAction action, RemoteAction files.buildOrThrow(), symlinks.buildOrThrow(), directories.buildOrThrow()); } + /** Check that all mandatory outputs are created. */ + private void checkOutputs(RemoteAction action, ActionResultMetadata metadata) throws IOException { + for (ActionInput output : action.getSpawn().getOutputFiles()) { + if (!action.getSpawn().isMandatoryOutput(output)) { + continue; + } + + // Don't check output that is tree artifact since spawn could generate nothing under that + // directory. Remote server typically doesn't create directory ahead of time resulting in + // empty tree artifact missing from action cache entry. + if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { + continue; + } + + Path localPath = execRoot.getRelative(output.getExecPath()); + if (metadata.files.containsKey(localPath) + || metadata.symlinks.containsKey(localPath) + || metadata.directories.containsKey(localPath)) { + continue; + } + + // Handle the case where output file is under an output directory and is omitted from + // ActionResult's output_files filed. Technically, this is an server side error, but it won't + // hurt if we detect this and continue the build. + boolean inDirectory = false; + for (Map.Entry entry : metadata.directories.entrySet()) { + Path path = entry.getKey(); + DirectoryMetadata directory = entry.getValue(); + if (!localPath.getPathString().startsWith(path.getPathString())) { + continue; + } + for (FileMetadata file : directory.files()) { + if (file.path.equals(localPath)) { + inDirectory = true; + break; + } + } + if (inDirectory) { + break; + } + } + if (inDirectory) { + continue; + } + + throw new IOException( + "Invalid action cache entry " + + action.getActionKey().getDigest().getHash() + + ": expected output " + + prettyPrint(output) + + " does not exist."); + } + } + /** * Download the output files and directory trees of a remotely executed action, as well stdin / * stdout to the local machine. @@ -935,29 +990,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } if (result.success()) { - // Check that all mandatory outputs are created. - for (ActionInput output : action.getSpawn().getOutputFiles()) { - if (action.getSpawn().isMandatoryOutput(output)) { - // Don't check output that is tree artifact since spawn could generate nothing under that - // directory. Remote server typically doesn't create directory ahead of time resulting in - // empty tree artifact missing from action cache entry. - if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { - continue; - } - - Path localPath = execRoot.getRelative(output.getExecPath()); - if (!metadata.files.containsKey(localPath) - && !metadata.directories.containsKey(localPath) - && !metadata.symlinks.containsKey(localPath)) { - throw new IOException( - "Invalid action cache entry " - + action.getActionKey().getDigest().getHash() - + ": expected output " - + prettyPrint(output) - + " does not exist."); - } - } - } + checkOutputs(action, metadata); // When downloading outputs from just remotely executed action, the action result comes from // Execution response which means, if disk cache is enabled, action result hasn't been @@ -984,6 +1017,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload)); if (downloadOutputs) { + // Deduplicate file downloads since output files could under an output directory. HashSet queuedFilePaths = new HashSet<>(); for (FileMetadata file : metadata.files()) { @@ -1111,7 +1145,8 @@ private Single buildUploadManifestAsync( // Check that all mandatory outputs are created. for (ActionInput outputFile : action.getSpawn().getOutputFiles()) { Path localPath = execRoot.getRelative(outputFile.getExecPath()); - if (action.getSpawn().isMandatoryOutput(outputFile) && !localPath.exists()) { + if (action.getSpawn().isMandatoryOutput(outputFile) + && !localPath.exists(Symlinks.NOFOLLOW)) { throw new IOException( "Expected output " + prettyPrint(outputFile) + " was not created locally."); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 925cc451603e70..ce52ae23e3e873 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -317,7 +317,6 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception { // Test that downloading a nested output directory works. // arrange - Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents"); Digest quxDigest = cache.addContents(remoteActionExecutionContext, "qux-contents"); Directory wobbleDirMessage = Directory.newBuilder() @@ -341,7 +340,7 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception { Digest barTreeDigest = cache.addContents(remoteActionExecutionContext, barTreeMessage.toByteArray()); ActionResult.Builder builder = ActionResult.newBuilder(); - builder.addOutputFilesBuilder().setPath("outputs/a/foo").setDigest(fooDigest); + builder.addOutputFilesBuilder().setPath("outputs/a/bar/wobble/qux").setDigest(quxDigest); builder.addOutputDirectoriesBuilder().setPath("outputs/a/bar").setTreeDigest(barTreeDigest); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); @@ -354,7 +353,62 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception { service.downloadOutputs(action, result); // assert - assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/foo"))).isEqualTo(fooDigest); + assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/bar/wobble/qux"))) + .isEqualTo(quxDigest); + assertThat(execRoot.getRelative("outputs/a/bar/wobble/qux").isExecutable()).isFalse(); + assertThat(context.isLockOutputFilesCalled()).isTrue(); + } + + @Test + public void downloadOutputs_nestedOutputDirectories_outputFileOmitted_works() throws Exception { + // Test that downloading a nested output directory works when the output file under an output + // directory is omitted in ActionResult. + + // arrange + Digest quxDigest = cache.addContents(remoteActionExecutionContext, "qux-contents"); + Directory wobbleDirMessage = + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("qux").setDigest(quxDigest)) + .build(); + Digest wobbleDirDigest = + cache.addContents(remoteActionExecutionContext, wobbleDirMessage.toByteArray()); + Tree barTreeMessage = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setName("qux") + .setDigest(quxDigest) + .setIsExecutable(true)) + .addDirectories( + DirectoryNode.newBuilder().setName("wobble").setDigest(wobbleDirDigest))) + .addChildren(wobbleDirMessage) + .build(); + Digest barTreeDigest = + cache.addContents(remoteActionExecutionContext, barTreeMessage.toByteArray()); + ActionResult.Builder builder = ActionResult.newBuilder(); + builder.addOutputDirectoriesBuilder().setPath("outputs/a/bar").setTreeDigest(barTreeDigest); + RemoteActionResult result = + RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); + Path barTreePath = remotePathResolver.outputPathToLocalPath("outputs/a/bar"); + Artifact barTreeOutput = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, barTreePath.relativeTo(execRoot)); + Path quxPath = remotePathResolver.outputPathToLocalPath("outputs/a/bar/wobble/qux"); + Artifact quxFileOutput = ActionsTestUtil.createArtifact(artifactRoot, quxPath); + Spawn spawn = + newSpawn( + ImmutableMap.of(), + ImmutableSet.builder().add(quxFileOutput).add(barTreeOutput).build()); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + // act + service.downloadOutputs(action, result); + + // assert assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/bar/wobble/qux"))) .isEqualTo(quxDigest); assertThat(execRoot.getRelative("outputs/a/bar/wobble/qux").isExecutable()).isFalse(); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 4c64bc30e2af2a..a78114fba7d747 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3771,4 +3771,37 @@ EOF expect_log "5 processes: 2 disk cache hit, 3 internal" } +function test_output_file_under_output_directory() { + # Test the case that output file is under an output directory. + # See https://github.com/bazelbuild/bazel/issues/15328. + + mkdir -p a + cat > a/rules.bzl <<'EOF' +def _nested_outputs(ctx): + out_dir = ctx.actions.declare_directory(ctx.attr.name) + out_file = ctx.actions.declare_file(ctx.attr.name + "/hello.txt") + + ctx.actions.run_shell( + inputs = [], + outputs = [out_dir, out_file], + command = """ +echo 'Hello, world!' > $1/hello.txt +echo 'additional output' > $1/extra.txt +""", + arguments = [out_dir.path], + ) + return DefaultInfo(files = depset([out_dir, out_file])) + +nested_outputs = rule(implementation = _nested_outputs) +EOF + cat > a/BUILD <<'EOF' +load(":rules.bzl", "nested_outputs") +nested_outputs(name = "nested_outputs") +EOF + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + //a:nested_outputs >& $TEST_log || fail "Failed to build //a:nested_outputs" +} + run_suite "Remote execution and remote cache tests" diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index af325e3b3b1a8b..d0604dcd881ca4 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -273,18 +273,18 @@ private ActionResult execute( workingDirectory.createDirectoryAndParents(); List outputs = new ArrayList<>(command.getOutputFilesList().size()); - for (String output : command.getOutputFilesList()) { + for (String output : command.getOutputDirectoriesList()) { Path file = workingDirectory.getRelative(output); if (file.exists()) { - throw new FileAlreadyExistsException("Output file already exists: " + file); + throw new FileAlreadyExistsException("Output directory/file already exists: " + file); } file.getParentDirectory().createDirectoryAndParents(); outputs.add(file); } - for (String output : command.getOutputDirectoriesList()) { + for (String output : command.getOutputFilesList()) { Path file = workingDirectory.getRelative(output); if (file.exists()) { - throw new FileAlreadyExistsException("Output directory/file already exists: " + file); + throw new FileAlreadyExistsException("Output file already exists: " + file); } file.getParentDirectory().createDirectoryAndParents(); outputs.add(file);