Revert "Move use of legacy sandbox -> local fallback to only be used after all strategies have been tried, and improve messages around it."

This reverts commit b2231c5.

Author: larsrc-google

src/main/java/com/google/devtools/build/lib/actions/SpawnStrategy.java

@@ -50,23 +50,9 @@ default SpawnContinuation beginExecution(
     }
   }
 
-  /**
-   * Returns whether this SpawnActionContext supports executing the given Spawn. This does not allow
-   * using the legacy fallback to local execution controlled by the {@code
-   * --incompatible_legacy_local_fallback} flag.
-   */
+  /** Returns whether this SpawnActionContext supports executing the given Spawn. */
   boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry);
 
-  /**
-   * Returns true if this SpawnActionContext supports executing the given Spawn through a legacy
-   * fallback system. This will only be used if no SpawnActionContexts were able to execute it by
-   * normal means.
-   */
-  default boolean canExecWithLegacyFallback(
-      Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) {
-    return false;
-  }
-
   /**
    * Performs any actions conditional on this strategy not only being registered but triggered as
    * used because its identifier was requested and it was not overridden.

src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java

@@ -433,8 +433,7 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
     for (SandboxedSpawnStrategy strategy :
         dynamicStrategyRegistry.getDynamicSpawnActionContexts(
             spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
-      if (strategy.canExec(spawn, actionContextRegistry)
-          || strategy.canExecWithLegacyFallback(spawn, actionContextRegistry)) {
+      if (strategy.canExec(spawn, actionContextRegistry)) {
         return true;
       }
     }
@@ -475,8 +474,7 @@ private static ImmutableList<SpawnResult> runLocally(
     for (SandboxedSpawnStrategy strategy :
         dynamicStrategyRegistry.getDynamicSpawnActionContexts(
             spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) {
-      if (strategy.canExec(spawn, actionExecutionContext)
-          || strategy.canExecWithLegacyFallback(spawn, actionExecutionContext)) {
+      if (strategy.canExec(spawn, actionExecutionContext)) {
         return strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns);
       }
     }

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -94,12 +94,6 @@ public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionCo
     return spawnRunner.canExec(spawn);
   }
 
-  @Override
-  public boolean canExecWithLegacyFallback(
-      Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) {
-    return spawnRunner.canExecWithLegacyFallback(spawn);
-  }
-
   @Override
   public ImmutableList<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {

src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java

@@ -255,11 +255,6 @@ SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
   /** Returns whether this SpawnRunner supports executing the given Spawn. */
   boolean canExec(Spawn spawn);
 
-  /** Returns whether this SpawnRunner supports executing the given Spawn using legacy fallbacks. */
-  default boolean canExecWithLegacyFallback(Spawn spawn) {
-    return false;
-  }
-
   /** Returns whether this SpawnRunner handles caching of actions internally. */
   boolean handlesCaching();
 

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java

@@ -88,37 +88,26 @@ public List<? extends SpawnStrategy> resolve(
             .getContext(SpawnStrategyRegistry.class)
             .getStrategies(spawn, actionExecutionContext.getEventHandler());
 
-    List<? extends SpawnStrategy> execableStrategies =
+    strategies =
         strategies.stream()
             .filter(spawnActionContext -> spawnActionContext.canExec(spawn, actionExecutionContext))
             .collect(Collectors.toList());
 
-    if (execableStrategies.isEmpty()) {
-      // Legacy implicit fallbacks should be a last-ditch option after all other strategies are
-      // found non-executable.
-      List<? extends SpawnStrategy> fallbackStrategies =
-          strategies.stream()
-              .filter(
-                  spawnActionContext ->
-                      spawnActionContext.canExecWithLegacyFallback(spawn, actionExecutionContext))
-              .collect(Collectors.toList());
-
-      if (fallbackStrategies.isEmpty()) {
-        String message =
-            String.format(
-                "No usable spawn strategy found for spawn with mnemonic %s.  Your --spawn_strategy,"
-                    + " --genrule_strategy and/or --strategy flags are probably too strict. Visit"
-                    + " https://github.com/bazelbuild/bazel/issues/7480 for migration advice",
-                spawn.getMnemonic());
-        throw new UserExecException(
-            FailureDetail.newBuilder()
-                .setMessage(message)
-                .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND))
-                .build());
-      }
-      return fallbackStrategies;
+    if (strategies.isEmpty()) {
+      String message =
+          String.format(
+              "No usable spawn strategy found for spawn with mnemonic %s.  Your"
+                  + " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably too"
+                  + " strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for"
+                  + " migration advice",
+              spawn.getMnemonic());
+      throw new UserExecException(
+          FailureDetail.newBuilder()
+              .setMessage(message)
+              .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NO_USABLE_STRATEGY_FOUND))
+              .build());
     }
 
