diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java index 5562bf03996505..d1fe0425f564ab 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java @@ -60,6 +60,7 @@ import java.time.Instant; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; /** This module provides the Sandbox spawn strategy. */ @@ -425,9 +426,15 @@ private static Path getPathToDockerClient(CommandEnvironment cmdEnv) { return null; } - private static SpawnRunner withFallback(CommandEnvironment env, SpawnRunner sandboxSpawnRunner) { - return new SandboxFallbackSpawnRunner( - sandboxSpawnRunner, createFallbackRunner(env), env.getReporter()); + private static SpawnRunner withFallback( + CommandEnvironment env, AbstractSandboxSpawnRunner sandboxSpawnRunner) { + SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class); + if (sandboxOptions != null && sandboxOptions.legacyLocalFallback) { + return new SandboxFallbackSpawnRunner( + sandboxSpawnRunner, createFallbackRunner(env), env.getReporter()); + } else { + return sandboxSpawnRunner; + } } private static SpawnRunner createFallbackRunner(CommandEnvironment env) { @@ -447,15 +454,16 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) { private static final class SandboxFallbackSpawnRunner implements SpawnRunner { private final SpawnRunner sandboxSpawnRunner; private final SpawnRunner fallbackSpawnRunner; - private final ExtendedEventHandler extendedEventHandler; + private final ExtendedEventHandler reporter; + private static final AtomicBoolean warningEmitted = new AtomicBoolean(); SandboxFallbackSpawnRunner( SpawnRunner sandboxSpawnRunner, SpawnRunner fallbackSpawnRunner, - ExtendedEventHandler extendedEventHandler) { + ExtendedEventHandler reporter) { this.sandboxSpawnRunner = sandboxSpawnRunner; this.fallbackSpawnRunner = fallbackSpawnRunner; - this.extendedEventHandler = extendedEventHandler; + this.reporter = reporter; } @Override @@ -471,10 +479,15 @@ 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); } - extendedEventHandler.post( - new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant)); + reporter.post(new SpawnExecutedEvent(spawn, spawnResult, spawnExecutionStartInstant)); return spawnResult; } @@ -491,7 +504,9 @@ public boolean handlesCaching() { @Override public void cleanupSandboxBase(Path sandboxBase, TreeDeleter treeDeleter) throws IOException { sandboxSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter); - fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter); + if (fallbackSpawnRunner != null) { + fallbackSpawnRunner.cleanupSandboxBase(sandboxBase, treeDeleter); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java index 47fc52637919b8..57d78dcc309ef7 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java @@ -27,6 +27,7 @@ import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.TriState; @@ -347,6 +348,21 @@ public ImmutableSet getInaccessiblePaths(FileSystem fs) { + "scheduler. This flag exists purely to support rolling this bug fix out.") public boolean delayVirtualInputMaterialization; + @Option( + name = "incompatible_legacy_local_fallback", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + 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. 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. */ public static final class AsyncTreeDeletesConverter extends ResourceConverter { public AsyncTreeDeletesConverter() { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 88d0962b49589b..43a3c058b6fdce 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -25,15 +25,11 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; -import com.google.devtools.build.lib.exec.SpawnRunner; import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; -import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; -import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent; import com.google.devtools.build.lib.sandbox.SandboxHelpers; import com.google.devtools.build.lib.sandbox.SandboxOptions; @@ -167,7 +163,6 @@ public void registerSpawnStrategies( workerPool, options.workerMultiplex, env.getReporter(), - createFallbackRunner(env, localEnvProvider), localEnvProvider, env.getBlazeWorkspace().getBinTools(), env.getLocalResourceManager(), @@ -181,21 +176,6 @@ public void registerSpawnStrategies( "worker"); } - private static SpawnRunner createFallbackRunner( - CommandEnvironment env, LocalEnvProvider localEnvProvider) { - LocalExecutionOptions localExecutionOptions = - env.getOptions().getOptions(LocalExecutionOptions.class); - return new LocalSpawnRunner( - env.getExecRoot(), - localExecutionOptions, - env.getLocalResourceManager(), - localEnvProvider, - env.getBlazeWorkspace().getBinTools(), - ProcessWrapper.fromCommandEnvironment(env), - // TODO(buchgr): Replace singleton by a command-scoped RunfilesTreeUpdater - RunfilesTreeUpdater.INSTANCE); - } - @Subscribe public void buildComplete(BuildCompleteEvent event) { if (options != null && options.workerQuitAfterBuild) { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 26f6019fa53136..354f7ca5e818a0 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -38,7 +38,6 @@ import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.actions.UserExecException; -import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; @@ -89,7 +88,6 @@ final class WorkerSpawnRunner implements SpawnRunner { private final WorkerPool workers; private final boolean multiplex; private final ExtendedEventHandler reporter; - private final SpawnRunner fallbackRunner; private final LocalEnvProvider localEnvProvider; private final BinTools binTools; private final ResourceManager resourceManager; @@ -103,7 +101,6 @@ public WorkerSpawnRunner( WorkerPool workers, boolean multiplex, ExtendedEventHandler reporter, - SpawnRunner fallbackRunner, LocalEnvProvider localEnvProvider, BinTools binTools, ResourceManager resourceManager, @@ -114,7 +111,6 @@ public WorkerSpawnRunner( this.workers = Preconditions.checkNotNull(workers); this.multiplex = multiplex; this.reporter = reporter; - this.fallbackRunner = fallbackRunner; this.localEnvProvider = localEnvProvider; this.binTools = binTools; this.resourceManager = resourceManager; @@ -127,24 +123,6 @@ public String getName() { return "worker"; } - @Override - public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) - throws ExecException, IOException, InterruptedException { - if (!Spawns.supportsWorkers(spawn) && !Spawns.supportsMultiplexWorkers(spawn)) { - // TODO(ulfjack): Don't circumvent SpawnExecutionPolicy. Either drop the warning here, or - // provide a mechanism in SpawnExecutionPolicy to report warnings. - reporter.handle( - Event.warn( - String.format(ERROR_MESSAGE_PREFIX + REASON_NO_EXECUTION_INFO, spawn.getMnemonic()))); - return fallbackRunner.exec(spawn, context); - } - - context.report( - ProgressStatus.SCHEDULING, - WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn))); - return actuallyExec(spawn, context); - } - @Override public boolean canExec(Spawn spawn) { if (!Spawns.supportsWorkers(spawn) && !Spawns.supportsMultiplexWorkers(spawn)) { @@ -161,8 +139,12 @@ public boolean handlesCaching() { return false; } - private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context) + @Override + public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) throws ExecException, IOException, InterruptedException { + context.report( + ProgressStatus.SCHEDULING, + WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn))); if (spawn.getToolFiles().isEmpty()) { throw createUserExecException( String.format(ERROR_MESSAGE_PREFIX + REASON_NO_TOOLS, spawn.getMnemonic()), diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index 0b035a923ad465..d81f3a65ecab6e 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -101,7 +101,6 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept createWorkerPool(), /* multiplex */ false, reporter, - null, localEnvProvider, /* binTools */ null, resourceManager, @@ -139,7 +138,6 @@ private void assertRecordedResponsethrowsException(String recordedResponse, Stri createWorkerPool(), /* multiplex */ false, reporter, - null, localEnvProvider, /* binTools */ null, resourceManager,