From ac3439c8b3ab61c095c10bac836a2910d2328b20 Mon Sep 17 00:00:00 2001 From: janakr Date: Tue, 1 Feb 2022 07:12:10 -0800 Subject: [PATCH 1/3] Add a method, Spawn#isOutputMandatory, to indicate whether a Spawn must create an output by the end of its execution. If not, a Spawn (like a test) may succeed without necessarily creating that output. The 4 categories of actions that do this are: 1. Tests (tests can create XML and other files, but may not). 2. Java compilations with reduced classpaths (the initial compilation with a reduced classpath may fail, but produce a usable jdeps proto file, so the Spawn will claim that it succeeded so that the action can process the proto file). 3. Extra actions with a dummy output that is produced locally, not by the Spawn. However, by changing the outputs of the Spawn to be empty, this situation is avoided. 4. C++ compilations with coverage (a .gcno coverage file is not produced for an empty translation unit). In particular, all SpawnActions' Spawns have #isMandatoryOutput always return true, and there should be no new cases of #isMandatoryOutput returning false besides the ones added in this CL. PiperOrigin-RevId: 425616085 --- .../devtools/build/lib/actions/BaseSpawn.java | 4 +- .../build/lib/actions/DelegateSpawn.java | 7 ++- .../build/lib/actions/SimpleSpawn.java | 20 ++++-- .../devtools/build/lib/actions/Spawn.java | 27 ++++++-- .../lib/analysis/actions/SpawnAction.java | 43 ++++++++----- .../lib/analysis/actions/StarlarkAction.java | 3 +- .../build/lib/analysis/extra/ExtraAction.java | 31 +++++---- .../lib/exec/StandaloneTestStrategy.java | 15 +++-- .../build/lib/rules/cpp/CppCompileAction.java | 63 ++++++++++++------- .../lib/rules/java/JavaCompileAction.java | 16 ++++- .../rules/java/JavaCompileActionBuilder.java | 3 +- .../lib/analysis/actions/SpawnActionTest.java | 3 +- .../build/lib/exec/util/SpawnBuilder.java | 12 +++- 13 files changed, 167 insertions(+), 80 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java index fe5cc2e386eb8a..36ab4aded7ad8b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java @@ -16,11 +16,11 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Set; @@ -113,7 +113,7 @@ public NestedSet getInputFiles() { } @Override - public Collection getOutputFiles() { + public ImmutableSet getOutputFiles() { return action.getOutputs(); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java index 1a810879f4cfb7..112452344d3d26 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java @@ -29,7 +29,7 @@ public class DelegateSpawn implements Spawn { private final Spawn spawn; - public DelegateSpawn(Spawn spawn){ + public DelegateSpawn(Spawn spawn) { this.spawn = spawn; } @@ -73,6 +73,11 @@ public Collection getOutputFiles() { return spawn.getOutputFiles(); } + @Override + public boolean isMandatoryOutput(ActionInput output) { + return spawn.isMandatoryOutput(output); + } + @Override public ActionExecutionMetadata getResourceOwner() { return spawn.getResourceOwner(); diff --git a/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java index 4dfbbf51ff3491..b96d631b114b52 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java @@ -22,13 +22,11 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import java.util.Set; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; -/** - * Immutable implementation of a Spawn that does not perform any processing on the parameters. - * Prefer this over all other Spawn implementations. - */ +/** Immutable implementation of a Spawn that does not perform any processing on the parameters. */ @Immutable public final class SimpleSpawn implements Spawn { private final ActionExecutionMetadata owner; @@ -41,6 +39,8 @@ public final class SimpleSpawn implements Spawn { private final ImmutableMap> filesetMappings; private final ImmutableList outputs; private final ResourceSet localResources; + // If null, all outputs are mandatory. + @Nullable private final Set mandatoryOutputs; public SimpleSpawn( ActionExecutionMetadata owner, @@ -52,6 +52,7 @@ public SimpleSpawn( NestedSet inputs, NestedSet tools, ImmutableSet outputs, + @Nullable Set mandatoryOutputs, ResourceSet localResources) { this.owner = Preconditions.checkNotNull(owner); this.arguments = Preconditions.checkNotNull(arguments); @@ -63,6 +64,7 @@ public SimpleSpawn( runfilesSupplier == null ? EmptyRunfilesSupplier.INSTANCE : runfilesSupplier; this.filesetMappings = filesetMappings; this.outputs = Preconditions.checkNotNull(outputs).asList(); + this.mandatoryOutputs = mandatoryOutputs; this.localResources = Preconditions.checkNotNull(localResources); } @@ -73,7 +75,7 @@ public SimpleSpawn( ImmutableMap executionInfo, NestedSet inputs, ImmutableSet outputs, - ResourceSet localResources) { + ResourceSet resourceSet) { this( owner, arguments, @@ -84,7 +86,8 @@ public SimpleSpawn( inputs, NestedSetBuilder.emptySet(Order.STABLE_ORDER), outputs, - localResources); + /*mandatoryOutputs=*/ null, + resourceSet); } @Override @@ -127,6 +130,11 @@ public ImmutableList getOutputFiles() { return outputs; } + @Override + public boolean isMandatoryOutput(ActionInput output) { + return mandatoryOutputs == null || mandatoryOutputs.contains(output); + } + @Override public ActionExecutionMetadata getResourceOwner() { return owner; diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java index 7de04bd563da5d..cf47e4203f63ff 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java @@ -99,18 +99,35 @@ public interface Spawn extends DescribableExecutionUnit { NestedSet getInputFiles(); /** - * Returns the collection of files that this command must write. Callers should not mutate - * the result. + * Returns the collection of files that this command will write. Callers should not mutate the + * result. * *