-    return execableStrategies;
+    return strategies;
   }
 }

src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java

@@ -429,11 +429,12 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) {
   private static SpawnRunner withFallback(
       CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) {
     SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class);
-    return new SandboxFallbackSpawnRunner(
-        sandboxSpawnRunner,
-        createFallbackRunner(env),
-        env.getReporter(),
-        sandboxOptions != null && sandboxOptions.legacyLocalFallback);
+    if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) {
+      return new SandboxFallbackSpawnRunner(
+          sandboxSpawnRunner, createFallbackRunner(env), env.getReporter());
+    } else {
+      return sandboxSpawnRunner;
+    }
   }
 
   private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
@@ -450,29 +451,19 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
         RunfilesTreeUpdater.INSTANCE);
   }
 
-  /**
-   * A SpawnRunner that does sandboxing if possible, but might fall back to local execution if
-   * ----incompatible_legacy_local_fallback is true and no other strategy has been usable. This is a
-   * legacy functionality from before the strategies system was added, and can deceive the user into
-   * thinking a build is hermetic when it isn't really. TODO(b/178356138): Flip flag to default to
-   * false and then later remove this code entirely.
-   */
   private static final class SandboxFallbackSpawnRunner implements SpawnRunner {
     private final SpawnRunner sandboxSpawnRunner;
     private final SpawnRunner fallbackSpawnRunner;
     private final ExtendedEventHandler reporter;
     private static final AtomicBoolean warningEmitted = new AtomicBoolean();
-    private final boolean fallbackAllowed;
 
     SandboxFallbackSpawnRunner(
         SpawnRunner sandboxSpawnRunner,
         SpawnRunner fallbackSpawnRunner,
-        ExtendedEventHandler reporter,
-        boolean fallbackAllowed) {
+        ExtendedEventHandler reporter) {
       this.sandboxSpawnRunner = sandboxSpawnRunner;
       this.fallbackSpawnRunner = fallbackSpawnRunner;
       this.reporter = reporter;
-      this.fallbackAllowed = fallbackAllowed;
     }
 
     @Override
@@ -488,6 +479,12 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
       if (sandboxSpawnRunner.canExec(spawn)) {
         spawnResult = sandboxSpawnRunner.exec(spawn, context);
       } else {
+        if (warningEmitted.compareAndSet(false, true)) {
+          reporter.handle(
+              Event.warn(
+                  "Use of implicit local fallback will go away soon, please"
+                      + " set a fallback strategy instead. See --legacy_local_fallback option."));
+        }
         spawnResult = fallbackSpawnRunner.exec(spawn, context);
       }
       reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant));
@@ -496,38 +493,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
 
     @Override
     public boolean canExec(Spawn spawn) {
-      return sandboxSpawnRunner.canExec(spawn);
-    }
-
-    @Override
-    public boolean canExecWithLegacyFallback(Spawn spawn) {
-      boolean canExec = !sandboxSpawnRunner.canExec(spawn) && fallbackSpawnRunner.canExec(spawn);
-      if (canExec) {
-        // We give a single warning to use strategies instead, whether or not we allow the fallback
-        // to happen. This allows people to switch early, but also explains why the build fails
-        // once we flip the flag. Unfortunately, we can't easily tell if the flag was explicitly
-        // set, if we could we should omit the warnings in that case.
-        if (warningEmitted.compareAndSet(false, true)) {
-          if (fallbackAllowed) {
-            reporter.handle(
-                Event.warn(
-                    String.format(
-                        "%s uses implicit fallback from sandbox to local, which is deprecated"
-                            + " because it is not hermetic. Prefer setting an explicit list of"
-                            + " strategies.",
-                        spawn.getMnemonic())));
-          } else {
-            reporter.handle(
-                Event.warn(
-                    String.format(
-                        "Implicit fallback from sandbox to local is deprecated. Prefer setting an"
-                            + " explicit list of strategies for %s, or for now pass"
-                            + " --incompatible_legacy_local_fallback.",
-                        spawn.getMnemonic())));
-          }
-        }
-      }
-      return canExec && fallbackAllowed;
+      return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn);
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java

