Skip to content

Commit 82b3896

Browse files
janakdrcopybara-github
janakdr
authored and
copybara-github
committed
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
1 parent 92ce115 commit 82b3896

13 files changed

Lines changed: 173 additions & 86 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,12 @@
1616

1717
import com.google.common.collect.ImmutableList;
1818
import com.google.common.collect.ImmutableMap;
19+
import com.google.common.collect.ImmutableSet;
1920
import com.google.common.collect.Iterables;
2021
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
2122
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2223
import com.google.devtools.build.lib.util.OS;
2324
import com.google.devtools.build.lib.vfs.PathFragment;
24-
import java.util.Collection;
2525
import java.util.List;
2626
import java.util.Map;
2727
import java.util.Set;
@@ -115,7 +115,7 @@ public NestedSet<? extends ActionInput> getInputFiles() {
115115
}
116116

117117
@Override
118-
public Collection<? extends ActionInput> getOutputFiles() {
118+
public ImmutableSet<Artifact> getOutputFiles() {
119119
return action.getOutputs();
120120
}
121121

‎src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public class DelegateSpawn implements Spawn {
2929

3030
private final Spawn spawn;
3131

32-
public DelegateSpawn(Spawn spawn){
32+
public DelegateSpawn(Spawn spawn) {
3333
this.spawn = spawn;
3434
}
3535

@@ -73,6 +73,11 @@ public Collection<? extends ActionInput> getOutputFiles() {
7373
return spawn.getOutputFiles();
7474
}
7575

76+
@Override
77+
public boolean isMandatoryOutput(ActionInput output) {
78+
return spawn.isMandatoryOutput(output);
79+
}
80+
7681
@Override
7782
public ActionExecutionMetadata getResourceOwner() {
7883
return spawn.getResourceOwner();

‎src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,11 @@
2424
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2525
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
2626
import com.google.devtools.build.lib.collect.nestedset.Order;
27+
import java.util.Set;
2728
import javax.annotation.Nullable;
2829
import javax.annotation.concurrent.Immutable;
2930

30-
/**
31-
* Immutable implementation of a Spawn that does not perform any processing on the parameters.
32-
* Prefer this over all other Spawn implementations.
33-
*/
31+
/** Immutable implementation of a Spawn that does not perform any processing on the parameters. */
3432
@Immutable
3533
public final class SimpleSpawn implements Spawn {
3634
private final ActionExecutionMetadata owner;
@@ -42,6 +40,8 @@ public final class SimpleSpawn implements Spawn {
4240
private final RunfilesSupplier runfilesSupplier;
4341
private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
4442
private final ImmutableList<? extends ActionInput> outputs;
43+
// If null, all outputs are mandatory.
44+
@Nullable private final Set<? extends ActionInput> mandatoryOutputs;
4545
private final LocalResourcesSupplier localResourcesSupplier;
4646
private ResourceSet localResourcesCached;
4747

@@ -55,6 +55,7 @@ private SimpleSpawn(
5555
NestedSet<? extends ActionInput> inputs,
5656
NestedSet<? extends ActionInput> tools,
5757
ImmutableSet<? extends ActionInput> outputs,
58+
@Nullable final Set<? extends ActionInput> mandatoryOutputs,
5859
@Nullable ResourceSet localResources,
5960
@Nullable LocalResourcesSupplier localResourcesSupplier) {
6061
this.owner = Preconditions.checkNotNull(owner);
@@ -67,6 +68,7 @@ private SimpleSpawn(
6768
runfilesSupplier == null ? EmptyRunfilesSupplier.INSTANCE : runfilesSupplier;
6869
this.filesetMappings = filesetMappings;
6970
this.outputs = Preconditions.checkNotNull(outputs).asList();
71+
this.mandatoryOutputs = mandatoryOutputs;
7072
checkState(
7173
(localResourcesSupplier == null) != (localResources == null),
7274
"Exactly one must be null: %s %s",
@@ -91,6 +93,7 @@ public SimpleSpawn(
9193
NestedSet<? extends ActionInput> inputs,
9294
NestedSet<? extends ActionInput> tools,
9395
ImmutableSet<? extends ActionInput> outputs,
96+
@Nullable Set<? extends ActionInput> mandatoryOutputs,
9497
ResourceSet localResources) {
9598
this(
9699
owner,
@@ -102,6 +105,7 @@ public SimpleSpawn(
102105
inputs,
103106
tools,
104107
outputs,
108+
mandatoryOutputs,
105109
localResources,
106110
/*localResourcesSupplier=*/ null);
107111
}
@@ -117,6 +121,7 @@ public SimpleSpawn(
117121
NestedSet<? extends ActionInput> inputs,
118122
NestedSet<? extends ActionInput> tools,
119123
ImmutableSet<? extends ActionInput> outputs,
124+
@Nullable Set<? extends ActionInput> mandatoryOutputs,
120125
LocalResourcesSupplier localResourcesSupplier) {
121126
this(
122127
owner,
@@ -128,6 +133,7 @@ public SimpleSpawn(
128133
inputs,
129134
tools,
130135
outputs,
136+
mandatoryOutputs,
131137
/*localResources=*/ null,
132138
localResourcesSupplier);
133139
}
@@ -145,11 +151,12 @@ public SimpleSpawn(
145151
arguments,
146152
environment,
147153
executionInfo,
148-
null,
149-
ImmutableMap.of(),
154+
/*runfilesSupplier=*/ null,
155+
/*filesetMappings=*/ ImmutableMap.of(),
150156
inputs,
151-
NestedSetBuilder.emptySet(Order.STABLE_ORDER),
157+
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
152158
outputs,
159+
/*mandatoryOutputs=*/ null,
153160
localResourcesSupplier);
154161
}
155162

@@ -171,6 +178,7 @@ public SimpleSpawn(
171178
inputs,
172179
NestedSetBuilder.emptySet(Order.STABLE_ORDER),
173180
outputs,
181+
/*mandatoryOutputs=*/ null,
174182
resourceSet);
175183
}
176184

@@ -214,6 +222,11 @@ public ImmutableList<? extends ActionInput> getOutputFiles() {
214222
return outputs;
215223
}
216224

225+
@Override
226+
public boolean isMandatoryOutput(ActionInput output) {
227+
return mandatoryOutputs == null || mandatoryOutputs.contains(output);
228+
}
229+
217230
@Override
218231
public ActionExecutionMetadata getResourceOwner() {
219232
return owner;

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

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -101,18 +101,35 @@ public interface Spawn extends DescribableExecutionUnit {
101101
NestedSet<? extends ActionInput> getInputFiles();
102102

103103
/**
104-
* Returns the collection of files that this command must write. Callers should not mutate
105-
* the result.
104+
* Returns the collection of files that this command will write. Callers should not mutate the
105+
* result.
106106
*
107107
* <p>This is for use with remote execution, so remote execution does not have to guess what
108-
* outputs the process writes. While the order does not affect the semantics, it should be
109-
* stable so it can be cached.
108+
* outputs the process writes. While the order does not affect the semantics, it should be stable
109+
* so it can be cached.
110110
*/
111111
Collection<? extends ActionInput> getOutputFiles();
112112

113113
/**
114-
* Returns the resource owner for local fallback.
114+
* Returns true if {@code output} must be created for the action to succeed. Can be used by remote
115+
* execution implementations to mark a command as failed if it did not create an output, even if
116+
* the command itself exited with a successful exit code.
117+
*
118+
* <p>Some actions, like tests, may have optional files (like .xml files) that may be created, but
119+
* are not required, so their spawns should return false for those optional files. Note that in
120+
* general, every output in {@link ActionAnalysisMetadata#getOutputs} is checked for existence in
121+
* {@link com.google.devtools.build.lib.skyframe.SkyframeActionExecutor#checkOutputs}, so
122+
* eventually all those outputs must be produced by at least one {@code Spawn} for that action, or
123+
* locally by the action in some cases.
124+
*
125+
* <p>This method should not be overridden by any new Spawns if possible: outputs should be
126+
* mandatory.
115127
*/
128+
default boolean isMandatoryOutput(ActionInput output) {
129+
return true;
130+
}
131+
132+
/** Returns the resource owner for local fallback. */
116133
ActionExecutionMetadata getResourceOwner();
117134

118135
/**

‎src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java‎

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,11 @@ private ActionExecutionException createCommandLineException(CommandLineExpansion
341341
return new ActionExecutionException(e, this, /*catastrophe=*/ false, detailedExitCode);
342342
}
343343

344+
@VisibleForTesting
345+
public ResourceSetOrBuilder getResourceSetOrBuilder() {
346+
return resourceSetOrBuilder;
347+
}
348+
344349
/**
345350
* Returns a Spawn that is representative of the command that this Action will execute. This
346351
* function must not modify any state.
@@ -354,11 +359,6 @@ public final Spawn getSpawn() throws CommandLineExpansionException, InterruptedE
354359
return getSpawn(getInputs());
355360
}
356361

357-
@VisibleForTesting
358-
public ResourceSetOrBuilder getResourceSetOrBuilder() {
359-
return resourceSetOrBuilder;
360-
}
361-
362362
final Spawn getSpawn(NestedSet<Artifact> inputs)
363363
throws CommandLineExpansionException, InterruptedException {
364364
return new ActionSpawn(
@@ -368,20 +368,22 @@ final Spawn getSpawn(NestedSet<Artifact> inputs)
368368
/*envResolved=*/ false,
369369
inputs,
370370
/*additionalInputs=*/ ImmutableList.of(),
371-
/*filesetMappings=*/ ImmutableMap.of());
371+
/*filesetMappings=*/ ImmutableMap.of(),
372+
/*reportOutputs=*/ true);
372373
}
373374

374375
/**
375-
* Return a spawn that is representative of the command that this Action will execute in the given
376-
* client environment.
376+
* Returns a spawn that is representative of the command that this Action will execute in the
377+
* given client environment.
377378
*/
378379
public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
379380
throws CommandLineExpansionException, InterruptedException {
380381
return getSpawn(
381382
actionExecutionContext.getArtifactExpander(),
382383
actionExecutionContext.getClientEnv(),
383384
/*envResolved=*/ false,
384-
actionExecutionContext.getTopLevelFilesets());
385+
actionExecutionContext.getTopLevelFilesets(),
386+
/*reportOutputs=*/ true);
385387
}
386388

387389
/**
@@ -392,11 +394,12 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
392394
* effective environment. Otherwise they will be used as client environment to resolve the
393395
* action env.
394396
*/
395-
Spawn getSpawn(
397+
protected Spawn getSpawn(
396398
ArtifactExpander artifactExpander,
397399
Map<String, String> env,
398400
boolean envResolved,
399-
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings)
401+
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
402+
boolean reportOutputs)
400403
throws CommandLineExpansionException, InterruptedException {
401404
ExpandedCommandLines expandedCommandLines =
402405
commandLines.expand(artifactExpander, getPrimaryOutput().getExecPath(), commandLineLimits);
@@ -407,7 +410,8 @@ Spawn getSpawn(
407410
envResolved,
408411
getInputs(),
409412
expandedCommandLines.getParamFiles(),
410-
filesetMappings);
413+
filesetMappings,
414+
reportOutputs);
411415
}
412416

413417
Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException {
@@ -560,10 +564,11 @@ public ImmutableMap<String, String> getExecutionInfo() {
560564
}
561565

562566
/** A spawn instance that is tied to a specific SpawnAction. */
563-
private static class ActionSpawn extends BaseSpawn {
567+
private static final class ActionSpawn extends BaseSpawn {
564568
private final NestedSet<ActionInput> inputs;
565569
private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
566570
private final ImmutableMap<String, String> effectiveEnvironment;
571+
private final boolean reportOutputs;
567572

568573
/**
569574
* Creates an ActionSpawn with the given environment variables.
@@ -578,7 +583,8 @@ private ActionSpawn(
578583
boolean envResolved,
579584
NestedSet<Artifact> inputs,
580585
Iterable<? extends ActionInput> additionalInputs,
581-
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings)
586+
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
587+
boolean reportOutputs)
582588
throws CommandLineExpansionException {
583589
super(
584590
arguments,
@@ -598,16 +604,15 @@ private ActionSpawn(
598604
this.inputs = inputsBuilder.build();
599605
this.filesetMappings = filesetMappings;
600606

601-
/**
602-
* If the action environment is already resolved using the client environment, the given
603-
* environment variables are used as they are. Otherwise, they are used as clientEnv to
604-
* resolve the action environment variables
605-
*/
607+
// If the action environment is already resolved using the client environment, the given
608+
// environment variables are used as they are. Otherwise, they are used as clientEnv to
609+
// resolve the action environment variables.
606610
if (envResolved) {
607611
effectiveEnvironment = ImmutableMap.copyOf(env);
608612
} else {
609613
effectiveEnvironment = parent.getEffectiveEnvironment(env);
610614
}
615+
this.reportOutputs = reportOutputs;
611616
}
612617

613618
@Override
@@ -624,6 +629,11 @@ public ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMap
624629
public NestedSet<? extends ActionInput> getInputFiles() {
625630
return inputs;
626631
}
632+
633+
@Override
634+
public ImmutableSet<Artifact> getOutputFiles() {
635+
return reportOutputs ? super.getOutputFiles() : ImmutableSet.of();
636+
}
627637
}
628638

629639
/**

‎src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,8 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
324324
actionExecutionContext.getArtifactExpander(),
325325
getEffectiveEnvironment(actionExecutionContext.getClientEnv()),
326326
/*envResolved=*/ true,
327-
actionExecutionContext.getTopLevelFilesets());
327+
actionExecutionContext.getTopLevelFilesets(),
328+
/*reportOutputs=*/ true);
328329
}
329330

330331
@Override

0 commit comments

Comments
 (0)