Exit if --experimental_remote_output_service is set, but not `--rem…

…ote_cache` nor `--remote_executor`
coeuvre · Apr 8, 2024 · 5e356f7 · 5e356f7
1 parent bf16de3
commit 5e356f7
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 12 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BazelOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/BazelOutputService.java
@@ -80,7 +80,9 @@ public class BazelOutputService implements OutputService {
   private final Supplier<Path> execRootSupplier;
   private final Supplier<Path> outputPathSupplier;
   private final DigestFunction.Value digestFunction;
-  private final RemoteOptions remoteOptions;
+  private final String remoteCache;
+  private final String remoteInstanceName;
+  private final String remoteOutputServiceOutputPathPrefix;
   private final boolean verboseFailures;
   private final RemoteRetrier retrier;
   private final ReferenceCountedChannel channel;
@@ -93,15 +95,19 @@ public BazelOutputService(
       Supplier<Path> execRootSupplier,
       Supplier<Path> outputPathSupplier,
       DigestFunction.Value digestFunction,
-      RemoteOptions remoteOptions,
+      String remoteCache,
+      String remoteInstanceName,
+      String remoteOutputServiceOutputPathPrefix,
       boolean verboseFailures,
       RemoteRetrier retrier,
       ReferenceCountedChannel channel) {
     this.outputBaseId = DigestUtil.hashCodeToString(md5().hashString(outputBase.toString(), UTF_8));
     this.execRootSupplier = execRootSupplier;
     this.outputPathSupplier = outputPathSupplier;
     this.digestFunction = digestFunction;
-    this.remoteOptions = remoteOptions;
+    this.remoteCache = remoteCache;
+    this.remoteInstanceName = remoteInstanceName;
+    this.remoteOutputServiceOutputPathPrefix = remoteOutputServiceOutputPathPrefix;
     this.verboseFailures = verboseFailures;
     this.retrier = retrier;
     this.channel = channel;
@@ -187,7 +193,7 @@ public ModifiedFileSet startBuild(
       throws AbruptExitException, InterruptedException {
     checkState(this.buildId == null, "this.buildId must be null");
     this.buildId = buildId.toString();
-    var outputPathPrefix = PathFragment.create(remoteOptions.remoteOutputServiceOutputPathPrefix);
+    var outputPathPrefix = PathFragment.create(remoteOutputServiceOutputPathPrefix);
     if (!outputPathPrefix.isEmpty() && !outputPathPrefix.isAbsolute()) {
       throw new AbruptExitException(
           DetailedExitCode.of(
@@ -212,8 +218,8 @@ public ModifiedFileSet startBuild(
             .setArgs(
                 Any.pack(
                     StartBuildArgs.newBuilder()
-                        .setRemoteCache(remoteOptions.remoteCache)
-                        .setInstanceName(remoteOptions.remoteInstanceName)
+                        .setRemoteCache(remoteCache)
+                        .setInstanceName(remoteInstanceName)
                         .setDigestFunction(digestFunction)
                         .build()))
             .setOutputPathPrefix(outputPathPrefix.toString())

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -301,13 +301,30 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     }
     boolean enableGrpcCache = GrpcCacheClient.isRemoteCacheOptions(remoteOptions);
     boolean enableRemoteDownloader = shouldEnableRemoteDownloader(remoteOptions);
+    boolean enableRemoteOutputService = !Strings.isNullOrEmpty(remoteOptions.remoteOutputService);
 
     if (enableRemoteDownloader && !enableGrpcCache) {
       throw createOptionsExitException(
           "The remote downloader can only be used in combination with gRPC caching",
           FailureDetails.RemoteOptions.Code.DOWNLOADER_WITHOUT_GRPC_CACHE);
     }
 
+    if (enableRemoteOutputService) {
+      if (enableDiskCache) {
+        env.getReporter()
+            .handle(
+                Event.warn(
+                    "--disk_cache is ignored when --experimental_remote_output_service is set."));
+      }
+
+      if (Strings.isNullOrEmpty(remoteOptions.remoteCache)) {
+        throw createOptionsExitException(
+            "--experimental_remote_output_service can only be used in combination with"
+                + " --remote_cache or --remote_executor.",
+            FailureDetails.RemoteOptions.Code.EXECUTION_WITH_INVALID_CACHE);
+      }
+    }
+
     if (!enableDiskCache && !enableHttpCache && !enableGrpcCache && !enableRemoteExecution) {
       // Quit if no remote caching or execution was enabled.
       actionContextProvider =
@@ -451,15 +468,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
               env::getExecRoot,
               () -> env.getDirectories().getOutputPath(env.getWorkspaceName()),
               digestUtil.getDigestFunction(),
-              remoteOptions,
+              remoteOptions.remoteCache,
+              remoteOptions.remoteInstanceName,
+              remoteOptions.remoteOutputServiceOutputPathPrefix,
               verboseFailures,
               retrier,
               bazelOutputServiceChannel);
-
-      throw createExitException(
-          "Remote Output Service is still WIP",
-          ExitCode.REMOTE_ERROR,
-          Code.REMOTE_EXECUTION_UNKNOWN);
     } else {
       outputService = new RemoteOutputService(env);
     }

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java
@@ -75,6 +75,7 @@
 import java.net.URI;
 import java.time.Duration;
 import java.util.ArrayList;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -85,6 +86,7 @@
 public final class RemoteModuleTest {
   private static final String EXECUTION_SERVER_NAME = "execution-server";
   private static final String CACHE_SERVER_NAME = "cache-server";
+  private static final String OUTPUT_SERVICE_SERVER_NAME = "output-service";
   private static final ServerCapabilities CACHE_ONLY_CAPS =
       ServerCapabilities.newBuilder()
           .setLowApiVersion(ApiVersion.low.toSemVer())
@@ -520,6 +522,21 @@ public void testVerifyCapabilities_cacheOnlyEndpointWithCircuitBreaker() throws
     }
   }
 
+  @Test
+  public void bazelOutputService_noRemoteCache_exit() throws Exception {
+    Server outputServiceService = createFakeServer(OUTPUT_SERVICE_SERVER_NAME);
+    try {
+    remoteOptions.remoteOutputService = OUTPUT_SERVICE_SERVER_NAME;
+
+    var exception = Assert.assertThrows(AbruptExitException.class, this::beforeCommand);
+
+    assertThat(exception).hasMessageThat().contains("--experimental_remote_output_service");
+    } finally{
+      outputServiceService.shutdownNow();
+      outputServiceService.awaitTermination();
+    }
+  }
+
   private void beforeCommand() throws IOException, AbruptExitException {
     CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions);
     remoteModule.beforeCommand(env);