diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 2a79e8a6edebf8..3424d74eb0e466 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -481,6 +481,16 @@ public static class GraveyardOptions extends OptionsBase { effectTags = {OptionEffectTag.NO_OP}, help = "No-op") public boolean skyframeEvalWithOrderedList; + + @Option( + name = "legacy_spawn_scheduler", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.UNKNOWN}, + defaultValue = "false", + deprecationWarning = + "The --legacy_spawn_scheduler flag is a no-op and will be removed soon.", + help = "Was used to enable the old spawn scheduler. Now a no-op.") + public boolean legacySpawnScheduler; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java index 14d550f655178f..3f876d98b7efab 100644 --- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java +++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java @@ -137,17 +137,12 @@ final void registerSpawnStrategies( return; } - SpawnStrategy strategy; - if (options.legacySpawnScheduler) { - strategy = new LegacyDynamicSpawnStrategy(executorService, options, this::getExecutionPolicy); - } else { - strategy = - new DynamicSpawnStrategy( - executorService, - options, - this::getExecutionPolicy, - this::getPostProcessingSpawnForLocalExecution); - } + SpawnStrategy strategy = + new DynamicSpawnStrategy( + executorService, + options, + this::getExecutionPolicy, + this::getPostProcessingSpawnForLocalExecution); registryBuilder.registerStrategy(strategy, "dynamic", "dynamic_worker"); registryBuilder.addDynamicLocalStrategies(getLocalStrategies(options)); diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionOptions.java index 3f570b5d666e8f..19d844eabd2c91 100644 --- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionOptions.java @@ -55,18 +55,6 @@ public class DynamicExecutionOptions extends OptionsBase { + "enabled.") public boolean internalSpawnScheduler; - @Option( - name = "legacy_spawn_scheduler", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.UNKNOWN}, - defaultValue = "false", - help = - "Enables the old but tested implementation of the spawn scheduler. This differs from the " - + "new version in that this version cannot stop a local spawn once it has started " - + "running. You should never have to enable the legacy scheduler except to " - + "workaround bugs in the new version.") - public boolean legacySpawnScheduler; - @Option( name = "dynamic_local_strategy", converter = Converters.StringToStringListConverter.class, diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/LegacyDynamicSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/dynamic/LegacyDynamicSpawnStrategy.java deleted file mode 100644 index 0bc3b567f9f881..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/dynamic/LegacyDynamicSpawnStrategy.java +++ /dev/null @@ -1,446 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.dynamic; - - -import com.google.auto.value.AutoValue; -import com.google.common.base.Preconditions; -import com.google.common.base.Strings; -import com.google.common.base.Throwables; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.common.flogger.GoogleLogger; -import com.google.common.io.Files; -import com.google.devtools.build.lib.actions.ActionContext; -import com.google.devtools.build.lib.actions.ActionExecutionContext; -import com.google.devtools.build.lib.actions.DynamicStrategyRegistry; -import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy; -import com.google.devtools.build.lib.actions.Spawn; -import com.google.devtools.build.lib.actions.SpawnResult; -import com.google.devtools.build.lib.actions.SpawnStrategy; -import com.google.devtools.build.lib.actions.UserExecException; -import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.exec.ExecutionPolicy; -import com.google.devtools.build.lib.server.FailureDetails.DynamicExecution; -import com.google.devtools.build.lib.server.FailureDetails.DynamicExecution.Code; -import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; -import com.google.devtools.build.lib.util.io.FileOutErr; -import com.google.devtools.build.lib.vfs.Path; -import java.io.IOException; -import java.util.List; -import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Phaser; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Function; -import javax.annotation.Nullable; - -/** - * A spawn strategy that speeds up incremental builds while not slowing down full builds. - * - *

This strategy tries to run spawn actions on the local and remote machine at the same time, and - * picks the spawn action that completes first. This gives the benefits of remote execution on full - * builds, and local execution on incremental builds. - * - *

One might ask, why we don't run spawns on the workstation all the time and just "spill over" - * actions to remote execution when there are no local resources available. This would work, except - * that the cost of transferring action inputs and outputs from the local machine to and from remote - * executors over the network is way too high - there is no point in executing an action locally and - * save 0.5s of time, when it then takes us 5 seconds to upload the results to remote executors for - * another action that's scheduled to run there. - */ -public class LegacyDynamicSpawnStrategy implements SpawnStrategy { - private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - - enum StrategyIdentifier { - NONE("unknown"), - LOCAL("locally"), - REMOTE("remotely"); - - private final String prettyName; - - StrategyIdentifier(String prettyName) { - this.prettyName = prettyName; - } - - String prettyName() { - return prettyName; - } - } - - @AutoValue - abstract static class DynamicExecutionResult { - static DynamicExecutionResult create( - StrategyIdentifier strategyIdentifier, - @Nullable FileOutErr fileOutErr, - @Nullable ExecException execException, - List spawnResults) { - return new AutoValue_LegacyDynamicSpawnStrategy_DynamicExecutionResult( - strategyIdentifier, fileOutErr, execException, ImmutableList.copyOf(spawnResults)); - } - - abstract StrategyIdentifier strategyIdentifier(); - - @Nullable - abstract FileOutErr fileOutErr(); - - @Nullable - abstract ExecException execException(); - - /** - * Returns a list of SpawnResults associated with executing a Spawn. - * - *

The list will typically contain one element, but could contain zero elements if spawn - * execution did not complete, or multiple elements if multiple sub-spawns were executed. - */ - abstract ImmutableList spawnResults(); - } - - private static final ImmutableSet DISABLED_MNEMONICS_FOR_WORKERS = - ImmutableSet.of("JavaDeployJar"); - - private final ExecutorService executorService; - private final DynamicExecutionOptions options; - private final Function getExecutionPolicy; - private final AtomicBoolean delayLocalExecution = new AtomicBoolean(false); - - // TODO(steinman): This field is never assigned and canExec() would throw if trying to access it. - @Nullable private SandboxedSpawnStrategy workerStrategy; - - /** - * Constructs a {@code DynamicSpawnStrategy}. - * - * @param executorService an {@link ExecutorService} that will be used to run Spawn actions. - */ - public LegacyDynamicSpawnStrategy( - ExecutorService executorService, - DynamicExecutionOptions options, - Function getExecutionPolicy) { - this.executorService = executorService; - this.options = options; - this.getExecutionPolicy = getExecutionPolicy; - } - - @Override - public ImmutableList exec( - final Spawn spawn, final ActionExecutionContext actionExecutionContext) - throws ExecException, InterruptedException { - DynamicSpawnStrategy.verifyAvailabilityInfo(options, spawn); - ExecutionPolicy executionPolicy = getExecutionPolicy.apply(spawn); - - // If a Spawn cannot run remotely, we must always execute it locally. Resources will already - // have been acquired by Skyframe for us. - if (executionPolicy.canRunLocallyOnly()) { - return runLocally(spawn, actionExecutionContext, null); - } - - // If a Spawn cannot run locally, we must always execute it remotely. For remote execution, - // local resources should not be acquired. - if (executionPolicy.canRunRemotelyOnly()) { - return runRemotely(spawn, actionExecutionContext, null); - } - - // At this point we have a Spawn that can run locally and can run remotely. Run it in parallel - // using both the remote and the local strategy. - ExecException exceptionDuringExecution = null; - DynamicExecutionResult dynamicExecutionResult = - DynamicExecutionResult.create( - StrategyIdentifier.NONE, null, null, /*spawnResults=*/ ImmutableList.of()); - - // As an invariant in Bazel, all actions must terminate before the build ends. We use a - // synchronizer here, in the main thread, to wait for the termination of both local and remote - // spawns. Termination implies successful completion, failure, or, if one spawn wins, - // cancellation by the executor. - // - // In the case where one task completes successfully before the other starts, Bazel must - // proceed and return, skipping the other spawn. To achieve this, we use Phaser for its ability - // to register a variable number of tasks. - // - // TODO(b/118451841): Note that this may incur a performance issue where a remote spawn is - // faster than a worker spawn, because the worker spawn cannot be cancelled once it starts. This - // nullifies the gains from the faster spawn. - Phaser bothTasksFinished = new Phaser(/*parties=*/ 1); - - try { - final AtomicReference outputsHaveBeenWritten = new AtomicReference<>(null); - dynamicExecutionResult = - executorService.invokeAny( - ImmutableList.of( - new DynamicExecutionCallable( - bothTasksFinished, - StrategyIdentifier.LOCAL, - actionExecutionContext.getFileOutErr()) { - @Override - List callImpl() throws InterruptedException, ExecException { - // This is a rather simple approach to make it possible to score a cache hit - // on remote execution before even trying to start the action locally. This - // saves resources that would otherwise be wasted by continuously starting and - // immediately killing local processes. One possibility for improvement would - // be to establish a reporting mechanism from strategies back to here, where - // we delay starting locally until the remote strategy tells us that the - // action isn't a cache hit. - if (delayLocalExecution.get()) { - Thread.sleep(options.localExecutionDelay); - } - return runLocally( - spawn, - actionExecutionContext.withFileOutErr(fileOutErr), - outputsHaveBeenWritten); - } - }, - new DynamicExecutionCallable( - bothTasksFinished, - StrategyIdentifier.REMOTE, - actionExecutionContext.getFileOutErr()) { - @Override - public List callImpl() throws InterruptedException, ExecException { - List spawnResults = - runRemotely( - spawn, - actionExecutionContext.withFileOutErr(fileOutErr), - outputsHaveBeenWritten); - delayLocalExecution.set(true); - return spawnResults; - } - })); - } catch (ExecutionException e) { - Throwables.propagateIfPossible(e.getCause(), InterruptedException.class); - // DynamicExecutionCallable.call only declares InterruptedException, so this should never - // happen. - exceptionDuringExecution = - new UserExecException( - e.getCause(), - createFailureDetail( - Strings.nullToEmpty(e.getCause().getMessage()), Code.RUN_FAILURE)); - } finally { - bothTasksFinished.arriveAndAwaitAdvance(); - if (dynamicExecutionResult.execException() != null) { - exceptionDuringExecution = dynamicExecutionResult.execException(); - } - if (Thread.currentThread().isInterrupted()) { - // Warn but don't throw, in case we're crashing. - logger.atWarning().log("Interrupted waiting for dynamic execution tasks to finish"); - } - } - // Check for interruption outside of finally block, so we don't mask any other exceptions. - // Clear the interrupt bit if it's set. - if (exceptionDuringExecution == null && Thread.interrupted()) { - throw new InterruptedException("Interrupted waiting for dynamic execution tasks to finish"); - } - StrategyIdentifier winningStrategy = dynamicExecutionResult.strategyIdentifier(); - FileOutErr fileOutErr = dynamicExecutionResult.fileOutErr(); - if (StrategyIdentifier.NONE.equals(winningStrategy) || fileOutErr == null) { - throw new IllegalStateException("Neither local or remote execution has started."); - } - - try { - moveFileOutErr(actionExecutionContext, fileOutErr); - } catch (IOException e) { - String strategyName = winningStrategy.name().toLowerCase(); - if (exceptionDuringExecution == null) { - throw new UserExecException( - e, - createFailureDetail( - String.format("Could not move action logs from %s execution", strategyName), - Code.ACTION_LOG_MOVE_FAILURE)); - } else { - actionExecutionContext - .getEventHandler() - .handle( - Event.warn( - String.format( - "Could not move action logs from %s execution: %s", - strategyName, e.toString()))); - } - } - - if (exceptionDuringExecution != null) { - throw exceptionDuringExecution; - } - - if (options.debugSpawnScheduler) { - actionExecutionContext - .getEventHandler() - .handle( - Event.info( - String.format( - "%s action %s %s", - spawn.getMnemonic(), - dynamicExecutionResult.execException() == null ? "finished" : "failed", - winningStrategy.prettyName()))); - } - - // TODO(b/62588075) If a second list of spawnResults was generated (before execution was - // cancelled), then we might want to save it as well (e.g. for metrics purposes). - return dynamicExecutionResult.spawnResults(); - } - - @Override - public boolean canExec(Spawn spawn, ActionContext.ActionContextRegistry actionContextRegistry) { - DynamicStrategyRegistry dynamicStrategyRegistry = - actionContextRegistry.getContext(DynamicStrategyRegistry.class); - - for (SandboxedSpawnStrategy strategy : - dynamicStrategyRegistry.getDynamicSpawnActionContexts( - spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - if (strategy.canExec(spawn, actionContextRegistry)) { - return true; - } - } - for (SandboxedSpawnStrategy strategy : - dynamicStrategyRegistry.getDynamicSpawnActionContexts( - spawn, DynamicStrategyRegistry.DynamicMode.REMOTE)) { - if (strategy.canExec(spawn, actionContextRegistry)) { - return true; - } - } - return workerStrategy.canExec(spawn, actionContextRegistry); - } - - private void moveFileOutErr(ActionExecutionContext actionExecutionContext, FileOutErr outErr) - throws IOException { - if (outErr.getOutputPath().exists()) { - Files.move( - outErr.getOutputPath().getPathFile(), - actionExecutionContext.getFileOutErr().getOutputPath().getPathFile()); - } - if (outErr.getErrorPath().exists()) { - Files.move( - outErr.getErrorPath().getPathFile(), - actionExecutionContext.getFileOutErr().getErrorPath().getPathFile()); - } - } - - private static FileOutErr getSuffixedFileOutErr(FileOutErr fileOutErr, String suffix) { - Path outDir = Preconditions.checkNotNull(fileOutErr.getOutputPath().getParentDirectory()); - String outBaseName = fileOutErr.getOutputPath().getBaseName(); - Path errDir = Preconditions.checkNotNull(fileOutErr.getErrorPath().getParentDirectory()); - String errBaseName = fileOutErr.getErrorPath().getBaseName(); - return new FileOutErr( - outDir.getChild(outBaseName + suffix), errDir.getChild(errBaseName + suffix)); - } - - private static SandboxedSpawnStrategy.StopConcurrentSpawns lockOutputFiles( - SandboxedSpawnStrategy token, @Nullable AtomicReference outputWriteBarrier) { - if (outputWriteBarrier == null) { - return null; - } else { - return () -> { - if (outputWriteBarrier.get() != token && !outputWriteBarrier.compareAndSet(null, token)) { - throw new DynamicInterruptedException( - "Execution stopped because other strategy finished first"); - } - }; - } - } - - private static ImmutableList runLocally( - Spawn spawn, - ActionExecutionContext actionExecutionContext, - @Nullable AtomicReference outputWriteBarrier) - throws ExecException, InterruptedException { - DynamicStrategyRegistry dynamicStrategyRegistry = - actionExecutionContext.getContext(DynamicStrategyRegistry.class); - - for (SandboxedSpawnStrategy strategy : - dynamicStrategyRegistry.getDynamicSpawnActionContexts( - spawn, DynamicStrategyRegistry.DynamicMode.LOCAL)) { - - if (strategy.toString().contains("worker") - && DISABLED_MNEMONICS_FOR_WORKERS.contains(spawn.getMnemonic())) { - continue; - } - if (strategy.canExec(spawn, actionExecutionContext)) { - return strategy.exec( - spawn, actionExecutionContext, lockOutputFiles(strategy, outputWriteBarrier)); - } - } - throw new RuntimeException( - "executorCreated not yet called or no default dynamic_local_strategy set"); - } - - private static ImmutableList runRemotely( - Spawn spawn, - ActionExecutionContext actionExecutionContext, - @Nullable AtomicReference outputWriteBarrier) - throws ExecException, InterruptedException { - DynamicStrategyRegistry dynamicStrategyRegistry = - actionExecutionContext.getContext(DynamicStrategyRegistry.class); - - for (SandboxedSpawnStrategy strategy : - dynamicStrategyRegistry.getDynamicSpawnActionContexts( - spawn, DynamicStrategyRegistry.DynamicMode.REMOTE)) { - if (strategy.canExec(spawn, actionExecutionContext)) { - return strategy.exec( - spawn, actionExecutionContext, lockOutputFiles(strategy, outputWriteBarrier)); - } - } - throw new RuntimeException( - "executorCreated not yet called or no default dynamic_remote_strategy set"); - } - - private static FailureDetail createFailureDetail(String message, Code detailedCode) { - return FailureDetail.newBuilder() - .setMessage(message) - .setDynamicExecution(DynamicExecution.newBuilder().setCode(detailedCode)) - .build(); - } - - private abstract static class DynamicExecutionCallable - implements Callable { - private final Phaser taskFinished; - private final StrategyIdentifier strategyIdentifier; - protected final FileOutErr fileOutErr; - - DynamicExecutionCallable( - Phaser taskFinished, - StrategyIdentifier strategyIdentifier, - FileOutErr fileOutErr) { - this.taskFinished = taskFinished; - this.strategyIdentifier = strategyIdentifier; - this.fileOutErr = getSuffixedFileOutErr(fileOutErr, "." + strategyIdentifier.name()); - } - - abstract List callImpl() throws InterruptedException, ExecException; - - @Override - public final DynamicExecutionResult call() throws InterruptedException { - taskFinished.register(); - try { - List spawnResults = callImpl(); - return DynamicExecutionResult.create(strategyIdentifier, fileOutErr, null, spawnResults); - } catch (Exception e) { - Throwables.throwIfInstanceOf(e, InterruptedException.class); - return DynamicExecutionResult.create( - strategyIdentifier, - fileOutErr, - e instanceof ExecException - ? (ExecException) e - : new UserExecException( - e, createFailureDetail(Strings.nullToEmpty(e.getMessage()), Code.RUN_FAILURE)), - /*spawnResults=*/ ImmutableList.of()); - } finally { - try { - fileOutErr.close(); - } catch (IOException ignored) { - // Nothing we can do here. - } - taskFinished.arriveAndDeregister(); - } - } - } -} diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java index d13558e6cf3f53..9c448091aff369 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java @@ -67,8 +67,7 @@ public class LocalExecutionOptions extends OptionsBase { help = "When true, the local spawn runner does lock the output tree during dynamic execution. " + "Instead, spawns are allowed to execute until they are explicitly interrupted by a " - + "faster remote action. Requires --legacy_spawn_scheduler=false because of the need " - + "for this explicit cancellation.") + + "faster remote action.") public boolean localLockfreeOutput; @Option( diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java index fbf972045a4d3f..79b92a2c530d99 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java +++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java @@ -22,7 +22,6 @@ import com.google.devtools.build.lib.analysis.test.TestActionContext; import com.google.devtools.build.lib.analysis.test.TestStrategy; import com.google.devtools.build.lib.buildtool.BuildRequest; -import com.google.devtools.build.lib.dynamic.DynamicExecutionOptions; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.FileWriteStrategy; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; @@ -39,11 +38,6 @@ import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.ProcessWrapper; -import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; -import com.google.devtools.build.lib.server.FailureDetails.LocalExecution; -import com.google.devtools.build.lib.server.FailureDetails.LocalExecution.Code; -import com.google.devtools.build.lib.util.AbruptExitException; -import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.vfs.Path; /** @@ -51,30 +45,6 @@ */ public class StandaloneModule extends BlazeModule { - @Override - public void beforeCommand(CommandEnvironment env) throws AbruptExitException { - LocalExecutionOptions localOptions = env.getOptions().getOptions(LocalExecutionOptions.class); - if (localOptions == null) { - // This module doesn't make sense in non-build commands (which don't register these options). - return; - } - - DynamicExecutionOptions dynamicOptions = - env.getOptions().getOptions(DynamicExecutionOptions.class); - if (dynamicOptions != null) { // Guard against tests that don't pull this module in. - if (localOptions.localLockfreeOutput && dynamicOptions.legacySpawnScheduler) { - throw new AbruptExitException( - DetailedExitCode.of( - FailureDetail.newBuilder() - .setMessage( - "--experimental_local_lockfree_output requires --nolegacy_spawn_scheduler") - .setLocalExecution( - LocalExecution.newBuilder().setCode(Code.LOCKFREE_OUTPUT_PREREQ_UNMET)) - .build())); - } - } - } - @Override public void registerActionContexts( ModuleActionContextRegistry.Builder registryBuilder, diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java index ae6b3e7085233f..995f716c0a071e 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java @@ -64,8 +64,6 @@ import com.google.devtools.common.options.OptionsParser; import java.io.FileNotFoundException; import java.io.IOException; -import java.util.Arrays; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Optional; @@ -79,12 +77,10 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameter; -import org.junit.runners.Parameterized.Parameters; +import org.junit.runners.JUnit4; /** Tests for {@link DynamicSpawnStrategy}. */ -@RunWith(Parameterized.class) +@RunWith(JUnit4.class) public class DynamicSpawnStrategyTest { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @@ -93,16 +89,6 @@ public class DynamicSpawnStrategyTest { private FileOutErr outErr; private final ActionKeyContext actionKeyContext = new ActionKeyContext(); - @Parameters(name = "{index}: legacy={0}") - public static Collection data() { - return Arrays.asList( - new Object[][] { - {true}, {false}, - }); - } - - @Parameter public boolean legacyBehavior; - /** Syntactic sugar to decrease and await for a latch in a single line. */ private static void countDownAndWait(CountDownLatch countDownLatch) throws InterruptedException { countDownLatch.countDown(); @@ -132,7 +118,7 @@ private class MockSpawnStrategy implements SandboxedSpawnStrategy { @Nullable private volatile Spawn executedSpawn; /** Tracks whether {@link #exec} completed successfully or not. */ - private CountDownLatch succeeded = new CountDownLatch(1); + private final CountDownLatch succeeded = new CountDownLatch(1); /** Hook to implement per-test custom logic. */ private final DoExec doExecBeforeStop; @@ -318,7 +304,6 @@ private StrategyAndContext createSpawnStrategyWithExecutor( options.dynamicWorkerStrategy = "mock-local"; options.internalSpawnScheduler = true; options.localExecutionDelay = 0; - options.legacySpawnScheduler = legacyBehavior; checkState(executorServiceForCleanup == null); executorServiceForCleanup = executorService; @@ -373,14 +358,11 @@ private StrategyAndContext createSpawnStrategyWithExecutor( newCustomSpawn("RunDynamic", ImmutableMap.of()), event -> {}); Optional optionalContext = - dynamicStrategies.stream() - .filter( - c -> c instanceof DynamicSpawnStrategy || c instanceof LegacyDynamicSpawnStrategy) - .findAny(); + dynamicStrategies.stream().filter(c -> c instanceof DynamicSpawnStrategy).findAny(); checkState(optionalContext.isPresent(), "Expected module to register a dynamic strategy"); return new AutoValue_DynamicSpawnStrategyTest_StrategyAndContext( - (SpawnStrategy) optionalContext.get(), actionExecutionContext); + optionalContext.get(), actionExecutionContext); } private static class NullActionWithMnemonic extends NullAction { @@ -682,13 +664,6 @@ public void actionFailsIfLocalAndRemoteFail() throws Exception { @Test public void stopConcurrentSpawnsWaitForCompletion() throws Exception { - if (legacyBehavior) { - // The legacy spawn scheduler does not implement cross-cancellations of the two parallel - // branches so this test makes no sense in that case. - logger.atInfo().log("Skipping test"); - return; - } - CountDownLatch countDownLatch = new CountDownLatch(2); AtomicBoolean slowCleanupFinished = new AtomicBoolean(false); @@ -827,16 +802,15 @@ interface CheckExecResult { private void assertThatStrategyWaitsForBothSpawnsToFinish( boolean executionFails, boolean interruptThread, CheckExecResult checkExecResult) throws Exception { - if (!legacyBehavior) { - // TODO(jmmv): I've spent *days* trying to make these tests work reliably with the new dynamic - // spawn scheduler implementation but I keep encountering tricky race conditions everywhere. I - // have strong reasons to believe that the races are due to inherent problems in these tests, - // not in the actual DynamicSpawnScheduler implementation. So whatever. I'll revisit these - // later as a new set of tests once I'm less tired^W^W^W the legacy spawn scheduler goes away. + if (true) { + // TODO(b/177406907): jmmv@: I spent *days* trying to make these tests work reliably with the + // new dynamic spawn scheduler implementation but I keep encountering tricky race conditions + // everywhere. I have strong reasons to believe that the races are due to inherent problems in + // these tests, not in the actual DynamicSpawnScheduler implementation. So whatever. We should + // revisit these as a new set of tests now that the legacy spawn scheduler has gone away. logger.atInfo().log("Skipping test"); return; } - AtomicBoolean stopLocal = new AtomicBoolean(false); CountDownLatch executionCanProceed = new CountDownLatch(2); CountDownLatch remoteDone = new CountDownLatch(1); @@ -1024,10 +998,7 @@ public void strategyPropagatesFasterLocalException() throws Exception { throw new AssertionError("Not reachable"); }; - assertThatStrategyPropagatesException( - localExec, - remoteExec, - legacyBehavior ? new UserExecException(e, createFailureDetail("")) : e); + assertThatStrategyPropagatesException(localExec, remoteExec, e); } @Test @@ -1044,10 +1015,7 @@ public void strategyPropagatesFasterRemoteException() throws Exception { throw e; }; - assertThatStrategyPropagatesException( - localExec, - remoteExec, - legacyBehavior ? new UserExecException(e, createFailureDetail("")) : e); + assertThatStrategyPropagatesException(localExec, remoteExec, e); } private static FailureDetail createFailureDetail(String message) { diff --git a/src/test/shell/integration/execution_strategies_test.sh b/src/test/shell/integration/execution_strategies_test.sh index 4b7c88d2ebd871..abc29bdd6846a1 100755 --- a/src/test/shell/integration/execution_strategies_test.sh +++ b/src/test/shell/integration/execution_strategies_test.sh @@ -100,7 +100,7 @@ function build_and_interrupt() { local file="${1}"; shift bazel clean - bazel build --genrule_strategy=local --nolegacy_spawn_scheduler \ + bazel build --genrule_strategy=local \ "${@}" //pkg &> $TEST_log & local pid=$! while [[ ! -e "${dir}" && ! -e "${file}" ]]; do