Skip to content

Commit

Permalink
[5.1] Remote: Action should not be successful and cached if outputs w…
Browse files Browse the repository at this point in the history
…ere not created (bazelbuild#15071)

* 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

* 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 bazelbuild#14543.

Closes bazelbuild#15016.

PiperOrigin-RevId: 434448255

* 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 bazelbuild#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 bazelbuild#14543.

Closes bazelbuild#15051.

PiperOrigin-RevId: 435307260

Co-authored-by: janakr <janakr@google.com>
  • Loading branch information
coeuvre and janakdr authored Mar 18, 2022
1 parent 267142f commit f192362
Show file tree
Hide file tree
Showing 20 changed files with 364 additions and 136 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -113,7 +113,7 @@ public NestedSet<? extends ActionInput> getInputFiles() {
}

@Override
public Collection<? extends ActionInput> getOutputFiles() {
public ImmutableSet<Artifact> getOutputFiles() {
return action.getOutputs();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class DelegateSpawn implements Spawn {

private final Spawn spawn;

public DelegateSpawn(Spawn spawn){
public DelegateSpawn(Spawn spawn) {
this.spawn = spawn;
}

Expand Down Expand Up @@ -73,6 +73,11 @@ public Collection<? extends ActionInput> getOutputFiles() {
return spawn.getOutputFiles();
}

@Override
public boolean isMandatoryOutput(ActionInput output) {
return spawn.isMandatoryOutput(output);
}

@Override
public ActionExecutionMetadata getResourceOwner() {
return spawn.getResourceOwner();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -41,6 +39,8 @@ public final class SimpleSpawn implements Spawn {
private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
private final ImmutableList<? extends ActionInput> outputs;
private final ResourceSet localResources;
// If null, all outputs are mandatory.
@Nullable private final Set<? extends ActionInput> mandatoryOutputs;

public SimpleSpawn(
ActionExecutionMetadata owner,
Expand All @@ -52,6 +52,7 @@ public SimpleSpawn(
NestedSet<? extends ActionInput> inputs,
NestedSet<? extends ActionInput> tools,
ImmutableSet<? extends ActionInput> outputs,
@Nullable Set<? extends ActionInput> mandatoryOutputs,
ResourceSet localResources) {
this.owner = Preconditions.checkNotNull(owner);
this.arguments = Preconditions.checkNotNull(arguments);
Expand All @@ -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);
}

Expand All @@ -73,7 +75,7 @@ public SimpleSpawn(
ImmutableMap<String, String> executionInfo,
NestedSet<? extends ActionInput> inputs,
ImmutableSet<? extends ActionInput> outputs,
ResourceSet localResources) {
ResourceSet resourceSet) {
this(
owner,
arguments,
Expand All @@ -84,7 +86,8 @@ public SimpleSpawn(
inputs,
NestedSetBuilder.emptySet(Order.STABLE_ORDER),
outputs,
localResources);
/*mandatoryOutputs=*/ null,
resourceSet);
}

@Override
Expand Down Expand Up @@ -127,6 +130,11 @@ public ImmutableList<? extends ActionInput> getOutputFiles() {
return outputs;
}

@Override
public boolean isMandatoryOutput(ActionInput output) {
return mandatoryOutputs == null || mandatoryOutputs.contains(output);
}

@Override
public ActionExecutionMetadata getResourceOwner() {
return owner;
Expand Down
27 changes: 22 additions & 5 deletions src/main/java/com/google/devtools/build/lib/actions/Spawn.java
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,35 @@ public interface Spawn extends DescribableExecutionUnit {
NestedSet<? extends ActionInput> 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.
*
* <p>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<? extends ActionInput> 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.
*
* <p>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.
*
* <p>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();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -361,20 +366,22 @@ final Spawn getSpawn(NestedSet<Artifact> 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 {
return getSpawn(
actionExecutionContext.getArtifactExpander(),
actionExecutionContext.getClientEnv(),
/*envResolved=*/ false,
actionExecutionContext.getTopLevelFilesets());
actionExecutionContext.getTopLevelFilesets(),
/*reportOutputs=*/ true);
}

/**
Expand All @@ -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<String, String> env,
boolean envResolved,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings)
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
boolean reportOutputs)
throws CommandLineExpansionException, InterruptedException {
ExpandedCommandLines expandedCommandLines =
commandLines.expand(artifactExpander, getPrimaryOutput().getExecPath(), commandLineLimits);
Expand All @@ -399,7 +407,8 @@ Spawn getSpawn(
envResolved,
getInputs(),
expandedCommandLines.getParamFiles(),
filesetMappings);
filesetMappings,
reportOutputs);
}

Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException {
Expand Down Expand Up @@ -554,10 +563,11 @@ public Map<String, String> 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<ActionInput> inputs;
private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
private final ImmutableMap<String, String> effectiveEnvironment;
private final boolean reportOutputs;

/**
* Creates an ActionSpawn with the given environment variables.
Expand All @@ -571,7 +581,8 @@ private ActionSpawn(
boolean envResolved,
NestedSet<Artifact> inputs,
Iterable<? extends ActionInput> additionalInputs,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings)
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
boolean reportOutputs)
throws CommandLineExpansionException {
super(
arguments,
Expand All @@ -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
Expand All @@ -618,6 +628,11 @@ public ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMap
public NestedSet<? extends ActionInput> getInputFiles() {
return inputs;
}

@Override
public ImmutableSet<Artifact> getOutputFiles() {
return reportOutputs ? super.getOutputFiles() : ImmutableSet.of();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,8 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
actionExecutionContext.getArtifactExpander(),
getEffectiveEnvironment(actionExecutionContext.getClientEnv()),
/*envResolved=*/ true,
actionExecutionContext.getTopLevelFilesets());
actionExecutionContext.getTopLevelFilesets(),
/*reportOutputs=*/ true);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -54,17 +56,8 @@ public final class ExtraAction extends SpawnAction {
private final boolean createDummyOutput;
private final NestedSet<Artifact> extraActionInputs;

/**
* A long way to say (ExtraAction xa) -> xa.getShadowedAction().
*/
public static final Function<ExtraAction, Action> GET_SHADOWED_ACTION =
new Function<ExtraAction, Action>() {
@Nullable
@Override
public Action apply(@Nullable ExtraAction extraAction) {
return extraAction != null ? extraAction.getShadowedAction() : null;
}
};
e -> e != null ? e.getShadowedAction() : null;

ExtraAction(
NestedSet<Artifact> extraActionInputs,
Expand Down Expand Up @@ -153,6 +146,20 @@ public NestedSet<Artifact> 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<SpawnResult> spawnResults)
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Expand All @@ -588,18 +586,19 @@ private static Spawn createCoveragePostProcessingSpawn(
ImmutableMap.copyOf(testEnvironment),
ImmutableMap.copyOf(action.getExecutionInfo()),
action.getLcovMergerRunfilesSupplier(),
/* filesetMappings= */ ImmutableMap.of(),
/* inputs= */ NestedSetBuilder.<ActionInput>compileOrder()
/*filesetMappings=*/ ImmutableMap.of(),
/*inputs=*/ NestedSetBuilder.<ActionInput>compileOrder()
.addTransitive(action.getInputs())
.addAll(expandedCoverageDir)
.add(action.getCollectCoverageScript())
.add(action.getCoverageDirectoryTreeArtifact())
.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);
}

Expand Down
Loading

0 comments on commit f192362

Please sign in to comment.