Remote: Support the case where output files are under an output direc…

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<Path, DirectoryMetadata> 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<PathFragment> queuedFilePaths = new HashSet<>();
 
       for (FileMetadata file : metadata.files()) {
@@ -1111,7 +1145,8 @@ private Single<UploadManifest> 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.");
             }

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.<Artifact>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();

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"

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java

@@ -273,18 +273,18 @@ private ActionResult execute(
     workingDirectory.createDirectoryAndParents();
 
     List<Path> 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);