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.
sluongng · Jul 24, 2024 · 11c09c1 · 11c09c1
1 parent afce68f
commit 11c09c1
Show file tree

Hide file tree

Showing 9 changed files with 281 additions and 61 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java b/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;

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
@@ -47,6 +47,7 @@
 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.SymlinkNode;
 import build.bazel.remote.execution.v2.Tree;
 import com.github.benmanes.caffeine.cache.Cache;
@@ -239,31 +240,38 @@ 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 = 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);
@@ -590,8 +598,23 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
         platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
       }
 
+      var readCachePolicy = getReadCachePolicy(spawn);
+      var writeCachePolicy = getWriteCachePolicy(spawn);
+      ServerCapabilities capabilities =
+          mayBeExecutedRemotely(spawn)
+              ? remoteExecutor.getServerCapabilities()
+              : writeCachePolicy.allowRemoteCache() ? remoteCache.getServerCapabilities : null;
+      var useOutputPaths = true;
+      if (capabilities != null) {
+        var supportStatus = ClientApiVersion.current.checkServerSupportedVersions(capabilities);
+        if (supportStatus.isSupported()) {
+          useOutputPaths =
+              supportStatus.getHighestSupportedVersion().compareTo(ApiVersion.twoPointOne) >= 0;
+        }
+      }
       Command command =
           buildCommand(
+              useOutputPaths,
               spawn.getOutputFiles(),
               spawn.getArguments(),
               spawn.getEnvironment(),
@@ -615,7 +638,7 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
               buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner());
       RemoteActionExecutionContext remoteActionExecutionContext =
           RemoteActionExecutionContext.create(
-              spawn, context, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn));
+              spawn, context, metadata, writeCachePolicy, readCachePolicy);
 
       return new RemoteAction(
           spawn,

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
@@ -38,6 +38,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;
@@ -46,9 +47,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;
@@ -153,6 +156,23 @@ public class RemoteExecutionServiceTest {
   private final Reporter reporter = new Reporter(new EventBus());
   private final StoredEventHandler eventHandler = new StoredEventHandler();
 
+  // In the past, Bazel only supports RemoteApi version 2.0.
+  // Use this to ensure we are backward compatible with Servers that only support 2.0.
+  private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
+  private final ServerCapabilities legacyRemoteExecutorCapabilities =
+      ServerCapabilities.newBuilder()
+          .setLowApiVersion(oldApiVersion.toSemVer())
+          .setHighApiVersion(oldApiVersion.toSemVer())
+          .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
+          .build();
+
+  private final ServerCapabilities remoteExecutorCapabilities =
+      ServerCapabilities.newBuilder()
+          .setLowApiVersion(ApiVersion.low.toSemVer())
+          .setHighApiVersion(ApiVersion.high.toSemVer())
+          .setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
+          .build();
+
   RemoteOptions remoteOptions;
   private FileSystem fs;
   private Path execRoot;
@@ -197,6 +217,7 @@ public final void setUp() throws Exception {
 
     cache = spy(new InMemoryRemoteCache(spy(new InMemoryCacheClient()), remoteOptions, digestUtil));
     executor = mock(RemoteExecutionClient.class);
+    when(executor.getServerCapabilities()).thenReturn(remoteExecutorCapabilities);
 
     RequestMetadata metadata =
         TracingMetadataUtils.buildMetadata("none", "none", "action-id", null);
@@ -215,9 +236,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(legacyRemoteExecutorCapabilities);
+    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
@@ -233,8 +272,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(legacyRemoteExecutorCapabilities);
+    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
@@ -250,11 +309,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(legacyRemoteExecutorCapabilities);
+    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 =
@@ -266,8 +344,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(legacyRemoteExecutorCapabilities);
+    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
@@ -2373,10 +2485,8 @@ public void buildRemoteActionWithPathMapping(@TestParameter boolean remoteMerkle
         .containsExactly(
             PathFragment.create("outputs/bin/input1"), mappedInput,
             PathFragment.create("outputs/bin/input2"), unmappedInput);
-    assertThat(remoteAction.getCommand().getOutputFilesList())
-        .containsExactly("outputs/bin/dir/output1", "outputs/bin/other_dir/output2");
-    assertThat(remoteAction.getCommand().getOutputDirectoriesList())
-        .containsExactly("outputs/bin/output_dir");
+    assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+    assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
     assertThat(remoteAction.getCommand().getOutputPathsList())
         .containsExactly(
             "outputs/bin/dir/output1", "outputs/bin/other_dir/output2", "outputs/bin/output_dir");

diff --git a/...ava/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/...ava/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
@@ -311,6 +311,8 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                         .build();
                 ServerCapabilities caps =
                     ServerCapabilities.newBuilder()
+                        .setLowApiVersion(ApiVersion.low.toSemVer())
+                        .setHighApiVersion(ApiVersion.high.toSemVer())
                         .setExecutionCapabilities(
                             ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
                         .build();
@@ -371,7 +373,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);

diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -178,6 +178,46 @@ function test_credential_helper_clear_cache() {
   expect_credential_helper_calls 10
 }
 
+function test_remote_grpc_cache_with_legacy_api() {
+  # Test if Bazel works with Remote Cache using Remote Api version 2.0.
+  stop_worker
+  start_worker --legacy_api
+
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "touch \$@",
+)
+EOF
+
+  bazel build \
+      --remote_cache=grpc://localhost:${worker_port} \
+      //a:foo \
+      || fail "Failed to build //a:foo with legacy api Remote Cache"
+}
+
+function test_remote_executor_with_legacy_api() {
+  # Test if Bazel works with Remote Executor using Remote Api version 2.0.
+  stop_worker
+  start_worker --legacy_api
+
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "touch \$@",
+)
+EOF
+
+  bazel build \
+      --remote_executor=grpc://localhost:${worker_port} \
+      //a:foo \
+      || fail "Failed to build //a:foo with legacy api Remote Executor"
+}
+
 function test_remote_grpc_cache_with_protocol() {
   # Test that if 'grpc' is provided as a scheme for --remote_cache flag, remote cache works.
   mkdir -p a