This is for use with remote execution, so remote execution does not have to guess what - * outputs the process writes. While the order does not affect the semantics, it should be - * stable so it can be cached. + * outputs the process writes. While the order does not affect the semantics, it should be stable + * so it can be cached. */ Collection getOutputFiles(); /** - * Returns the resource owner for local fallback. + * Returns true if {@code output} must be created for the action to succeed. Can be used by remote + * execution implementations to mark a command as failed if it did not create an output, even if + * the command itself exited with a successful exit code. + * + *

Some actions, like tests, may have optional files (like .xml files) that may be created, but + * are not required, so their spawns should return false for those optional files. Note that in + * general, every output in {@link ActionAnalysisMetadata#getOutputs} is checked for existence in + * {@link com.google.devtools.build.lib.skyframe.SkyframeActionExecutor#checkOutputs}, so + * eventually all those outputs must be produced by at least one {@code Spawn} for that action, or + * locally by the action in some cases. + * + *

This method should not be overridden by any new Spawns if possible: outputs should be + * mandatory. */ + default boolean isMandatoryOutput(ActionInput output) { + return true; + } + + /** Returns the resource owner for local fallback. */ ActionExecutionMetadata getResourceOwner(); /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index c3568932536580..14c42dd77ddd4b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -340,6 +340,11 @@ private ActionExecutionException createCommandLineException(CommandLineExpansion return new ActionExecutionException(e, this, /*catastrophe=*/ false, detailedExitCode); } + @VisibleForTesting + public ResourceSetOrBuilder getResourceSetOrBuilder() { + return resourceSetOrBuilder; + } + /** * Returns a Spawn that is representative of the command that this Action will execute. This * function must not modify any state. @@ -361,12 +366,13 @@ final Spawn getSpawn(NestedSet inputs) /*envResolved=*/ false, inputs, /*additionalInputs=*/ ImmutableList.of(), - /*filesetMappings=*/ ImmutableMap.of()); + /*filesetMappings=*/ ImmutableMap.of(), + /*reportOutputs=*/ true); } /** - * Return a spawn that is representative of the command that this Action will execute in the given - * client environment. + * Returns a spawn that is representative of the command that this Action will execute in the + * given client environment. */ public Spawn getSpawn(ActionExecutionContext actionExecutionContext) throws CommandLineExpansionException, InterruptedException { @@ -374,7 +380,8 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext) actionExecutionContext.getArtifactExpander(), actionExecutionContext.getClientEnv(), /*envResolved=*/ false, - actionExecutionContext.getTopLevelFilesets()); + actionExecutionContext.getTopLevelFilesets(), + /*reportOutputs=*/ true); } /** @@ -385,11 +392,12 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext) * effective environment. Otherwise they will be used as client environment to resolve the * action env. */ - Spawn getSpawn( + protected Spawn getSpawn( ArtifactExpander artifactExpander, Map env, boolean envResolved, - Map> filesetMappings) + Map> filesetMappings, + boolean reportOutputs) throws CommandLineExpansionException, InterruptedException { ExpandedCommandLines expandedCommandLines = commandLines.expand(artifactExpander, getPrimaryOutput().getExecPath(), commandLineLimits); @@ -399,7 +407,8 @@ Spawn getSpawn( envResolved, getInputs(), expandedCommandLines.getParamFiles(), - filesetMappings); + filesetMappings, + reportOutputs); } Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException { @@ -554,10 +563,11 @@ public Map getExecutionInfo() { } /** A spawn instance that is tied to a specific SpawnAction. */ - private class ActionSpawn extends BaseSpawn { + private final class ActionSpawn extends BaseSpawn { private final NestedSet inputs; private final Map> filesetMappings; private final ImmutableMap effectiveEnvironment; + private final boolean reportOutputs; /** * Creates an ActionSpawn with the given environment variables. @@ -571,7 +581,8 @@ private ActionSpawn( boolean envResolved, NestedSet inputs, Iterable additionalInputs, - Map> filesetMappings) + Map> filesetMappings, + boolean reportOutputs) throws CommandLineExpansionException { super( arguments, @@ -592,16 +603,15 @@ private ActionSpawn( this.inputs = inputsBuilder.build(); this.filesetMappings = filesetMappings; - /** - * If the action environment is already resolved using the client environment, the given - * environment variables are used as they are. Otherwise, they are used as clientEnv to - * resolve the action environment variables - */ + // If the action environment is already resolved using the client environment, the given + // environment variables are used as they are. Otherwise, they are used as clientEnv to + // resolve the action environment variables. if (envResolved) { effectiveEnvironment = ImmutableMap.copyOf(env); } else { effectiveEnvironment = SpawnAction.this.getEffectiveEnvironment(env); } + this.reportOutputs = reportOutputs; } @Override @@ -618,6 +628,11 @@ public ImmutableMap> getFilesetMap public NestedSet getInputFiles() { return inputs; } + + @Override + public ImmutableSet getOutputFiles() { + return reportOutputs ? super.getOutputFiles() : ImmutableSet.of(); + } } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java index 867f2ddbbe216a..e0648697dd0982 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java @@ -324,7 +324,8 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext) actionExecutionContext.getArtifactExpander(), getEffectiveEnvironment(actionExecutionContext.getClientEnv()), /*envResolved=*/ true, - actionExecutionContext.getTopLevelFilesets()); + actionExecutionContext.getTopLevelFilesets(), + /*reportOutputs=*/ true); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java index 342f3a4ce5b7ad..f6e6d061e4e154 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java @@ -26,12 +26,14 @@ import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CommandLine; +import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLines; import com.google.devtools.build.lib.actions.CommandLines.CommandLineLimits; import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.RunfilesSupplier; +import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -54,17 +56,8 @@ public final class ExtraAction extends SpawnAction { private final boolean createDummyOutput; private final NestedSet extraActionInputs; - /** - * A long way to say (ExtraAction xa) -> xa.getShadowedAction(). - */ public static final Function GET_SHADOWED_ACTION = - new Function() { - @Nullable - @Override - public Action apply(@Nullable ExtraAction extraAction) { - return extraAction != null ? extraAction.getShadowedAction() : null; - } - }; + e -> e != null ? e.getShadowedAction() : null; ExtraAction( NestedSet extraActionInputs, @@ -153,6 +146,20 @@ public NestedSet getAllowedDerivedInputs() { return shadowedAction.getAllowedDerivedInputs(); } + @Override + public Spawn getSpawn(ActionExecutionContext actionExecutionContext) + throws CommandLineExpansionException, InterruptedException { + if (!createDummyOutput) { + return super.getSpawn(actionExecutionContext); + } + return getSpawn( + actionExecutionContext.getArtifactExpander(), + actionExecutionContext.getClientEnv(), + /*envResolved=*/ false, + actionExecutionContext.getTopLevelFilesets(), + /*reportOutputs=*/ false); + } + @Override protected void afterExecute( ActionExecutionContext actionExecutionContext, List spawnResults) @@ -171,9 +178,7 @@ protected void afterExecute( } } - /** - * Returns the action this extra action is 'shadowing'. - */ + /** Returns the action this extra action is 'shadowing'. */ public Action getShadowedAction() { return shadowedAction; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 569e419566e1ea..ef99daec0bb371 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -151,6 +151,7 @@ public TestRunnerSpawn createTestRunnerSpawn( ? action.getTools() : NestedSetBuilder.emptySet(Order.STABLE_ORDER), createSpawnOutputs(action), + /*mandatoryOutputs=*/ ImmutableSet.of(), localResourceUsage); Path execRoot = actionExecutionContext.getExecRoot(); ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver(); @@ -558,13 +559,10 @@ private static Spawn createXmlGeneratingSpawn( Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()), /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), /*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())), + /*mandatoryOutputs=*/ null, SpawnAction.DEFAULT_RESOURCE_SET); } - /** - * A spawn to generate a test.xml file from the test log. This is only used if the test does not - * generate a test.xml file itself. - */ private static Spawn createCoveragePostProcessingSpawn( ActionExecutionContext actionExecutionContext, TestRunnerAction action, @@ -588,8 +586,8 @@ private static Spawn createCoveragePostProcessingSpawn( ImmutableMap.copyOf(testEnvironment), ImmutableMap.copyOf(action.getExecutionInfo()), action.getLcovMergerRunfilesSupplier(), - /* filesetMappings= */ ImmutableMap.of(), - /* inputs= */ NestedSetBuilder.compileOrder() + /*filesetMappings=*/ ImmutableMap.of(), + /*inputs=*/ NestedSetBuilder.compileOrder() .addTransitive(action.getInputs()) .addAll(expandedCoverageDir) .add(action.getCollectCoverageScript()) @@ -597,9 +595,10 @@ private static Spawn createCoveragePostProcessingSpawn( .add(action.getCoverageManifest()) .addTransitive(action.getLcovMergerFilesToRun().build()) .build(), - /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), - /* outputs= */ ImmutableSet.of( + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*outputs=*/ ImmutableSet.of( ActionInputHelper.fromPath(action.getCoverageData().getExecPath())), + /*mandatoryOutputs=*/ null, SpawnAction.DEFAULT_RESOURCE_SET); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index d3d33f7109f72c..eececf7312c480 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -62,6 +62,7 @@ import com.google.devtools.build.lib.actions.extra.EnvironmentVariable; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.analysis.starlark.Args; +import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -144,6 +145,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable private final boolean usePic; private final boolean useHeaderModules; protected final boolean needsIncludeValidation; + private final boolean hasCoverageArtifact; private final CcCompilationContext ccCompilationContext; private final ImmutableList builtinIncludeFiles; @@ -306,6 +308,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable .getParentDirectory() .getChild(outputFile.getFilename() + ".params"); } + this.hasCoverageArtifact = gcnoFile != null; } private static ImmutableSet collectOutputs( @@ -316,6 +319,10 @@ private static ImmutableSet collectOutputs( @Nullable Artifact ltoIndexingFile, ImmutableList additionalOutputs) { ImmutableSet.Builder outputs = ImmutableSet.builder(); + // gcnoFile comes first because easy access to it is occasionally useful. + if (gcnoFile != null) { + outputs.add(gcnoFile); + } outputs.addAll(additionalOutputs); if (outputFile != null) { outputs.add(outputFile); @@ -323,9 +330,6 @@ private static ImmutableSet collectOutputs( if (dotdFile != null) { outputs.add(dotdFile); } - if (gcnoFile != null) { - outputs.add(gcnoFile); - } if (dwoFile != null) { outputs.add(dwoFile); } @@ -1500,8 +1504,15 @@ protected Spawn createSpawn(Path execRoot, Map clientEnv) ImmutableList.copyOf(getArguments()), getEffectiveEnvironment(clientEnv), executionInfo.build(), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), inputs, + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), getOutputs(), + // In coverage mode, .gcno file not produced for an empty translation unit. + /*mandatoryOutputs=*/ hasCoverageArtifact + ? ImmutableSet.copyOf(getOutputs().asList().subList(1, getOutputs().size())) + : null, estimateResourceConsumptionLocal( enabledCppCompileResourcesEstimation(), getMnemonic(), @@ -1590,27 +1601,35 @@ public List getPermittedSystemIncludePrefixes(Path execRoot) { } /** - * Gcc only creates ".gcno" files if the compilation unit is non-empty. To ensure that the set of + * Gcc only creates a ".gcno" file if the compilation unit is non-empty. To ensure that the set of * outputs for a CppCompileAction remains consistent and doesn't vary dynamically depending on the - * _contents_ of the input files, we create empty ".gcno" files if gcc didn't create them. + * _contents_ of the input files, we create an empty ".gcno" file if gcc didn't create it. */ - private void ensureCoverageNotesFilesExist(ActionExecutionContext actionExecutionContext) + private void ensureCoverageNotesFileExists(ActionExecutionContext actionExecutionContext) throws ActionExecutionException { - for (Artifact output : getOutputs()) { - if (output.isFileType(CppFileTypes.COVERAGE_NOTES)) { // ".gcno" - Path outputPath = actionExecutionContext.getInputPath(output); - if (outputPath.exists()) { - continue; - } - try { - FileSystemUtils.createEmptyFile(outputPath); - } catch (IOException e) { - String message = "Error creating file '" + outputPath + "': " + e.getMessage(); - DetailedExitCode code = - createDetailedExitCode(message, Code.COVERAGE_NOTES_CREATION_FAILURE); - throw new ActionExecutionException(message, e, this, false, code); - } - } + if (!hasCoverageArtifact) { + return; + } + Artifact gcnoArtifact = getOutputs().iterator().next(); + if (!gcnoArtifact.isFileType(CppFileTypes.COVERAGE_NOTES)) { + BugReport.sendBugReport( + new IllegalStateException( + "In coverage mode but gcno artifact is not first output: " + + gcnoArtifact + + ", " + + this)); + return; + } + Path outputPath = actionExecutionContext.getInputPath(gcnoArtifact); + if (outputPath.exists()) { + return; + } + try { + FileSystemUtils.createEmptyFile(outputPath); + } catch (IOException e) { + String message = "Error creating file '" + outputPath + "': " + e.getMessage(); + DetailedExitCode code = createDetailedExitCode(message, Code.COVERAGE_NOTES_CREATION_FAILURE); + throw new ActionExecutionException(message, e, this, false, code); } } @@ -1822,7 +1841,7 @@ public ActionContinuationOrResult execute() copyTempOutErrToActionOutErr(); - ensureCoverageNotesFilesExist(actionExecutionContext); + ensureCoverageNotesFileExists(actionExecutionContext); CppIncludeExtractionContext scanningContext = actionExecutionContext.getContext(CppIncludeExtractionContext.class); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 63347864f01d86..31f97c27891a0f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -317,7 +317,8 @@ JavaSpawn getReducedSpawn( expandedCommandLines, getEffectiveEnvironment(actionExecutionContext.getClientEnv()), executionInfo, - inputs); + inputs, + /*onlyMandatoryOutput=*/ fallback ? null : outputDepsProto); } private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) @@ -344,7 +345,8 @@ private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) // .jdeps files, which have config prefixes in output paths, which compromise caching // possible by stripping prefixes on the executor. .addTransitive(dependencyArtifacts) - .build()); + .build(), + /*onlyMandatoryOutput=*/ null); } @Override @@ -481,12 +483,14 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont private final class JavaSpawn extends BaseSpawn { final NestedSet inputs; + private final Artifact onlyMandatoryOutput; public JavaSpawn( CommandLines.ExpandedCommandLines expandedCommandLines, Map environment, Map executionInfo, - NestedSet inputs) { + NestedSet inputs, + @Nullable Artifact onlyMandatoryOutput) { super( ImmutableList.copyOf(expandedCommandLines.arguments()), environment, @@ -494,6 +498,7 @@ public JavaSpawn( EmptyRunfilesSupplier.INSTANCE, JavaCompileAction.this, LOCAL_RESOURCES); + this.onlyMandatoryOutput = onlyMandatoryOutput; this.inputs = NestedSetBuilder.fromNestedSet(inputs) .addAll(expandedCommandLines.getParamFiles()) @@ -504,6 +509,11 @@ public JavaSpawn( public NestedSet getInputFiles() { return inputs; } + + @Override + public boolean isMandatoryOutput(ActionInput output) { + return onlyMandatoryOutput == null || onlyMandatoryOutput.equals(output); + } } @VisibleForTesting diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index cc34c32de6f14d..492deef8d7792e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.util.StringCanonicalizer; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collections; +import java.util.Objects; import java.util.Optional; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -288,7 +289,7 @@ private ImmutableSet allOutputs() { .add(outputs.output()) .addAll(additionalOutputs); Stream.of(outputs.depsProto(), outputs.nativeHeader(), genSourceOutput, manifestOutput) - .filter(x -> x != null) + .filter(Objects::nonNull) .forEachOrdered(result::add); return result.build(); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java index 3500634000c4b2..30261ce8db4946 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java @@ -218,7 +218,8 @@ public void testBuilderWithJavaExecutableAndParameterFile2() throws Exception { (artifact, outputs) -> outputs.add(artifact), ImmutableMap.of(), /*envResolved=*/ false, - ImmutableMap.of()); + ImmutableMap.of(), + /*reportOutputs=*/ true); String paramFileName = output.getExecPathString() + "-0.params"; // The spawn's primary arguments should reference the param file assertThat(spawn.getArguments()) diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java index 60ec4d08f49fb2..a3c4761dba7f09 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java @@ -35,11 +35,10 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import javax.annotation.Nullable; -/** - * Builder class to create {@link Spawn} instances for testing. - */ +/** Builder class to create {@link Spawn} instances for testing. */ public final class SpawnBuilder { private String mnemonic = "Mnemonic"; private String progressMessage = "progress message"; @@ -52,6 +51,7 @@ public final class SpawnBuilder { private ImmutableMap execProperties = ImmutableMap.of(); private final NestedSetBuilder inputs = NestedSetBuilder.stableOrder(); private final List outputs = new ArrayList<>(); + @Nullable private Set mandatoryOutputs; private final Map> filesetMappings = new HashMap<>(); private final NestedSetBuilder tools = NestedSetBuilder.stableOrder(); @@ -77,6 +77,7 @@ public Spawn build() { inputs.build(), tools.build(), ImmutableSet.copyOf(outputs), + mandatoryOutputs, resourceSet); } @@ -160,6 +161,11 @@ public SpawnBuilder withOutputs(String... names) { return this; } + public SpawnBuilder withMandatoryOutputs(@Nullable Set mandatoryOutputs) { + this.mandatoryOutputs = mandatoryOutputs; + return this; + } + public SpawnBuilder withFilesetMapping( Artifact fileset, ImmutableList mappings) { Preconditions.checkArgument(fileset.isFileset(), "Artifact %s is not fileset", fileset); From f32814d5b3184ebcb99f08e746497d77912e440d Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 14 Mar 2022 06:34:32 -0700 Subject: [PATCH 2/3] Remote: Don't upload action result if declared outputs are not created. Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry. Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread. Fixes https://github.com/bazelbuild/bazel/issues/14543. Closes #15016. PiperOrigin-RevId: 434448255 --- .../lib/remote/RemoteExecutionService.java | 132 +++++++++++------- .../build/lib/remote/UploadManifest.java | 3 +- .../remote/RemoteExecutionServiceTest.java | 21 +++ .../bazel/remote/remote_execution_test.sh | 23 +++ 4 files changed, 127 insertions(+), 52 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index c09934743d9036..f54fd5ef72f154 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -58,6 +58,7 @@ import com.google.common.annotations.VisibleForTesting; 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.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -1120,24 +1121,52 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + private Single buildUploadManifestAsync( + RemoteAction action, SpawnResult spawnResult) { + return Single.fromCallable( + () -> { + ImmutableList.Builder outputFiles = ImmutableList.builder(); + for (ActionInput outputFile : action.spawn.getOutputFiles()) { + Path outputPath = execRoot.getRelative(outputFile.getExecPath()); + if (!outputPath.exists()) { + String output; + if (outputFile instanceof Artifact) { + output = ((Artifact) outputFile).prettyPrint(); + } else { + output = outputFile.getExecPathString(); + } + throw new IOException("Expected output " + output + " was not created locally."); + } + outputFiles.add(outputPath); + } + + return UploadManifest.create( + remoteOptions, + digestUtil, + remotePathResolver, + action.actionKey, + action.action, + action.command, + outputFiles.build(), + action.spawnExecutionContext.getFileOutErr(), + spawnResult.exitCode()); + }); + } + @VisibleForTesting UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) - throws ExecException, IOException { - Collection outputFiles = - action.spawn.getOutputFiles().stream() - .map((inp) -> execRoot.getRelative(inp.getExecPath())) - .collect(ImmutableList.toImmutableList()); - - return UploadManifest.create( - remoteOptions, - digestUtil, - remotePathResolver, - action.actionKey, - action.action, - action.command, - outputFiles, - action.spawnExecutionContext.getFileOutErr(), - /* exitCode= */ 0); + throws IOException, ExecException, InterruptedException { + try { + return buildUploadManifestAsync(action, spawnResult).blockingGet(); + } catch (RuntimeException e) { + Throwable cause = e.getCause(); + if (cause != null) { + Throwables.throwIfInstanceOf(cause, IOException.class); + Throwables.throwIfInstanceOf(cause, ExecException.class); + Throwables.throwIfInstanceOf(cause, InterruptedException.class); + } + throw e; + } } /** Upload outputs of a remote action which was executed locally to remote cache. */ @@ -1149,42 +1178,43 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult) SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0, "shouldn't upload outputs of failed local action"); - try { - UploadManifest manifest = buildUploadManifest(action, spawnResult); - if (remoteOptions.remoteCacheAsync) { - Single.using( - remoteCache::retain, - remoteCache -> - manifest.uploadAsync( - action.getRemoteActionExecutionContext(), remoteCache, reporter), - RemoteCache::release) - .subscribeOn(scheduler) - .subscribe( - new SingleObserver() { - @Override - public void onSubscribe(@NonNull Disposable d) { - backgroundTaskPhaser.register(); - } - - @Override - public void onSuccess(@NonNull ActionResult actionResult) { - backgroundTaskPhaser.arriveAndDeregister(); - } - - @Override - public void onError(@NonNull Throwable e) { - backgroundTaskPhaser.arriveAndDeregister(); - reportUploadError(e); - } - }); - } else { - try (SilentCloseable c = - Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) { - manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter); - } + if (remoteOptions.remoteCacheAsync) { + Single.using( + remoteCache::retain, + remoteCache -> + buildUploadManifestAsync(action, spawnResult) + .flatMap( + manifest -> + manifest.uploadAsync( + action.getRemoteActionExecutionContext(), remoteCache, reporter)), + RemoteCache::release) + .subscribeOn(scheduler) + .subscribe( + new SingleObserver() { + @Override + public void onSubscribe(@NonNull Disposable d) { + backgroundTaskPhaser.register(); + } + + @Override + public void onSuccess(@NonNull ActionResult actionResult) { + backgroundTaskPhaser.arriveAndDeregister(); + } + + @Override + public void onError(@NonNull Throwable e) { + backgroundTaskPhaser.arriveAndDeregister(); + reportUploadError(e); + } + }); + } else { + try (SilentCloseable c = + Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) { + UploadManifest manifest = buildUploadManifest(action, spawnResult); + manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter); + } catch (IOException e) { + reportUploadError(e); } - } catch (IOException e) { - reportUploadError(e); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java index b9b391227306d4..ae7755ea3bccd0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -350,7 +350,7 @@ ActionResult getActionResult() { /** Uploads outputs and action result (if exit code is 0) to remote cache. */ public ActionResult upload( RemoteActionExecutionContext context, RemoteCache remoteCache, ExtendedEventHandler reporter) - throws IOException, InterruptedException { + throws IOException, InterruptedException, ExecException { try { return uploadAsync(context, remoteCache, reporter).blockingGet(); } catch (RuntimeException e) { @@ -358,6 +358,7 @@ public ActionResult upload( if (cause != null) { throwIfInstanceOf(cause, InterruptedException.class); throwIfInstanceOf(cause, IOException.class); + throwIfInstanceOf(cause, ExecException.class); } throw e; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index b90cc763060c69..7b30846067a92a 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1365,6 +1365,27 @@ public void uploadOutputs_firesUploadEvents() throws Exception { spawn.getResourceOwner(), "ac/" + action.getActionId())); } + @Test + public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception { + Path file = execRoot.getRelative("outputs/file"); + Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file); + RemoteExecutionService service = newRemoteExecutionService(); + Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputFile)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteAction action = service.buildRemoteAction(spawn, context); + SpawnResult spawnResult = + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(SpawnResult.Status.SUCCESS) + .setRunnerName("test") + .build(); + + service.uploadOutputs(action, spawnResult); + + // assert + assertThat(cache.getNumFindMissingDigests()).isEmpty(); + } + @Test public void uploadInputsIfNotPresent_deduplicateFindMissingBlobCalls() throws Exception { int taskCount = 100; diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 03068d27ec06ee..b70c2772ce8e84 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3499,4 +3499,27 @@ EOF [[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files" } +function test_missing_outputs_dont_upload_action_result() { + # Test that if declared outputs are not created, even the exit code of action + # is 0, we treat this as failed and don't upload action result. + # See https://github.com/bazelbuild/bazel/issues/14543. + mkdir -p a + cat > a/BUILD <& $TEST_log && fail "Should failed to build" + + remote_cas_files="$(count_remote_cas_files)" + [[ "$remote_cas_files" == 0 ]] || fail "Expected 0 remote cas entries, not $remote_cas_files" + remote_ac_files="$(count_remote_ac_files)" + [[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files" +} + run_suite "Remote execution and remote cache tests" From 722ee13106b00afeb2b31d33cfa8c2b64ff39bc7 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Thu, 17 Mar 2022 04:14:41 -0700 Subject: [PATCH 3/3] Remote: Check declared outputs when downloading outputs. An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build. This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue. Also fixes an issue introduced by #15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`. Fixes #14543. Closes #15051. PiperOrigin-RevId: 435307260 --- .../lib/remote/RemoteExecutionService.java | 41 ++++++++++++----- .../remote/RemoteExecutionServiceTest.java | 44 +++++++++++++++++-- .../lib/remote/RemoteSpawnCacheTest.java | 2 + .../lib/remote/RemoteSpawnRunnerTest.java | 1 + ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 8 +++- 5 files changed, 81 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index f54fd5ef72f154..6a5feb87901313 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1010,6 +1010,23 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re metadata = parseActionResultMetadata(action, result); } + // Check that all mandatory outputs are created. + for (ActionInput output : action.spawn.getOutputFiles()) { + if (action.spawn.isMandatoryOutput(output)) { + Path localPath = execRoot.getRelative(output.getExecPath()); + if (!metadata.files.containsKey(localPath) + && !metadata.directories.containsKey(localPath) + && !metadata.symlinks.containsKey(localPath)) { + throw new IOException( + "Invalid action cache entry " + + action.actionKey.getDigest().getHash() + + ": expected output " + + prettyPrint(output) + + " does not exist."); + } + } + } + FileOutErr outErr = action.spawnExecutionContext.getFileOutErr(); ImmutableList.Builder> downloadsBuilder = @@ -1121,23 +1138,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + private static String prettyPrint(ActionInput actionInput) { + if (actionInput instanceof Artifact) { + return ((Artifact) actionInput).prettyPrint(); + } else { + return actionInput.getExecPathString(); + } + } + private Single buildUploadManifestAsync( RemoteAction action, SpawnResult spawnResult) { return Single.fromCallable( () -> { ImmutableList.Builder outputFiles = ImmutableList.builder(); + // Check that all mandatory outputs are created. for (ActionInput outputFile : action.spawn.getOutputFiles()) { - Path outputPath = execRoot.getRelative(outputFile.getExecPath()); - if (!outputPath.exists()) { - String output; - if (outputFile instanceof Artifact) { - output = ((Artifact) outputFile).prettyPrint(); - } else { - output = outputFile.getExecPathString(); - } - throw new IOException("Expected output " + output + " was not created locally."); + Path localPath = execRoot.getRelative(outputFile.getExecPath()); + if (action.spawn.isMandatoryOutput(outputFile) && !localPath.exists()) { + throw new IOException( + "Expected output " + prettyPrint(outputFile) + " was not created locally."); } - outputFiles.add(outputPath); + outputFiles.add(localPath); } return UploadManifest.create( diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 7b30846067a92a..b885e3b53d70d4 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1099,9 +1099,21 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw ActionResult r = ActionResult.newBuilder().setExitCode(0).build(); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r)); Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1"); + // set file1 as declared output but not mandatory output Spawn spawn = - newSpawn( - ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), ImmutableSet.of(a1)); + new SimpleSpawn( + new FakeOwner("foo", "bar", "//dummy:label"), + /*arguments=*/ ImmutableList.of(), + /*environment=*/ ImmutableMap.of(), + /*executionInfo=*/ ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), + /*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*outputs=*/ ImmutableSet.of(a1), + /*mandatoryOutputs=*/ ImmutableSet.of(), + ResourceSet.ZERO); + MetadataInjector injector = mock(MetadataInjector.class); FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, injector); RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); @@ -1118,6 +1130,32 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1)); } + @Test + public void downloadOutputs_missingMandatoryOutputs_reportError() throws Exception { + // Test that an AC which misses mandatory outputs is correctly ignored. + Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents"); + ActionResult.Builder builder = ActionResult.newBuilder(); + builder.addOutputFilesBuilder().setPath("outputs/foo").setDigest(fooDigest); + RemoteActionResult result = + RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); + ImmutableSet.Builder outputs = ImmutableSet.builder(); + ImmutableList expectedOutputFiles = ImmutableList.of("outputs/foo", "outputs/bar"); + for (String outputFile : expectedOutputFiles) { + Path path = remotePathResolver.outputPathToLocalPath(outputFile); + Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path); + outputs.add(output); + } + Spawn spawn = newSpawn(ImmutableMap.of(), outputs.build()); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + IOException error = + assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); + + assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist."); + } + @Test public void uploadOutputs_uploadDirectory_works() throws Exception { // Test that uploading a directory works. @@ -1366,7 +1404,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception { } @Test - public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception { + public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception { Path file = execRoot.getRelative("outputs/file"); Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file); RemoteExecutionService service = newRemoteExecutionService(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index da10d379004642..3c97b692edb029 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -19,6 +19,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -597,6 +598,7 @@ public void testDownloadMinimal() throws Exception { when(remoteCache.downloadActionResult( any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false))) .thenReturn(CachedActionResult.remote(success)); + doReturn(null).when(cache.getRemoteExecutionService()).downloadOutputs(any(), any()); // act CacheHandle cacheHandle = cache.lookup(simpleSpawn, simplePolicy); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 68fce99846b68b..b458da16523f62 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -1197,6 +1197,7 @@ public void testDownloadTopLevel() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput)); RemoteExecutionService service = runner.getRemoteExecutionService(); doReturn(cachedActionResult).when(service).lookupCache(any()); + doReturn(null).when(service).downloadOutputs(any(), any()); Spawn spawn = newSimpleSpawn(topLevelOutput); FakeSpawnExecutionContext policy = getSpawnContext(spawn); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index 817394ff742fd4..bcb5c9c2d15f30 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -1,4 +1,4 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. +/// Copyright 2017 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. @@ -196,9 +196,12 @@ public final void setUp() throws Exception { ImmutableList.of("/bin/echo", "Hi!"), ImmutableMap.of("VARIABLE", "value"), /*executionInfo=*/ ImmutableMap.of(), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), /*inputs=*/ NestedSetBuilder.create( Order.STABLE_ORDER, ActionInputHelper.fromPath("input")), - /*outputs=*/ ImmutableSet.of( + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*outputs=*/ ImmutableSet.of( new ActionInput() { @Override public String getExecPathString() { @@ -231,6 +234,7 @@ public PathFragment getExecPath() { return PathFragment.create("bar"); } }), + /*mandatoryOutputs=*/ ImmutableSet.of(), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout");