Enable --experimental_remote_spawn_cache by default

- It is now an error to specify the gRPC remote execution backend in combination with a local disk or HTTP-based cache. - It is now an error to specify both local disk and HTTP-based caches. Note that before this CL, enabling the local disk cache silently disabled remote execution - we now give an error in that case. With these combination no longer being accepted, remote execution being enabled now means that we only create a RemoteSpawnRunner, and don't provide a SpawnCache. This is not a semantic change - we never created both. In principle, it should be possible for users to combine local execution with remote caching for actions that are marked local or no-remote, and still use remote execution otherwise. However, Bazel cannot currently express this combination of execution strategies. RELNOTES: The --experimental_remote_spawn_cache flag is now enabled by default, and remote caching no longer needs --*_strategy=remote flags (it will fail if they are specified). PiperOrigin-RevId: 198280398
bazelbuild · May 28, 2018 · cdf118a · cdf118a
1 parent 2ad044c
commit cdf118a
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 53 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -38,8 +38,8 @@
  */
 final class RemoteActionContextProvider extends ActionContextProvider {
   private final CommandEnvironment env;
-  private final AbstractRemoteActionCache cache;
-  private final GrpcRemoteExecutor executor;
+  @Nullable private final AbstractRemoteActionCache cache;
+  @Nullable private final GrpcRemoteExecutor executor;
   private final DigestUtil digestUtil;
   private final Path logDir;
 
@@ -64,7 +64,7 @@ public Iterable<? extends ActionContext> getActionContexts() {
     String buildRequestId = env.getBuildRequestId().toString();
     String commandId = env.getCommandId().toString();
 
-    if (remoteOptions.experimentalRemoteSpawnCache || remoteOptions.diskCache != null) {
+    if (executor == null && cache != null) {
       RemoteSpawnCache spawnCache =
           new RemoteSpawnCache(
               env.getExecRoot(),

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
@@ -97,7 +97,7 @@ public void serverInit(OptionsProvider startupOptions, ServerBuilder builder)
   }
 
   @Override
-  public void beforeCommand(CommandEnvironment env) {
+  public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     env.getEventBus().register(this);
     String buildRequestId = env.getBuildRequestId().toString();
     String commandId = env.getCommandId().toString();
@@ -113,6 +113,7 @@ public void beforeCommand(CommandEnvironment env) {
     } catch (IOException e) {
       env.getReporter()
           .handle(Event.error("Could not create base directory for remote logs: " + logDir));
+      throw new AbruptExitException(ExitCode.LOCAL_ENVIRONMENTAL_ERROR, e);
     }
     RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class);
     AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class);
@@ -126,10 +127,22 @@ public void beforeCommand(CommandEnvironment env) {
       return;
     }
 
-    try {
-      boolean remoteOrLocalCache = SimpleBlobStoreFactory.isRemoteCacheOptions(remoteOptions);
-      boolean grpcCache = GrpcRemoteCache.isRemoteCacheOptions(remoteOptions);
+    boolean enableRestCache = SimpleBlobStoreFactory.isRestUrlOptions(remoteOptions);
+    boolean enableDiskCache = SimpleBlobStoreFactory.isDiskCache(remoteOptions);
+    if (enableRestCache && enableDiskCache) {
+      throw new AbruptExitException(
+          "Cannot enable HTTP-based and local disk cache simultaneously",
+          ExitCode.COMMAND_LINE_ERROR);
+    }
+    boolean enableBlobStoreCache = enableRestCache || enableDiskCache;
+    boolean enableGrpcCache = GrpcRemoteCache.isRemoteCacheOptions(remoteOptions);
+    if (enableBlobStoreCache && remoteOptions.remoteExecutor != null) {
+      throw new AbruptExitException(
+          "Cannot combine gRPC based remote execution with local disk or HTTP-based caching",
+          ExitCode.COMMAND_LINE_ERROR);
+    }
 
+    try {
       LoggingInterceptor logger = null;
       if (!remoteOptions.experimentalRemoteGrpcLog.isEmpty()) {
         rpcLogFile = new AsynchronousFileOutputStream(remoteOptions.experimentalRemoteGrpcLog);
@@ -139,10 +152,8 @@ public void beforeCommand(CommandEnvironment env) {
       RemoteRetrier retrier =
           new RemoteRetrier(
               remoteOptions, RemoteRetrier.RETRIABLE_GRPC_ERRORS, Retrier.ALLOW_ALL_CALLS);
-      // TODO(davido): The naming is wrong here. "Remote"-prefix in RemoteActionCache class has no
-      // meaning.
       final AbstractRemoteActionCache cache;
-      if (remoteOrLocalCache) {
+      if (enableBlobStoreCache) {
         cache =
             new SimpleBlobStoreActionCache(
                 remoteOptions,
@@ -151,9 +162,9 @@ public void beforeCommand(CommandEnvironment env) {
                     GoogleAuthUtils.newCredentials(authAndTlsOptions),
                     env.getWorkingDirectory()),
                 digestUtil);
-      } else if (grpcCache || remoteOptions.remoteExecutor != null) {
+      } else if (enableGrpcCache || remoteOptions.remoteExecutor != null) {
         // If a remote executor but no remote cache is specified, assume both at the same target.
-        String target = grpcCache ? remoteOptions.remoteCache : remoteOptions.remoteExecutor;
+        String target = enableGrpcCache ? remoteOptions.remoteCache : remoteOptions.remoteExecutor;
         Channel ch = GoogleAuthUtils.newChannel(target, authAndTlsOptions);
         if (logger != null) {
           ch = ClientInterceptors.intercept(ch, logger);
@@ -169,6 +180,8 @@ public void beforeCommand(CommandEnvironment env) {
         cache = null;
       }
 
+      // TODO(davido): The naming is wrong here. "Remote"-prefix in RemoteActionCache class has no
+      // meaning.
       final GrpcRemoteExecutor executor;
       if (remoteOptions.remoteExecutor != null) {
         Channel ch = GoogleAuthUtils.newChannel(remoteOptions.remoteExecutor, authAndTlsOptions);
@@ -184,7 +197,6 @@ public void beforeCommand(CommandEnvironment env) {
       } else {
         executor = null;
       }
-
       actionContextProvider =
           new RemoteActionContextProvider(env, cache, executor, digestUtil, logDir);
     } catch (IOException e) {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java
@@ -164,11 +164,12 @@ public final class RemoteOptions extends OptionsBase {
   )
   public double experimentalRemoteRetryJitter;
 
+  @Deprecated
   @Option(
     name = "experimental_remote_spawn_cache",
     defaultValue = "false",
     documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-    effectTags = {OptionEffectTag.UNKNOWN},
+    effectTags = {OptionEffectTag.NO_OP},
     help =
         "Whether to use the experimental spawn cache infrastructure for remote caching. "
             + "Enabling this flag makes Bazel ignore any setting for remote_executor."

diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java
@@ -77,7 +77,7 @@ public static boolean isDiskCache(RemoteOptions options) {
     return options.diskCache != null;
   }
 
-  private static boolean isRestUrlOptions(RemoteOptions options) {
+  static boolean isRestUrlOptions(RemoteOptions options) {
     return options.remoteHttpCache != null;
   }
 }
diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh
@@ -78,7 +78,6 @@ EOF
 
   bazel clean --expunge >& $TEST_log
   bazel build \
-      --experimental_remote_spawn_cache=true  \
       --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote REST cache service"
@@ -112,7 +111,6 @@ EOF
 
   bazel clean --expunge >& $TEST_log
   bazel build \
-      --experimental_remote_spawn_cache=true \
       --remote_http_cache=http://bad.hostname/bad/cache \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote REST cache service"
@@ -136,15 +134,6 @@ genrule(
 )
 EOF
     bazel build \
-          --genrule_strategy=remote \
-          --noremote_allow_symlink_upload \
-          --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
-          //:make-link &> $TEST_log \
-          && fail "should have failed" || true
-    expect_log "/l is a symbolic link"
-
-    bazel build \
-          --experimental_remote_spawn_cache \
           --noremote_allow_symlink_upload \
           --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
           //:make-link &> $TEST_log \
@@ -161,15 +150,6 @@ genrule(
 )
 EOF
     bazel build \
-          --genrule_strategy=remote \
-          --noremote_allow_symlink_upload \
-          --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
-          //:make-link &> $TEST_log \
-          && fail "should have failed" || true
-    expect_log "dir/l is a symbolic link"
-
-    bazel build \
-          --experimental_remote_spawn_cache \
           --noremote_allow_symlink_upload \
           --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
           //:make-link &> $TEST_log \
@@ -224,7 +204,6 @@ function test_directory_artifact() {
   bazel build \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote execution"
   diff bazel-genfiles/a/qux/out.txt a/test_expected \
@@ -235,7 +214,6 @@ function test_directory_artifact_grpc_cache() {
   set_directory_artifact_testfixtures
 
   bazel build \
-      --spawn_strategy=remote \
       --remote_cache=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote gRPC cache"
@@ -247,7 +225,6 @@ function test_directory_artifact_rest_cache() {
   set_directory_artifact_testfixtures
 
   bazel build \
-      --spawn_strategy=remote \
       --remote_rest_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote REST cache"
@@ -320,7 +297,6 @@ function test_directory_artifact_skylark() {
   bazel build \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote execution"
   diff bazel-genfiles/a/qux/out.txt a/test_expected \
@@ -331,7 +307,6 @@ function test_directory_artifact_skylark_grpc_cache() {
   set_directory_artifact_skylark_testfixtures
 
   bazel build \
-      --spawn_strategy=remote \
       --remote_cache=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote gRPC cache"
@@ -343,7 +318,6 @@ function test_directory_artifact_skylark_rest_cache() {
   set_directory_artifact_skylark_testfixtures
 
   bazel build \
-      --spawn_strategy=remote \
       --remote_rest_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote REST cache"

diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -89,7 +89,6 @@ EOF
   bazel build \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote execution"
   diff bazel-bin/a/test ${TEST_TMPDIR}/test_expected \
@@ -119,7 +118,6 @@ EOF
   bazel test \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
       || fail "Failed to run //a:test with remote execution"
@@ -144,7 +142,6 @@ EOF
 
   bazel clean --expunge >& $TEST_log
   bazel build \
-      --spawn_strategy=remote \
       --remote_cache=localhost:${worker_port} \
       //a:test >& $TEST_log \
       || fail "Failed to build //a:test with remote gRPC cache service"
@@ -168,7 +165,6 @@ EOF
   bazel test \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
       && fail "Expected test failure" || true
@@ -194,7 +190,6 @@ EOF
 int main() { std::cout << "Fail me!" << std::endl; return 1; }
 EOF
   bazel test \
-      --spawn_strategy=remote \
       --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
@@ -219,7 +214,6 @@ EOF
 int main() { std::cout << "Fail me!" << std::endl; return 1; }
 EOF
   bazel test \
-      --experimental_remote_spawn_cache \
       --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
@@ -236,7 +230,6 @@ EOF
 int main() { std::cout << "Fail me again!" << std::endl; return 1; }
 EOF
   bazel test \
-      --experimental_remote_spawn_cache \
       --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
@@ -272,7 +265,6 @@ EOF
   bazel build \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       //a:large_output >& $TEST_log \
       || fail "Failed to build //a:large_output with remote execution"
   diff bazel-genfiles/a/large_blob.txt ${TEST_TMPDIR}/large_blob_expected.txt \
@@ -296,7 +288,6 @@ EOF
   bazel test \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
       || fail "Failed to run //a:test with remote execution"
@@ -331,7 +322,6 @@ EOF
   bazel test \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
       || fail "Failed to run //a:test with remote execution"
@@ -370,7 +360,6 @@ EOF
   bazel test \
       --spawn_strategy=remote \
       --remote_executor=localhost:${worker_port} \
-      --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \
       && fail "Expected test failure" || true
@@ -400,7 +389,6 @@ package(default_visibility = ["//visibility:public"])
 empty(name = 'test')
 EOF
   bazel build \
-      --spawn_strategy=remote \
       --remote_cache=localhost:${worker_port} \
       --test_output=errors \
       //a:test >& $TEST_log \