StandaloneTestStrategy sets the full list of outputs on the test spawn
All spawn strategies already treat all normal outputs as optional. Bazel checks
at the action level whether all action outputs are created, but does not check
at the spawn level. Spawn.getOptionalOutputs is therefore unnecessary, and
removed in this change.
The only place where this was set was in StandaloneTestStrategy, which now
specifies the full set of outputs, which is now computed by TestRunnerAction.
The internal test strategy implementations are also updated in this change.
While I'm at it, also remove the use of BaseSpawn and use SimpleSpawn instead.
This may go some way towards fixing #1413 and #942.
--
PiperOrigin-RevId: 149397100
MOS_MIGRATED_REVID=149397100
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 e3143fd..901b225 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,7 +16,6 @@
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.vfs.PathFragment;
import java.util.Collection;
@@ -31,7 +30,6 @@
private final ImmutableList<String> arguments;
private final ImmutableMap<String, String> environment;
private final ImmutableMap<String, String> executionInfo;
- private final ImmutableSet<PathFragment> optionalOutputFiles;
private final RunfilesSupplier runfilesSupplier;
private final ActionExecutionMetadata action;
private final ResourceSet localResources;
@@ -42,39 +40,15 @@
Map<String, String> executionInfo,
RunfilesSupplier runfilesSupplier,
ActionExecutionMetadata action,
- ResourceSet localResources,
- Collection<PathFragment> optionalOutputFiles) {
+ ResourceSet localResources) {
this.arguments = ImmutableList.copyOf(arguments);
this.environment = ImmutableMap.copyOf(environment);
this.executionInfo = ImmutableMap.copyOf(executionInfo);
this.runfilesSupplier = runfilesSupplier;
this.action = action;
this.localResources = localResources;
- this.optionalOutputFiles = ImmutableSet.copyOf(optionalOutputFiles);
}
- /**
- * Returns a new Spawn. The caller must not modify the parameters after the call; neither will
- * this method.
- */
- public BaseSpawn(
- List<String> arguments,
- Map<String, String> environment,
- Map<String, String> executionInfo,
- RunfilesSupplier runfilesSupplier,
- ActionExecutionMetadata action,
- ResourceSet localResources) {
- this(
- arguments,
- environment,
- executionInfo,
- runfilesSupplier,
- action,
- localResources,
- ImmutableSet.<PathFragment>of());
- }
-
- /** Returns a new Spawn. */
public BaseSpawn(
List<String> arguments,
Map<String, String> environment,
@@ -170,11 +144,6 @@
}
@Override
- public Collection<PathFragment> getOptionalOutputFiles() {
- return optionalOutputFiles;
- }
-
- @Override
public ActionExecutionMetadata getResourceOwner() {
return action;
}
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 503979c..8341301 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
@@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
/**
@@ -82,11 +81,6 @@
}
@Override
- public Collection<PathFragment> getOptionalOutputFiles() {
- return spawn.getOptionalOutputFiles();
- }
-
- @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 d6fc5a3..564460e 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
@@ -17,8 +17,6 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.vfs.PathFragment;
import javax.annotation.concurrent.Immutable;
/**
@@ -36,7 +34,6 @@
private final RunfilesSupplier runfilesSupplier;
private final ImmutableList<Artifact> filesetManifests;
private final ImmutableList<? extends ActionInput> outputs;
- private final ImmutableSet<PathFragment> optionalOutputFiles;
private final ResourceSet localResources;
public SimpleSpawn(
@@ -49,7 +46,6 @@
ImmutableList<? extends ActionInput> tools,
ImmutableList<Artifact> filesetManifests,
ImmutableList<? extends ActionInput> outputs,
- ImmutableSet<PathFragment> optionalOutputFiles,
ResourceSet localResources) {
this.owner = Preconditions.checkNotNull(owner);
this.arguments = Preconditions.checkNotNull(arguments);
@@ -61,7 +57,6 @@
runfilesSupplier == null ? EmptyRunfilesSupplier.INSTANCE : runfilesSupplier;
this.filesetManifests = Preconditions.checkNotNull(filesetManifests);
this.outputs = Preconditions.checkNotNull(outputs);
- this.optionalOutputFiles = Preconditions.checkNotNull(optionalOutputFiles);
this.localResources = Preconditions.checkNotNull(localResources);
}
@@ -83,7 +78,6 @@
ImmutableList.<Artifact>of(),
ImmutableList.<Artifact>of(),
outputs,
- ImmutableSet.<PathFragment>of(),
localResources);
}
@@ -138,11 +132,6 @@
}
@Override
- public ImmutableSet<PathFragment> getOptionalOutputFiles() {
- return optionalOutputFiles;
- }
-
- @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 26f636b..01e9010 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
@@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
/**
@@ -106,12 +105,6 @@
Collection<? extends ActionInput> getOutputFiles();
/**
- * Instructs the spawn strategy to try to fetch these optional output files in addition to the
- * usual output artifacts. The PathFragments should be relative to the exec root.
- */
- Collection<PathFragment> getOptionalOutputFiles();
-
- /**
* Returns the resource owner for local fallback.
*/
ActionExecutionMetadata getResourceOwner();
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 7e862d6..831183a 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
@@ -16,13 +16,13 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
-import com.google.devtools.build.lib.actions.BaseSpawn;
+import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionStrategy;
import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.TestExecException;
@@ -112,15 +112,20 @@
info.putAll(action.getTestProperties().getExecutionInfo());
Spawn spawn =
- new BaseSpawn(
+ new SimpleSpawn(
+ action,
getArgs(COLLECT_COVERAGE, action),
- env,
- info,
+ ImmutableMap.copyOf(env),
+ ImmutableMap.copyOf(info),
new RunfilesSupplierImpl(
runfilesDir.asFragment(), action.getExecutionSettings().getRunfiles()),
- action,
- action.getTestProperties().getLocalResourceUsage(executionOptions.usingLocalTestJobs()),
- ImmutableSet.of(resolvedPaths.getXmlOutputPath().relativeTo(execRoot)));
+ /*inputs=*/ImmutableList.copyOf(action.getInputs()),
+ /*tools=*/ImmutableList.<Artifact>of(),
+ /*filesetManifests=*/ImmutableList.<Artifact>of(),
+ ImmutableList.copyOf(action.getSpawnOutputs()),
+ action
+ .getTestProperties()
+ .getLocalResourceUsage(executionOptions.usingLocalTestJobs()));
Executor executor = actionExecutionContext.getExecutor();
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index 1a31d5c..9a99031 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.exec;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.io.ByteStreams;
@@ -158,7 +159,7 @@
* @return the command line as string list.
* @throws ExecException
*/
- protected List<String> getArgs(String coverageScript, TestRunnerAction testAction)
+ protected ImmutableList<String> getArgs(String coverageScript, TestRunnerAction testAction)
throws ExecException {
List<String> args = Lists.newArrayList();
// TODO(ulfjack): This is incorrect for remote execution, where we need to consider the target
@@ -187,7 +188,7 @@
// Execute the test using the alias in the runfiles tree, as mandated by the Test Encyclopedia.
args.add(execSettings.getExecutable().getRootRelativePath().getCallablePathString());
args.addAll(execSettings.getArgs());
- return args;
+ return ImmutableList.copyOf(args);
}
private static void addRunUnderArgs(TestRunnerAction testAction, List<String> args) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
index ac9ba6a..4003cd1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
@@ -20,6 +20,8 @@
import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionException;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ExecException;
@@ -42,7 +44,9 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.util.ArrayList;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.logging.Level;
import javax.annotation.Nullable;
@@ -188,6 +192,29 @@
return true;
}
+ public List<ActionInput> getSpawnOutputs() {
+ final List<ActionInput> outputs = new ArrayList<>();
+ outputs.add(ActionInputHelper.fromPath(getXmlOutputPath()));
+ outputs.add(ActionInputHelper.fromPath(getExitSafeFile()));
+ if (isSharded()) {
+ outputs.add(ActionInputHelper.fromPath(getTestShard()));
+ }
+ outputs.add(ActionInputHelper.fromPath(getTestWarningsPath()));
+ outputs.add(ActionInputHelper.fromPath(getSplitLogsPath()));
+ outputs.add(ActionInputHelper.fromPath(getUnusedRunfilesLogPath()));
+ outputs.add(ActionInputHelper.fromPath(getInfrastructureFailureFile()));
+ outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsZipPath()));
+ outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsManifestPath()));
+ outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsAnnotationsPath()));
+ if (isCoverageMode()) {
+ outputs.add(getCoverageData());
+ if (isMicroCoverageMode()) {
+ outputs.add(getMicroCoverageData());
+ }
+ }
+ return outputs;
+ }
+
@Override
protected String computeKey() {
Fingerprint f = new Fingerprint();
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
index 2a4c7f7..9776210 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -52,10 +52,6 @@
public static ImmutableSet<PathFragment> getOutputFiles(Spawn spawn) {
Builder<PathFragment> outputFiles = ImmutableSet.builder();
- for (PathFragment optionalOutput : spawn.getOptionalOutputFiles()) {
- Preconditions.checkArgument(!optionalOutput.isAbsolute());
- outputFiles.add(optionalOutput);
- }
for (ActionInput output : spawn.getOutputFiles()) {
outputFiles.add(new PathFragment(output.getExecPathString()));
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/BaseSpawnTest.java b/src/test/java/com/google/devtools/build/lib/actions/BaseSpawnTest.java
index 34deb6e..7f339dc 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/BaseSpawnTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/BaseSpawnTest.java
@@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -76,7 +75,6 @@
ImmutableMap.<String, String>of(),
runfilesSupplier,
null,
- null,
- ImmutableSet.<PathFragment>of());
+ null);
}
}
