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/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index c09934743d9036..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 @@ -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; @@ -1009,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 = @@ -1120,24 +1138,56 @@ 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 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(localPath); + } + + 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 +1199,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/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); 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..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. @@ -1365,6 +1403,27 @@ public void uploadOutputs_firesUploadEvents() throws Exception { spawn.getResourceOwner(), "ac/" + action.getActionId())); } + @Test + public void uploadOutputs_missingMandatoryOutputs_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/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"); 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"