Add a method, Spawn#isOutputMandatory, to indicate whether a Spawn mu…

…st 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

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

@@ -16,12 +16,12 @@
 
 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.util.OS;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -115,7 +115,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

@@ -24,13 +24,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;
@@ -42,6 +40,8 @@ public final class SimpleSpawn implements Spawn {
   private final RunfilesSupplier runfilesSupplier;
   private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
   private final ImmutableList<? extends ActionInput> outputs;
+  // If null, all outputs are mandatory.
+  @Nullable private final Set<? extends ActionInput> mandatoryOutputs;
   private final LocalResourcesSupplier localResourcesSupplier;
   private ResourceSet localResourcesCached;
 
@@ -55,6 +55,7 @@ private SimpleSpawn(
       NestedSet<? extends ActionInput> inputs,
       NestedSet<? extends ActionInput> tools,
       ImmutableSet<? extends ActionInput> outputs,
+      @Nullable final Set<? extends ActionInput> mandatoryOutputs,
       @Nullable ResourceSet localResources,
       @Nullable LocalResourcesSupplier localResourcesSupplier) {
     this.owner = Preconditions.checkNotNull(owner);
@@ -67,6 +68,7 @@ private SimpleSpawn(
         runfilesSupplier == null ? EmptyRunfilesSupplier.INSTANCE : runfilesSupplier;
     this.filesetMappings = filesetMappings;
     this.outputs = Preconditions.checkNotNull(outputs).asList();
+    this.mandatoryOutputs = mandatoryOutputs;
     checkState(
         (localResourcesSupplier == null) != (localResources == null),
         "Exactly one must be null: %s %s",
@@ -91,6 +93,7 @@ public SimpleSpawn(
       NestedSet<? extends ActionInput> inputs,
       NestedSet<? extends ActionInput> tools,
       ImmutableSet<? extends ActionInput> outputs,
+      @Nullable Set<? extends ActionInput> mandatoryOutputs,
       ResourceSet localResources) {
     this(
         owner,
@@ -102,6 +105,7 @@ public SimpleSpawn(
         inputs,
         tools,
         outputs,
+        mandatoryOutputs,
         localResources,
         /*localResourcesSupplier=*/ null);
   }
@@ -117,6 +121,7 @@ public SimpleSpawn(
       NestedSet<? extends ActionInput> inputs,
       NestedSet<? extends ActionInput> tools,
       ImmutableSet<? extends ActionInput> outputs,
+      @Nullable Set<? extends ActionInput> mandatoryOutputs,
       LocalResourcesSupplier localResourcesSupplier) {
     this(
         owner,
@@ -128,6 +133,7 @@ public SimpleSpawn(
         inputs,
         tools,
         outputs,
+        mandatoryOutputs,
         /*localResources=*/ null,
         localResourcesSupplier);
   }
@@ -145,11 +151,12 @@ public SimpleSpawn(
         arguments,
         environment,
         executionInfo,
-        null,
-        ImmutableMap.of(),
+        /*runfilesSupplier=*/ null,
+        /*filesetMappings=*/ ImmutableMap.of(),
         inputs,
-        NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+        /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
         outputs,
+        /*mandatoryOutputs=*/ null,
         localResourcesSupplier);
   }
 
@@ -171,6 +178,7 @@ public SimpleSpawn(
         inputs,
         NestedSetBuilder.emptySet(Order.STABLE_ORDER),
         outputs,
+        /*mandatoryOutputs=*/ null,
         resourceSet);
   }
 
@@ -214,6 +222,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

@@ -101,18 +101,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

@@ -341,6 +341,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.
@@ -354,11 +359,6 @@ public final Spawn getSpawn() throws CommandLineExpansionException, InterruptedE
     return getSpawn(getInputs());
   }
 
-  @VisibleForTesting
-  public ResourceSetOrBuilder getResourceSetOrBuilder() {
-    return resourceSetOrBuilder;
-  }
-
   final Spawn getSpawn(NestedSet<Artifact> inputs)
       throws CommandLineExpansionException, InterruptedException {
     return new ActionSpawn(
@@ -368,20 +368,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);
   }
 
   /**
@@ -392,11 +394,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);
@@ -407,7 +410,8 @@ Spawn getSpawn(
         envResolved,
         getInputs(),
         expandedCommandLines.getParamFiles(),
-        filesetMappings);
+        filesetMappings,
+        reportOutputs);
   }
 
   Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException {
@@ -560,10 +564,11 @@ public ImmutableMap<String, String> getExecutionInfo() {
   }
 
   /** A spawn instance that is tied to a specific SpawnAction. */
-  private static class ActionSpawn extends BaseSpawn {
+  private static 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.
@@ -578,7 +583,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,
@@ -598,16 +604,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 = parent.getEffectiveEnvironment(env);
       }
+      this.reportOutputs = reportOutputs;
     }
 
     @Override
@@ -624,6 +629,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