@@ -80,11 +80,12 @@ public String getTypeDescription() {
   }
 
   @Option(
-      name = "ignore_unsupported_sandboxing",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help = "Do not print a warning when sandboxed execution is not supported on this system.")
+    name = "ignore_unsupported_sandboxing",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    help = "Do not print a warning when sandboxed execution is not supported on this system."
+  )
   public boolean ignoreUnsupportedSandboxing;
 
   @Option(
@@ -114,19 +115,21 @@ public String getTypeDescription() {
   public String sandboxBase;
 
   @Option(
-      name = "sandbox_fake_hostname",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help = "Change the current hostname to 'localhost' for sandboxed actions.")
+    name = "sandbox_fake_hostname",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    help = "Change the current hostname to 'localhost' for sandboxed actions."
+  )
   public boolean sandboxFakeHostname;
 
   @Option(
-      name = "sandbox_fake_username",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help = "Change the current username to 'nobody' for sandboxed actions.")
+    name = "sandbox_fake_username",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    help = "Change the current username to 'nobody' for sandboxed actions."
+  )
   public boolean sandboxFakeUsername;
 
   @Option(
@@ -184,13 +187,14 @@ public String getTypeDescription() {
   public TriState useSandboxfs;
 
   @Option(
-      name = "experimental_sandboxfs_path",
-      defaultValue = "sandboxfs",
-      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help =
-          "Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a "
-              + "bare name, use the first binary of that name found in the PATH.")
+    name = "experimental_sandboxfs_path",
+    defaultValue = "sandboxfs",
+    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    help =
+        "Path to the sandboxfs binary to use when --experimental_use_sandboxfs is true. If a "
+            + "bare name, use the first binary of that name found in the PATH."
+  )
   public String sandboxfsPath;
 
   @Option(
@@ -243,48 +247,50 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
   }
 
   @Option(
-      name = "experimental_collect_local_sandbox_action_metrics",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-      effectTags = {OptionEffectTag.EXECUTION},
-      help =
-          "When enabled, execution statistics (such as user and system time) are recorded for "
-              + "locally executed actions which use sandboxing")
+    name = "experimental_collect_local_sandbox_action_metrics",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+    effectTags = {OptionEffectTag.EXECUTION},
+    help =
+        "When enabled, execution statistics (such as user and system time) are recorded for "
+            + "locally executed actions which use sandboxing"
+  )
   public boolean collectLocalSandboxExecutionStatistics;
 
   @Option(
-      name = "experimental_enable_docker_sandbox",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-      effectTags = {OptionEffectTag.EXECUTION},
-      help =
-          "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.")
+    name = "experimental_enable_docker_sandbox",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+    effectTags = {OptionEffectTag.EXECUTION},
+    help = "Enable Docker-based sandboxing. This option has no effect if Docker is not installed.")
   public boolean enableDockerSandbox;
 
   @Option(
-      name = "experimental_docker_image",
-      defaultValue = "",
-      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-      effectTags = {OptionEffectTag.EXECUTION},
-      help =
-          "Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute a"
-              + " sandboxed action when using the docker strategy and the action itself doesn't"
-              + " already have a container-image attribute in its remote_execution_properties in"
-              + " the platform description. The value of this flag is passed verbatim to 'docker"
-              + " run', so it supports the same syntax and mechanisms as Docker itself.")
+    name = "experimental_docker_image",
+    defaultValue = "",
+    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+    effectTags = {OptionEffectTag.EXECUTION},
+    help =
+        "Specify a Docker image name (e.g. \"ubuntu:latest\") that should be used to execute "
+            + "a sandboxed action when using the docker strategy and the action itself doesn't "
+            + "already have a container-image attribute in its remote_execution_properties in the "
+            + "platform description. The value of this flag is passed verbatim to 'docker run', so "
+            + "it supports the same syntax and mechanisms as Docker itself."
+  )
   public String dockerImage;
 
   @Option(
-      name = "experimental_docker_use_customized_images",
-      defaultValue = "true",
-      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-      effectTags = {OptionEffectTag.EXECUTION},
-      help =
-          "If enabled, injects the uid and gid of the current user into the Docker image before"
-              + " using it. This is required if your build / tests depend on the user having a name"
-              + " and home directory inside the container. This is on by default, but you can"
-              + " disable it in case the automatic image customization feature doesn't work in your"
-              + " case or you know that you don't need it.")
+    name = "experimental_docker_use_customized_images",
+    defaultValue = "true",
+    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+    effectTags = {OptionEffectTag.EXECUTION},
+    help =
+        "If enabled, injects the uid and gid of the current user into the Docker image before "
+            + "using it. This is required if your build / tests depend on the user having a name "
+            + "and home directory inside the container. This is on by default, but you can disable "
+            + "it in case the automatic image customization feature doesn't work in your case or "
+            + "you know that you don't need it."
+  )
   public boolean dockerUseCustomizedImages;
 
   @Option(
@@ -353,9 +359,8 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
       },
       help =
           "If set to true, enables the legacy implicit fallback from sandboxed to local strategy."
-              + " This flag will eventually default to false and then become a no-op. Use"
-              + " --strategy, --spawn_strategy, or --dynamic_local_strategy to configure fallbacks"
-              + " instead.")
+              + " This flag will eventually default to false and then become a no-op. You should"
+              + " use --strategy or --spawn_strategy to configure fallbacks instead.")
   public boolean legacyLocalFallback;
 
   /** Converter for the number of threads used for asynchronous tree deletion. */