[5.1] Remote: Action should not be successful and cached if outputs were not created

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<? extends ActionInput> getInputFiles() {
  }
 
  @Override
- public Collection<? extends ActionInput> getOutputFiles() {
+ public ImmutableSet<Artifact> getOutputFiles() {
  return action.getOutputs();
  }
 

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<? extends ActionInput> getOutputFiles() {
  return spawn.getOutputFiles();
  }
 
+ @Override
+ public boolean isMandatoryOutput(ActionInput output) {
+ return spawn.isMandatoryOutput(output);
+ }
+
  @Override
  public ActionExecutionMetadata getResourceOwner() {
  return spawn.getResourceOwner();

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<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,
@@ -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);
@@ -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<String, String> executionInfo,
  NestedSet<? extends ActionInput> inputs,
  ImmutableSet<? extends ActionInput> 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<? extends ActionInput> getOutputFiles() {
  return outputs;
  }
 
+ @Override
+ public boolean isMandatoryOutput(ActionInput output) {
+ return mandatoryOutputs == null || mandatoryOutputs.contains(output);
+ }
+
  @Override
  public ActionExecutionMetadata getResourceOwner() {
  return owner;

src/main/java/com/google/devtools/build/lib/actions/Spawn.java

@@ -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();
 
  /**

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,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);
  }
 
  /**
@@ -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);
@@ -399,7 +407,8 @@ Spawn getSpawn(
  envResolved,
  getInputs(),
  expandedCommandLines.getParamFiles(),
- filesetMappings);
+ filesetMappings,
+ reportOutputs);
  }
 
  Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException {
@@ -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.
@@ -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,
@@ -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<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMap
  public NestedSet<? extends ActionInput> getInputFiles() {
  return inputs;
  }
+
+ @Override
+ public ImmutableSet<Artifact> getOutputFiles() {
+ return reportOutputs ? super.getOutputFiles() : ImmutableSet.of();
+ }
  }
 
  /**

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

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<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,
@@ -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)
@@ -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;
  }

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,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);
  }