Conditionally set output_paths based on Remote Executor capabilities

This is a follow up to #18269, toward the discussion in #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.

Closes #18270.

PiperOrigin-RevId: 675060530
Change-Id: If08975b48696e06da0da91898ba2dd4b1c6677d2

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

@@ -23,12 +23,21 @@ public class ApiVersion implements Comparable<ApiVersion> {
   public final int patch;
   public final String prerelease;
 
+  // The version of the Remote Execution API that Bazel supports initially.
+  public static final ApiVersion twoPointZero =
+      new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).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());
+  // The latest version of the Remote Execution API that Bazel is compatible with.
+  public static final ApiVersion twoPointTwo =
+      new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(2).build());
+
   // The current lowest/highest versions (inclusive) of the Remote Execution API that Bazel
   // supports. These fields will need to be updated together with all version changes.
-  public static final ApiVersion low =
-      new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build());
-  public static final ApiVersion high =
-      new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build());
+  public static final ApiVersion low = twoPointZero;
+  public static final ApiVersion high = twoPointTwo;
 
   public ApiVersion(int major, int minor, int patch, String prerelease) {
     this.major = major;

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

@@ -29,6 +29,7 @@
 import build.bazel.remote.execution.v2.FindMissingBlobsResponse;
 import build.bazel.remote.execution.v2.GetActionResultRequest;
 import build.bazel.remote.execution.v2.RequestMetadata;
+import build.bazel.remote.execution.v2.ServerCapabilities;
 import build.bazel.remote.execution.v2.UpdateActionResultRequest;
 import com.google.bytestream.ByteStreamGrpc;
 import com.google.bytestream.ByteStreamGrpc.ByteStreamStub;
@@ -264,6 +265,11 @@ public CacheCapabilities getCacheCapabilities() throws IOException {
     return channel.getServerCapabilities().getCacheCapabilities();
   }
 
+  @Override
+  public ServerCapabilities getServerCapabilities() throws IOException {
+    return channel.getServerCapabilities();
+  }
+
   @Override
   public ListenableFuture<String> getAuthority() {
     return channel.withChannelFuture(ch -> Futures.immediateFuture(ch.authority()));

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

@@ -26,6 +26,7 @@
 import build.bazel.remote.execution.v2.ActionResult;
 import build.bazel.remote.execution.v2.CacheCapabilities;
 import build.bazel.remote.execution.v2.Digest;
+import build.bazel.remote.execution.v2.ServerCapabilities;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.flogger.GoogleLogger;
@@ -125,6 +126,13 @@ public ListenableFuture<String> getRemoteAuthority() {
     return remoteCacheClient.getAuthority();
   }
 
+  public ServerCapabilities getRemoteServerCapabilities() throws IOException {
+    if (remoteCacheClient == null) {
+      return ServerCapabilities.getDefaultInstance();
+    }
+    return remoteCacheClient.getServerCapabilities();
+  }
+
   /**
    * Class to keep track of which cache (disk or remote) a given [cached] ActionResult comes from.
    */

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

@@ -29,6 +29,7 @@
 import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer;
 import static com.google.devtools.build.lib.util.StringUtil.reencodeExternalToInternal;
 import static com.google.devtools.build.lib.util.StringUtil.reencodeInternalToExternal;
+import static java.util.Collections.min;
 
 import build.bazel.remote.execution.v2.Action;
 import build.bazel.remote.execution.v2.ActionResult;
@@ -156,6 +157,7 @@
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
 /**
@@ -196,6 +198,8 @@ public class RemoteExecutionService {
   @Nullable private final Scrubber scrubber;
   private final Set<Digest> knownMissingCasDigests;
 
+  private Boolean useOutputPaths;
+
   public RemoteExecutionService(
       Executor executor,
       Reporter reporter,
@@ -242,32 +246,39 @@ public RemoteExecutionService(
   }
 
   private Command buildCommand(
+      boolean useOutputPaths,
       Collection<? extends ActionInput> outputs,
       List<String> arguments,
       ImmutableMap<String, String> env,
       @Nullable Platform platform,
       RemotePathResolver remotePathResolver,
       @Nullable SpawnScrubber spawnScrubber) {
     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 =
-          reencodeInternalToExternal(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 =
+            reencodeInternalToExternal(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 =
+            reencodeInternalToExternal(remotePathResolver.localPathToOutputPath(output));
+        if (output.isDirectory()) {
+          outputDirectories.add(pathString);
+        } else {
+          outputFiles.add(pathString);
+        }
+      }
+      Collections.sort(outputFiles);
+      Collections.sort(outputDirectories);
+      command.addAllOutputFiles(outputFiles).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,6 +553,65 @@ private void maybeReleaseRemoteActionBuildingSemaphore() {
     remoteActionBuildingSemaphore.release();
   }
 
+  private boolean useOutputPaths() {
+    if (this.useOutputPaths == null) {
+      initUseOutputPaths();
+    }
+    return this.useOutputPaths;
+  }
+
+  private synchronized void initUseOutputPaths() {
+    // If this has already been initialized, return
+    if (this.useOutputPaths != null) {
+      return;
+    }
+    ApiVersion serverHighestVersion = null;
+    try {
+      // If both Remote Executor and Remote Cache are configured,
+      // use the highest version supported by both.
+
+      ClientApiVersion.ServerSupportedStatus executorSupportStatus = null;
+      if (remoteExecutor != null) {
+        var serverCapabilities = remoteExecutor.getServerCapabilities();
+        if (serverCapabilities != null) {
+          executorSupportStatus =
+              ClientApiVersion.current.checkServerSupportedVersions(serverCapabilities);
+        }
+      }
+
+      ClientApiVersion.ServerSupportedStatus cacheSupportStatus = null;
+      if (remoteCache != null) {
+        var serverCapabilities = remoteCache.getRemoteServerCapabilities();
+        if (serverCapabilities != null) {
+          cacheSupportStatus =
+              ClientApiVersion.current.checkServerSupportedVersions(serverCapabilities);
+        }
+      }
+
+      ApiVersion executorHighestVersion = null;
+      if (executorSupportStatus != null && executorSupportStatus.isSupported()) {
+        executorHighestVersion = executorSupportStatus.getHighestSupportedVersion();
+      }
+
+      ApiVersion cacheHighestVersion = null;
+      if (cacheSupportStatus != null && cacheSupportStatus.isSupported()) {
+        cacheHighestVersion = cacheSupportStatus.getHighestSupportedVersion();
+      }
+
+      if (executorHighestVersion != null && cacheHighestVersion != null) {
+        serverHighestVersion = min(ImmutableList.of(executorHighestVersion, cacheHighestVersion));
+      } else if (executorHighestVersion != null) {
+        serverHighestVersion = executorHighestVersion;
+      } else if (cacheHighestVersion != null) {
+        serverHighestVersion = cacheHighestVersion;
+      }
+    } catch (IOException e) {
+      // Intentionally ignored.
+    }
+    this.useOutputPaths =
+        serverHighestVersion == null || serverHighestVersion.compareTo(ApiVersion.twoPointOne) >= 0;
+  }
+
   /** Creates a new {@link RemoteAction} instance from spawn. */
   public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context)
       throws IOException, ExecException, ForbiddenActionInputException, InterruptedException {
@@ -570,6 +640,7 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
 
       Command command =
           buildCommand(
+              useOutputPaths(),
               spawn.getOutputFiles(),
               spawn.getArguments(),
               spawn.getEnvironment(),
@@ -1540,8 +1611,15 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr
     Map<Path, Path> realToTmpPath = new HashMap<>();
     ByteString inMemoryOutputContent = null;
     String inMemoryOutputPath = null;
+    var outputPathsList =
+        useOutputPaths()
+            ? action.getCommand().getOutputPathsList()
+            : Stream.concat(
+                    action.getCommand().getOutputFilesList().stream(),
+                    action.getCommand().getOutputDirectoriesList().stream())
+                .toList();
     try {
-      for (String output : action.getCommand().getOutputPathsList()) {
+      for (String output : outputPathsList) {
         String reencodedOutput = reencodeExternalToInternal(output);
         Path sourcePath =
             previousExecution.action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput);

src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java

@@ -18,6 +18,7 @@
 import build.bazel.remote.execution.v2.ActionResult;
 import build.bazel.remote.execution.v2.CacheCapabilities;
 import build.bazel.remote.execution.v2.Digest;
+import build.bazel.remote.execution.v2.ServerCapabilities;
 import com.google.common.base.Preconditions;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.vfs.Path;
@@ -34,6 +35,8 @@
 public interface RemoteCacheClient extends MissingDigestsFinder {
   CacheCapabilities getCacheCapabilities() throws IOException;
 
+  ServerCapabilities getServerCapabilities() throws IOException;
+
   ListenableFuture<String> getAuthority();
 
   /**

src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java

@@ -17,6 +17,7 @@
 import build.bazel.remote.execution.v2.ActionResult;
 import build.bazel.remote.execution.v2.CacheCapabilities;
 import build.bazel.remote.execution.v2.Digest;
+import build.bazel.remote.execution.v2.ServerCapabilities;
 import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy;
 import com.google.auth.Credentials;
 import com.google.common.collect.ImmutableList;
@@ -611,6 +612,11 @@ public CacheCapabilities getCacheCapabilities() {
         .build();
   }
 
+  @Override
+  public ServerCapabilities getServerCapabilities() {
+    return ServerCapabilities.newBuilder().setCacheCapabilities(getCacheCapabilities()).build();
+  }
+
   @Override
   public ListenableFuture<String> getAuthority() {
     return Futures.immediateFuture("");