From 82b3896baaf8e982d44e1d233b041d57e74da192 Mon Sep 17 00:00:00 2001 From: janakr Date: Tue, 1 Feb 2022 07:12:10 -0800 Subject: [PATCH] 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 --- .../devtools/build/lib/actions/BaseSpawn.java | 4 +- .../build/lib/actions/DelegateSpawn.java | 7 ++- .../build/lib/actions/SimpleSpawn.java | 27 +++++--- .../devtools/build/lib/actions/Spawn.java | 27 ++++++-- .../lib/analysis/actions/SpawnAction.java | 48 ++++++++------ .../lib/analysis/actions/StarlarkAction.java | 3 +- .../build/lib/analysis/extra/ExtraAction.java | 31 +++++---- .../lib/exec/StandaloneTestStrategy.java | 15 +++-- .../build/lib/rules/cpp/CppCompileAction.java | 63 ++++++++++++------- .../lib/rules/java/JavaCompileAction.java | 16 ++++- .../rules/java/JavaCompileActionBuilder.java | 3 +- .../lib/analysis/actions/SpawnActionTest.java | 3 +- .../build/lib/exec/util/SpawnBuilder.java | 12 +++- 13 files changed, 173 insertions(+), 86 deletions(-) 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 9129dd631b38a2..8139803d8d5d12 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,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 getInputFiles() { } @Override - public Collection getOutputFiles() { + public ImmutableSet getOutputFiles() { return action.getOutputs(); } 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 a8095183100358..1cc002e666bd78 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 @@ -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 getOutputFiles() { return spawn.getOutputFiles(); } + @Override + public boolean isMandatoryOutput(ActionInput output) { + return spawn.isMandatoryOutput(output); + } + @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 74381db958acd8..b683daa0b10d4d 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 @@ -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> filesetMappings; private final ImmutableList outputs; + // If null, all outputs are mandatory. + @Nullable private final Set mandatoryOutputs; private final LocalResourcesSupplier localResourcesSupplier; private ResourceSet localResourcesCached; @@ -55,6 +55,7 @@ private SimpleSpawn( NestedSet inputs, NestedSet tools, ImmutableSet outputs, + @Nullable final Set 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 inputs, NestedSet tools, ImmutableSet outputs, + @Nullable Set 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 inputs, NestedSet tools, ImmutableSet outputs, + @Nullable Set 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 getOutputFiles() { return outputs; } + @Override + public boolean isMandatoryOutput(ActionInput output) { + return mandatoryOutputs == null || mandatoryOutputs.contains(output); + } + @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 07b7fc701105bb..20fcdf3ccc57f9 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 @@ -101,18 +101,35 @@ public interface Spawn extends DescribableExecutionUnit { NestedSet 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. * *

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 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. + * + *

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. + * + *

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(); /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 5e8042a3261fda..a7041752c41ae6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/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 inputs) throws CommandLineExpansionException, InterruptedException { return new ActionSpawn( @@ -368,12 +368,13 @@ final Spawn getSpawn(NestedSet 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 { @@ -381,7 +382,8 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext) 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 env, boolean envResolved, - Map> filesetMappings) + Map> 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 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 inputs; private final Map> filesetMappings; private final ImmutableMap effectiveEnvironment; + private final boolean reportOutputs; /** * Creates an ActionSpawn with the given environment variables. @@ -578,7 +583,8 @@ private ActionSpawn( boolean envResolved, NestedSet inputs, Iterable additionalInputs, - Map> filesetMappings) + Map> 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> getFilesetMap public NestedSet getInputFiles() { return inputs; } + + @Override + public ImmutableSet getOutputFiles() { + return reportOutputs ? super.getOutputFiles() : ImmutableSet.of(); + } } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java index 5c45c24b656f3b..5371908a2837ba 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java +++ b/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 diff --git a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java index 342f3a4ce5b7ad..f6e6d061e4e154 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java +++ b/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 extraActionInputs; - /** - * A long way to say (ExtraAction xa) -> xa.getShadowedAction(). - */ public static final Function GET_SHADOWED_ACTION = - new Function() { - @Nullable - @Override - public Action apply(@Nullable ExtraAction extraAction) { - return extraAction != null ? extraAction.getShadowedAction() : null; - } - }; + e -> e != null ? e.getShadowedAction() : null; ExtraAction( NestedSet extraActionInputs, @@ -153,6 +146,20 @@ public NestedSet 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 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; } 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 3faf39eb37cab6..b1c93425577141 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 @@ -151,6 +151,7 @@ public TestRunnerSpawn createTestRunnerSpawn( ? action.getTools() : NestedSetBuilder.emptySet(Order.STABLE_ORDER), createSpawnOutputs(action), + /*mandatoryOutputs=*/ ImmutableSet.of(), localResourcesSupplier); 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,8 +586,8 @@ private static Spawn createCoveragePostProcessingSpawn( ImmutableMap.copyOf(testEnvironment), action.getExecutionInfo(), action.getLcovMergerRunfilesSupplier(), - /* filesetMappings= */ ImmutableMap.of(), - /* inputs= */ NestedSetBuilder.compileOrder() + /*filesetMappings=*/ ImmutableMap.of(), + /*inputs=*/ NestedSetBuilder.compileOrder() .addTransitive(action.getInputs()) .addAll(expandedCoverageDir) .add(action.getCollectCoverageScript()) @@ -597,9 +595,10 @@ private static Spawn createCoveragePostProcessingSpawn( .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); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 652eee1b520a54..1288e96f5ab419 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -63,6 +63,7 @@ import com.google.devtools.build.lib.actions.extra.EnvironmentVariable; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.analysis.starlark.Args; +import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -145,6 +146,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable private final boolean usePic; private final boolean useHeaderModules; final boolean needsIncludeValidation; + private final boolean hasCoverageArtifact; private final CcCompilationContext ccCompilationContext; private final ImmutableList builtinIncludeFiles; @@ -308,6 +310,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable .getParentDirectory() .getChild(outputFile.getFilename() + ".params"); } + this.hasCoverageArtifact = gcnoFile != null; } private static ImmutableSet collectOutputs( @@ -318,6 +321,10 @@ private static ImmutableSet collectOutputs( @Nullable Artifact ltoIndexingFile, ImmutableList additionalOutputs) { ImmutableSet.Builder outputs = ImmutableSet.builder(); + // gcnoFile comes first because easy access to it is occasionally useful. + if (gcnoFile != null) { + outputs.add(gcnoFile); + } outputs.addAll(additionalOutputs); if (outputFile != null) { outputs.add(outputFile); @@ -325,9 +332,6 @@ private static ImmutableSet collectOutputs( if (dotdFile != null) { outputs.add(dotdFile); } - if (gcnoFile != null) { - outputs.add(gcnoFile); - } if (dwoFile != null) { outputs.add(dwoFile); } @@ -1534,8 +1538,15 @@ protected Spawn createSpawn(Path execRoot, Map clientEnv) ImmutableList.copyOf(getArguments()), getEffectiveEnvironment(clientEnv), executionInfo.buildOrThrow(), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), inputs, + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), getOutputs(), + // In coverage mode, .gcno file not produced for an empty translation unit. + /*mandatoryOutputs=*/ hasCoverageArtifact + ? ImmutableSet.copyOf(getOutputs().asList().subList(1, getOutputs().size())) + : null, () -> estimateResourceConsumptionLocal( enabledCppCompileResourcesEstimation(), @@ -1625,27 +1636,35 @@ public List getPermittedSystemIncludePrefixes(Path execRoot) { } /** - * Gcc only creates ".gcno" files if the compilation unit is non-empty. To ensure that the set of + * Gcc only creates a ".gcno" file if the compilation unit is non-empty. To ensure that the set of * outputs for a CppCompileAction remains consistent and doesn't vary dynamically depending on the - * _contents_ of the input files, we create empty ".gcno" files if gcc didn't create them. + * _contents_ of the input files, we create an empty ".gcno" file if gcc didn't create it. */ - private void ensureCoverageNotesFilesExist(ActionExecutionContext actionExecutionContext) + private void ensureCoverageNotesFileExists(ActionExecutionContext actionExecutionContext) throws ActionExecutionException { - for (Artifact output : getOutputs()) { - if (output.isFileType(CppFileTypes.COVERAGE_NOTES)) { // ".gcno" - Path outputPath = actionExecutionContext.getInputPath(output); - if (outputPath.exists()) { - continue; - } - try { - FileSystemUtils.createEmptyFile(outputPath); - } catch (IOException e) { - String message = "Error creating file '" + outputPath + "': " + e.getMessage(); - DetailedExitCode code = - createDetailedExitCode(message, Code.COVERAGE_NOTES_CREATION_FAILURE); - throw new ActionExecutionException(message, e, this, false, code); - } - } + if (!hasCoverageArtifact) { + return; + } + Artifact gcnoArtifact = getOutputs().iterator().next(); + if (!gcnoArtifact.isFileType(CppFileTypes.COVERAGE_NOTES)) { + BugReport.sendBugReport( + new IllegalStateException( + "In coverage mode but gcno artifact is not first output: " + + gcnoArtifact + + ", " + + this)); + return; + } + Path outputPath = actionExecutionContext.getInputPath(gcnoArtifact); + if (outputPath.exists()) { + return; + } + try { + FileSystemUtils.createEmptyFile(outputPath); + } catch (IOException e) { + String message = "Error creating file '" + outputPath + "': " + e.getMessage(); + DetailedExitCode code = createDetailedExitCode(message, Code.COVERAGE_NOTES_CREATION_FAILURE); + throw new ActionExecutionException(message, e, this, false, code); } } @@ -1857,7 +1876,7 @@ public ActionContinuationOrResult execute() copyTempOutErrToActionOutErr(); - ensureCoverageNotesFilesExist(actionExecutionContext); + ensureCoverageNotesFileExists(actionExecutionContext); CppIncludeExtractionContext scanningContext = actionExecutionContext.getContext(CppIncludeExtractionContext.class); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 240b09ed411aa9..2c748cb649f156 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -322,7 +322,8 @@ JavaSpawn getReducedSpawn( expandedCommandLines, getEffectiveEnvironment(actionExecutionContext.getClientEnv()), getExecutionInfo(), - inputs); + inputs, + /*onlyMandatoryOutput=*/ fallback ? null : outputDepsProto); } private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) @@ -349,7 +350,8 @@ private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) // .jdeps files, which have config prefixes in output paths, which compromise caching // possible by stripping prefixes on the executor. .addTransitive(dependencyArtifacts) - .build()); + .build(), + /*onlyMandatoryOutput=*/ null); } @Override @@ -483,12 +485,14 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont private final class JavaSpawn extends BaseSpawn { private final NestedSet inputs; + private final Artifact onlyMandatoryOutput; JavaSpawn( CommandLines.ExpandedCommandLines expandedCommandLines, Map environment, Map executionInfo, - NestedSet inputs) { + NestedSet inputs, + @Nullable Artifact onlyMandatoryOutput) { super( ImmutableList.copyOf(expandedCommandLines.arguments()), environment, @@ -496,6 +500,7 @@ private final class JavaSpawn extends BaseSpawn { EmptyRunfilesSupplier.INSTANCE, JavaCompileAction.this, LOCAL_RESOURCES); + this.onlyMandatoryOutput = onlyMandatoryOutput; this.inputs = NestedSetBuilder.fromNestedSet(inputs) .addAll(expandedCommandLines.getParamFiles()) @@ -506,6 +511,11 @@ private final class JavaSpawn extends BaseSpawn { public NestedSet getInputFiles() { return inputs; } + + @Override + public boolean isMandatoryOutput(ActionInput output) { + return onlyMandatoryOutput == null || onlyMandatoryOutput.equals(output); + } } @VisibleForTesting diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index f968535403496c..64ec799a725e1d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.util.StringCanonicalizer; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collections; +import java.util.Objects; import java.util.Optional; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -286,7 +287,7 @@ private ImmutableSet allOutputs() { .add(outputs.output()) .addAll(additionalOutputs); Stream.of(outputs.depsProto(), outputs.nativeHeader(), genSourceOutput, manifestOutput) - .filter(x -> x != null) + .filter(Objects::nonNull) .forEachOrdered(result::add); return result.build(); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java index 3500634000c4b2..30261ce8db4946 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java @@ -218,7 +218,8 @@ public void testBuilderWithJavaExecutableAndParameterFile2() throws Exception { (artifact, outputs) -> outputs.add(artifact), ImmutableMap.of(), /*envResolved=*/ false, - ImmutableMap.of()); + ImmutableMap.of(), + /*reportOutputs=*/ true); String paramFileName = output.getExecPathString() + "-0.params"; // The spawn's primary arguments should reference the param file assertThat(spawn.getArguments()) diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java index 60ec4d08f49fb2..a3c4761dba7f09 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java @@ -35,11 +35,10 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import javax.annotation.Nullable; -/** - * Builder class to create {@link Spawn} instances for testing. - */ +/** Builder class to create {@link Spawn} instances for testing. */ public final class SpawnBuilder { private String mnemonic = "Mnemonic"; private String progressMessage = "progress message"; @@ -52,6 +51,7 @@ public final class SpawnBuilder { private ImmutableMap execProperties = ImmutableMap.of(); private final NestedSetBuilder inputs = NestedSetBuilder.stableOrder(); private final List outputs = new ArrayList<>(); + @Nullable private Set mandatoryOutputs; private final Map> filesetMappings = new HashMap<>(); private final NestedSetBuilder tools = NestedSetBuilder.stableOrder(); @@ -77,6 +77,7 @@ public Spawn build() { inputs.build(), tools.build(), ImmutableSet.copyOf(outputs), + mandatoryOutputs, resourceSet); } @@ -160,6 +161,11 @@ public SpawnBuilder withOutputs(String... names) { return this; } + public SpawnBuilder withMandatoryOutputs(@Nullable Set mandatoryOutputs) { + this.mandatoryOutputs = mandatoryOutputs; + return this; + } + public SpawnBuilder withFilesetMapping( Artifact fileset, ImmutableList mappings) { Preconditions.checkArgument(fileset.isFileset(), "Artifact %s is not fileset", fileset);