Conditionally set output_paths based on Remote Executor capabilities

This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.

src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java

@@ -17,9 +17,7 @@
 import build.bazel.remote.execution.v2.ServerCapabilities;
 import build.bazel.semver.SemVer;
 
-/**
- * Represents a version of the Remote Execution API.
- */
+/** Represents a version of the Remote Execution API. */
 public class ApiVersion implements Comparable<ApiVersion> {
   public final int major;
   public final int minor;
@@ -28,7 +26,12 @@ public class ApiVersion implements Comparable<ApiVersion> {
 
   // The current version of the Remote Execution API. This field will need to be updated
   // together with all version changes.
-  public static final ApiVersion current = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
+  public static final ApiVersion current =
+      new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());
+  // The version of the Remote Execution API that starts supporting the
+  // Command.output_paths and ActionResult.output_symlinks fields.
+  public static final ApiVersion twoPointOne =
+      new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());
 
   public ApiVersion(int major, int minor, int patch, String prerelease) {
     this.major = major;
@@ -100,6 +103,7 @@ private enum State {
       UNSUPPORTED,
       DEPRECATED,
     }
+
     private final String message;
     private final State state;
 

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -221,30 +221,37 @@ public RemoteExecutionService(
   }
 
   static Command buildCommand(
+      boolean useOutputPaths,
       Collection<? extends ActionInput> outputs,
       List<String> arguments,
       ImmutableMap<String, String> env,
       @Nullable Platform platform,
       RemotePathResolver remotePathResolver) {
     Command.Builder command = Command.newBuilder();
-    ArrayList<String> outputFiles = new ArrayList<>();
-    ArrayList<String> outputDirectories = new ArrayList<>();
-    ArrayList<String> outputPaths = new ArrayList<>();
-    for (ActionInput output : outputs) {
-      String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
-      if (output.isDirectory()) {
-        outputDirectories.add(pathString);
-      } else {
-        outputFiles.add(pathString);
+    if (useOutputPaths) {
+      var outputPaths = new ArrayList<String>();
+      for (ActionInput output : outputs) {
+        String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
+        outputPaths.add(pathString);
       }
-      outputPaths.add(pathString);
+      Collections.sort(outputPaths);
+      command.addAllOutputPaths(outputPaths);
+    } else {
+      var outputFiles = new ArrayList<String>();
+      var outputDirectories = new ArrayList<String>();
+      for (ActionInput output : outputs) {
+        String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
+        if (output.isDirectory()) {
+          outputDirectories.add(pathString);
+        } else {
+          outputFiles.add(pathString);
+        }
+      }
+      Collections.sort(outputFiles);
+      Collections.sort(outputDirectories);
+      command.addAllOutputFiles(outputFiles);
+      command.addAllOutputDirectories(outputDirectories);
     }
-    Collections.sort(outputFiles);
-    Collections.sort(outputDirectories);
-    Collections.sort(outputPaths);
-    command.addAllOutputFiles(outputFiles);
-    command.addAllOutputDirectories(outputDirectories);
-    command.addAllOutputPaths(outputPaths);
 
     if (platform != null) {
       command.setPlatform(platform);
@@ -542,8 +549,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
         platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
       }
 
+      var useOutputPaths = true;
+      if (mayBeExecutedRemotely(spawn)) {
+        var capabilities = remoteExecutor.getServerCapabilities();
+        if (capabilities != null) {
+          var highApiVersion = new ApiVersion(capabilities.getHighApiVersion());
+          useOutputPaths = highApiVersion.compareTo(ApiVersion.twoPointOne) >= 0;
+        }
+      }
       Command command =
           buildCommand(
+              useOutputPaths,
               spawn.getOutputFiles(),
               spawn.getArguments(),
               spawn.getEnvironment(),

src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

@@ -39,6 +39,7 @@
 import build.bazel.remote.execution.v2.Digest;
 import build.bazel.remote.execution.v2.Directory;
 import build.bazel.remote.execution.v2.DirectoryNode;
+import build.bazel.remote.execution.v2.ExecutionCapabilities;
 import build.bazel.remote.execution.v2.FileNode;
 import build.bazel.remote.execution.v2.NodeProperties;
 import build.bazel.remote.execution.v2.NodeProperty;
@@ -47,9 +48,11 @@
 import build.bazel.remote.execution.v2.OutputSymlink;
 import build.bazel.remote.execution.v2.Platform;
 import build.bazel.remote.execution.v2.RequestMetadata;
+import build.bazel.remote.execution.v2.ServerCapabilities;
 import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy;
 import build.bazel.remote.execution.v2.SymlinkNode;
 import build.bazel.remote.execution.v2.Tree;
+import build.bazel.semver.SemVer;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableClassToInstanceMap;
 import com.google.common.collect.ImmutableList;
@@ -138,6 +141,20 @@ public class RemoteExecutionServiceTest {
   private final Reporter reporter = new Reporter(new EventBus());
   private final StoredEventHandler eventHandler = new StoredEventHandler();
 
+  private final ServerCapabilities removeExecutorCapabilities =
+      ServerCapabilities.newBuilder()
+          .setLowApiVersion(ApiVersion.current.toSemVer())
+          .setHighApiVersion(ApiVersion.current.toSemVer())
+          .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
+          .build();
+  private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
+  private final ServerCapabilities legacyRemoveExecutorCapabilities =
+      ServerCapabilities.newBuilder()
+          .setLowApiVersion(oldApiVersion.toSemVer())
+          .setHighApiVersion(oldApiVersion.toSemVer())
+          .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
+          .build();
+
   RemoteOptions remoteOptions;
   private Path execRoot;
   private ArtifactRoot artifactRoot;
@@ -177,6 +194,7 @@ public final void setUp() throws Exception {
 
     cache = spy(new InMemoryRemoteCache(spy(new InMemoryCacheClient()), remoteOptions, digestUtil));
     executor = mock(RemoteExecutionClient.class);
+    when(executor.getServerCapabilities()).thenReturn(removeExecutorCapabilities);
 
     RequestMetadata metadata =
         TracingMetadataUtils.buildMetadata("none", "none", "action-id", null);
@@ -195,9 +213,27 @@ public void buildRemoteAction_withRegularFileAsOutput() throws Exception {
 
     RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
 
-    assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString());
+    assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
     assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly(execPath.toString());
+  }
+
+  @Test
+  public void legacy_buildRemoteAction_withRegularFileAsOutput() throws Exception {
+    when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
+    PathFragment execPath = execRoot.getRelative("path/to/tree").asFragment();
+    Spawn spawn =
+        new SpawnBuilder("dummy")
+            .withOutput(ActionsTestUtil.createArtifactWithExecPath(artifactRoot, execPath))
+            .build();
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+
+    RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+    assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString());
     assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
   }
 
   @Test
@@ -213,8 +249,28 @@ public void buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
 
     RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
 
+    assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
+  }
+
+  @Test
+  public void legacy_buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
+    when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
+    Spawn spawn =
+        new SpawnBuilder("dummy")
+            .withOutput(
+                ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+                    artifactRoot, PathFragment.create("path/to/dir")))
+            .build();
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+
+    RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
     assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
     assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
+    assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
   }
 
   @Test
@@ -230,11 +286,30 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
 
     RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
 
-    assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
+    assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
     assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
     assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link");
   }
 
+  @Test
+  public void legacy_buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
+    when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
+    Spawn spawn =
+        new SpawnBuilder("dummy")
+            .withOutput(
+                ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath(
+                    artifactRoot, PathFragment.create("path/to/link")))
+            .build();
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+
+    RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+    assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
+    assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
+  }
+
   @Test
   public void buildRemoteAction_withActionInputAsOutput() throws Exception {
     Spawn spawn =
@@ -246,8 +321,42 @@ public void buildRemoteAction_withActionInputAsOutput() throws Exception {
 
     RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
 
+    assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/file");
+  }
+
+  @Test
+  public void legacy_buildRemoteAction_withActionInputFileAsOutput() throws Exception {
+    when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
+    Spawn spawn =
+        new SpawnBuilder("dummy")
+            .withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/file")))
+            .build();
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+
+    RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
     assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/file");
     assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
+  }
+
+  @Test
+  public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exception {
+    Spawn spawn =
+        new SpawnBuilder("dummy")
+            .withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/dir")))
+            .build();
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+
+    RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+    assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java

@@ -302,6 +302,8 @@ public int maxConcurrency() {
             });
     ServerCapabilities caps =
         ServerCapabilities.newBuilder()
+            .setLowApiVersion(ApiVersion.current.toSemVer())
+            .setHighApiVersion(ApiVersion.current.toSemVer())
             .setExecutionCapabilities(
                 ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
             .build();
@@ -353,7 +355,6 @@ public int maxConcurrency() {
                     .setName("VARIABLE")
                     .setValue("value")
                     .build())
-            .addAllOutputFiles(ImmutableList.of("bar", "foo"))
             .addAllOutputPaths(ImmutableList.of("bar", "foo"))
             .build();
     cmdDigest = DIGEST_UTIL.compute(command);

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

@@ -272,27 +272,21 @@ private ActionResult execute(
     workingDirectory.createDirectoryAndParents();
 
     List<Path> outputs =
-        new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount());
+        new ArrayList<>(command.getOutputPathsCount());
 
-    for (String output : command.getOutputFilesList()) {
+    for (String output : command.getOutputPathsList()) {
       Path file = workingDirectory.getRelative(output);
-      if (file.exists()) {
+      // Since https://github.com/bazelbuild/bazel/pull/15818,
+      // Bazel includes all expected output directories as part of Action's inputs.
+      //
+      // Ensure no output file exists before execution happen.
+      // Ignore if output directories pre-exist.
+      if (file.exists() && !file.isDirectory()) {
         throw new FileAlreadyExistsException("Output file already exists: " + file);
       }
       file.getParentDirectory().createDirectoryAndParents();
       outputs.add(file);
     }
-    for (String output : command.getOutputDirectoriesList()) {
-      Path file = workingDirectory.getRelative(output);
-      if (file.exists()) {
-        if (!file.isDirectory()) {
-          throw new FileAlreadyExistsException(
-              "Non-directory exists at output directory path: " + file);
-        }
-      }
-      file.getParentDirectory().createDirectoryAndParents();
-      outputs.add(file);
-    }
 
     // TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that
     // implementation instead of copying it.
@@ -419,8 +413,8 @@ private static long getUid() throws InterruptedException {
     com.google.devtools.build.lib.shell.Command cmd =
         new com.google.devtools.build.lib.shell.Command(
             new String[] {"id", "-u"},
-            /*environmentVariables=*/ null,
-            /*workingDirectory=*/ null,
+            /* environmentVariables= */ null,
+            /* workingDirectory= */ null,
             uidTimeout);
     try {
       ByteArrayOutputStream stdout = new ByteArrayOutputStream();