diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index 0a91ee6b06fd0c..c637e8a397778b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -309,10 +309,8 @@ public Artifact getPrimaryInput() { } @Override - public Artifact getPrimaryOutput() { - // Default behavior is to return the first output artifact. - // Use the method rather than field in case of overriding in subclasses. - return Iterables.getFirst(getOutputs(), null); + public final Artifact getPrimaryOutput() { + return Iterables.get(outputs, 0); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java index 8a513508b917a1..b7df6037af557d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java @@ -186,7 +186,8 @@ NestedSet getInputFilesForExtraAction(ActionExecutionContext actionExe Artifact getPrimaryInput(); /** - * Returns the "primary" output of this action. + * Returns the "primary" output of this action, which is the same as the first artifact in {@link + * #getOutputs}. * *

For example, the linked library would be the primary output of a LinkAction. * diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java index 4d21092df79b82..df9172da59b4fd 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java @@ -48,9 +48,9 @@ default String getProgressMessage(RepositoryMapping mainRepositoryMapping) { } /** - * Returns a human-readable description of the inputs to {@link #getKey(ActionKeyContext)}. Used - * in the output from '--explain', and in error messages for '--check_up_to_date' and - * '--check_tests_up_to_date'. May return null, meaning no extra information is available. + * Returns a human-readable description of the inputs to {@link #getKey}. Used in the output from + * '--explain', and in error messages for '--check_up_to_date' and '--check_tests_up_to_date'. May + * return null, meaning no extra information is available. * *

If the return value is non-null, for consistency it should be a multiline message of the * form: 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 c6cf7f5162e74d..6d42148f5e1789 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 @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis.actions; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps; import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME; @@ -103,7 +104,7 @@ public interface ExtraActionInfoSupplier { private static final String GUID = "ebd6fce3-093e-45ee-adb6-bf513b602f0d"; - public static final Interner> executionInfoInterner = + private static final Interner> executionInfoInterner = BlazeInterners.newWeakInterner(); private final CommandLines commandLines; @@ -118,7 +119,6 @@ public interface ExtraActionInfoSupplier { private final ImmutableMap executionInfo; private final ExtraActionInfoSupplier extraActionInfoSupplier; - private final Artifact primaryOutput; private final Consumer>> resultConsumer; private final boolean stripOutputPaths; @@ -132,7 +132,6 @@ public interface ExtraActionInfoSupplier { * @param inputs the set of all files potentially read by this action; must not be subsequently * modified. * @param outputs the set of all files written by this action; must not be subsequently modified. - * @param primaryOutput the primary output of this action * @param resourceSetOrBuilder the resources consumed by executing this Action. * @param env the action environment * @param commandLines the command lines to execute. This includes the main argv vector and any @@ -148,7 +147,6 @@ public SpawnAction( NestedSet tools, NestedSet inputs, Iterable outputs, - Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -161,7 +159,6 @@ public SpawnAction( tools, inputs, outputs, - primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -188,7 +185,6 @@ public SpawnAction( * @param inputs the set of all files potentially read by this action; must not be subsequently * modified * @param outputs the set of all files written by this action; must not be subsequently modified. - * @param primaryOutput the primary output of this action * @param resourceSetOrBuilder the resources consumed by executing this Action. * @param env the action's environment * @param executionInfo out-of-band information for scheduling the spawn @@ -206,7 +202,6 @@ public SpawnAction( NestedSet tools, NestedSet inputs, Iterable outputs, - Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -221,7 +216,6 @@ public SpawnAction( Consumer>> resultConsumer, boolean stripOutputPaths) { super(owner, tools, inputs, runfilesSupplier, outputs, env); - this.primaryOutput = primaryOutput; this.resourceSetOrBuilder = resourceSetOrBuilder; this.executionInfo = executionInfo.isEmpty() @@ -238,11 +232,6 @@ public SpawnAction( this.stripOutputPaths = stripOutputPaths; } - @Override - public Artifact getPrimaryOutput() { - return primaryOutput; - } - @VisibleForTesting public CommandLines getCommandLines() { return commandLines; @@ -261,12 +250,10 @@ public List getArguments() throws CommandLineExpansionException, Interru } @Override - public Sequence getStarlarkArgs() throws EvalException { + public Sequence getStarlarkArgs() { ImmutableList.Builder result = ImmutableList.builder(); ImmutableSet directoryInputs = - getInputs().toList().stream() - .filter(artifact -> artifact.isDirectory()) - .collect(ImmutableSet.toImmutableSet()); + getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet()); for (CommandLineAndParamFileInfo commandLine : commandLines.getCommandLines()) { result.add(Args.forRegisteredAction(commandLine, directoryInputs)); @@ -536,7 +523,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont *

Subclasses of SpawnAction may override this in order to provide action-specific behaviour. * This can be necessary, for example, when the action discovers inputs. */ - protected SpawnInfo getExtraActionSpawnInfo() + private SpawnInfo getExtraActionSpawnInfo() throws CommandLineExpansionException, InterruptedException { SpawnInfo.Builder info = SpawnInfo.newBuilder(); Spawn spawn = getSpawnForExtraAction(); @@ -601,7 +588,7 @@ private ActionSpawn( throws CommandLineExpansionException { super( arguments, - ImmutableMap.of(), + ImmutableMap.of(), parent.getExecutionInfo(), parent.getRunfilesSupplier(), parent, @@ -805,8 +792,7 @@ private SpawnAction buildSpawnAction( owner, tools, inputsAndTools, - ImmutableList.copyOf(outputs), - outputs.get(0), + ImmutableSet.copyOf(outputs), resourceSetOrBuilder, commandLines, commandLineLimits, @@ -826,8 +812,7 @@ protected SpawnAction createSpawnAction( ActionOwner owner, NestedSet tools, NestedSet inputsAndTools, - ImmutableList outputs, - Artifact primaryOutput, + ImmutableSet outputs, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -843,7 +828,6 @@ protected SpawnAction createSpawnAction( tools, inputsAndTools, outputs, - primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -949,13 +933,6 @@ public Builder addOutputs(Iterable artifacts) { return this; } - /** - * Checks whether the action produces any outputs - */ - public boolean hasOutputs() { - return !outputs.isEmpty(); - } - /** * Sets RecourceSet for builder. If ResourceSetBuilder set, then ResourceSetBuilder will * override setResources. 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 69eb744b5ae8a3..e44d64bbd0b4f7 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 @@ -18,6 +18,8 @@ import com.google.common.annotations.VisibleForTesting; 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.common.collect.Maps; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Action; @@ -81,7 +83,6 @@ public final class StarlarkAction extends SpawnAction implements ActionCacheAwar * @param inputs the set of all files potentially read by this action; must not be subsequently * modified * @param outputs the set of all files written by this action; must not be subsequently modified. - * @param primaryOutput the primary output of this action * @param resourceSetOrBuilder the resources consumed by executing this Action * @param commandLines the command lines to execute. This includes the main argv vector and any * param file-backed command lines. @@ -103,7 +104,6 @@ private StarlarkAction( NestedSet tools, NestedSet inputs, Iterable outputs, - Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -123,7 +123,6 @@ private StarlarkAction( ? createInputs(shadowedAction.get().getInputs(), inputs) : inputs, outputs, - primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -376,8 +375,7 @@ protected SpawnAction createSpawnAction( ActionOwner owner, NestedSet tools, NestedSet inputsAndTools, - ImmutableList outputs, - Artifact primaryOutput, + ImmutableSet outputs, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -403,7 +401,6 @@ protected SpawnAction createSpawnAction( tools, inputsAndTools, outputs, - primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -415,7 +412,7 @@ protected SpawnAction createSpawnAction( mnemonic, unusedInputsList, shadowedAction, - stripOutputPaths(mnemonic, inputsAndTools, primaryOutput, configuration)); + stripOutputPaths(mnemonic, inputsAndTools, outputs, configuration)); } /** @@ -435,7 +432,7 @@ protected SpawnAction createSpawnAction( private static boolean stripOutputPaths( String mnemonic, NestedSet inputs, - Artifact primaryOutput, + ImmutableSet outputs, BuildConfigurationValue configuration) { ImmutableList qualifyingMnemonics = ImmutableList.of( @@ -456,7 +453,8 @@ private static boolean stripOutputPaths( CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class); return coreOptions.outputPathsMode == OutputPathsMode.STRIP && qualifyingMnemonics.contains(mnemonic) - && PathStripper.isPathStrippable(inputs, primaryOutput.getExecPath().subFragment(0, 1)); + && PathStripper.isPathStrippable( + inputs, Iterables.get(outputs, 0).getExecPath().subFragment(0, 1)); } } } 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 28ef8c35003fa0..a10bb4e9c5e97c 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 @@ -14,10 +14,8 @@ package com.google.devtools.build.lib.analysis.extra; -import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.Action; @@ -56,9 +54,6 @@ public final class ExtraAction extends SpawnAction { private final boolean createDummyOutput; private final NestedSet extraActionInputs; - public static final Function GET_SHADOWED_ACTION = - e -> e != null ? e.getShadowedAction() : null; - ExtraAction( NestedSet extraActionInputs, RunfilesSupplier runfilesSupplier, @@ -78,7 +73,6 @@ public final class ExtraAction extends SpawnAction { NestedSetBuilder.emptySet(Order.STABLE_ORDER), extraActionInputs), outputs, - Iterables.getFirst(outputs, null), AbstractAction.DEFAULT_RESOURCE_SET, CommandLines.of(argv), CommandLineLimits.UNLIMITED, @@ -128,7 +122,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution updateInputs( createInputs(shadowedAction.getInputs(), inputFilesForExtraAction, extraActionInputs)); return NestedSetBuilder.wrap( - Order.STABLE_ORDER, Sets.difference(getInputs().toSet(), oldInputs.toSet())); + Order.STABLE_ORDER, Sets.difference(getInputs().toSet(), oldInputs.toSet())); } private static NestedSet createInputs( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 8e628c3ed5800d..2206435b04f3e8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -676,7 +676,7 @@ public void setupEnvVariables(Map env, Duration timeout) { env.put("TEST_RANDOM_SEED", Integer.toString(getRunNumber() + 1)); } // TODO(b/184206260): Actually set TEST_RANDOM_SEED with random seed. - // The above TEST_RANDOM_SEED has histroically been set with the run number, but we should + // The above TEST_RANDOM_SEED has historically been set with the run number, but we should // explicitly set TEST_RUN_NUMBER to indicate the run number and actually set TEST_RANDOM_SEED // with a random seed. However, much code has come to depend on it being set to the run number // and this is an externally documented behavior. Modifying TEST_RANDOM_SEED should be done @@ -923,11 +923,6 @@ public String getRunfilesPrefix() { return workspaceName; } - @Override - public Artifact getPrimaryOutput() { - return testLog; - } - public PackageSpecificationProvider getNetworkAllowlist() { return networkAllowlist; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index 2a182d5872c3b2..caa067d69d1c16 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -1732,7 +1732,7 @@ private void createModuleCodegenAction( configuration, featureConfiguration, builder, ruleErrorConsumer); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer); actionRegistry.registerAction(compileAction); - Artifact objectFile = compileAction.getOutputFile(); + Artifact objectFile = compileAction.getPrimaryOutput(); if (pic) { result.addPicObjectFile(objectFile); } else { @@ -1775,7 +1775,7 @@ private void createHeaderAction( configuration, featureConfiguration, builder, ruleErrorConsumer); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer); actionRegistry.registerAction(compileAction); - Artifact tokenFile = compileAction.getOutputFile(); + Artifact tokenFile = compileAction.getPrimaryOutput(); result.addHeaderTokenFile(tokenFile); } @@ -1868,13 +1868,13 @@ private ImmutableList createSourceAction( configuration, featureConfiguration, picBuilder, ruleErrorConsumer); CppCompileAction picAction = picBuilder.buildOrThrowRuleError(ruleErrorConsumer); actionRegistry.registerAction(picAction); - directOutputs.add(picAction.getOutputFile()); + directOutputs.add(picAction.getPrimaryOutput()); if (addObject) { - result.addPicObjectFile(picAction.getOutputFile()); + result.addPicObjectFile(picAction.getPrimaryOutput()); if (bitcodeOutput) { result.addLtoBitcodeFile( - picAction.getOutputFile(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel)); + picAction.getPrimaryOutput(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel)); } } if (dwoFile != null) { @@ -1939,7 +1939,7 @@ private ImmutableList createSourceAction( configuration, featureConfiguration, builder, ruleErrorConsumer); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer); actionRegistry.registerAction(compileAction); - Artifact objectFile = compileAction.getOutputFile(); + Artifact objectFile = compileAction.getPrimaryOutput(); directOutputs.add(objectFile); if (addObject) { result.addObjectFile(objectFile); @@ -2118,7 +2118,7 @@ private ImmutableList createTempsActions( CppCompileAction sdAction = sdBuilder.buildOrThrowRuleError(ruleErrorConsumer); actionRegistry.registerAction(sdAction); - return ImmutableList.of(dAction.getOutputFile(), sdAction.getOutputFile()); + return ImmutableList.of(dAction.getPrimaryOutput(), sdAction.getPrimaryOutput()); } /** 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 3d093729a3cba1..82d2010058dd8e 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 @@ -123,10 +123,10 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable private static final boolean VALIDATION_DEBUG_WARN = false; - @VisibleForTesting public static final String CPP_COMPILE_MNEMONIC = "CppCompile"; - @VisibleForTesting public static final String OBJC_COMPILE_MNEMONIC = "ObjcCompile"; + @VisibleForTesting static final String CPP_COMPILE_MNEMONIC = "CppCompile"; + @VisibleForTesting static final String OBJC_COMPILE_MNEMONIC = "ObjcCompile"; - final Artifact outputFile; + @Nullable private final Artifact gcnoFile; private final Artifact sourceFile; private final CppConfiguration cppConfiguration; private final NestedSet mandatoryInputs; @@ -144,8 +144,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable private final boolean shouldScanIncludes; private final boolean usePic; private final boolean useHeaderModules; - final boolean needsIncludeValidation; - private final boolean hasCoverageArtifact; + private final boolean needsIncludeValidation; private final CcCompilationContext ccCompilationContext; private final ImmutableList builtinIncludeFiles; @@ -273,7 +272,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable .addTransitive(inputsForInvalidation) .build(), collectOutputs( - outputFile, + Preconditions.checkNotNull(outputFile, "outputFile"), dotdFile, diagnosticsFile, gcnoFile, @@ -281,8 +280,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable ltoIndexingFile, additionalOutputs), env); - Preconditions.checkNotNull(outputFile); - this.outputFile = outputFile; + this.gcnoFile = gcnoFile; this.sourceFile = sourceFile; this.shareable = shareable; this.cppConfiguration = cppConfiguration; @@ -304,7 +302,6 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable coptsFilter, actionName, dotdFile, - diagnosticsFile, featureConfiguration, variables); this.executionInfo = executionInfo; @@ -324,11 +321,10 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable .getParentDirectory() .getChild(outputFile.getFilename() + ".params"); } - this.hasCoverageArtifact = gcnoFile != null; } private static ImmutableSet collectOutputs( - @Nullable Artifact outputFile, + Artifact outputFile, @Nullable Artifact dotdFile, @Nullable Artifact diagnosticsFile, @Nullable Artifact gcnoFile, @@ -336,14 +332,11 @@ 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. + outputs.add(outputFile); if (gcnoFile != null) { outputs.add(gcnoFile); } outputs.addAll(additionalOutputs); - if (outputFile != null) { - outputs.add(outputFile); - } if (dotdFile != null) { outputs.add(dotdFile); } @@ -364,7 +357,6 @@ static CompileCommandLine buildCommandLine( CoptsFilter coptsFilter, String actionName, Artifact dotdFile, - Artifact diagnosticsFile, FeatureConfiguration featureConfiguration, CcToolchainVariables variables) { return CompileCommandLine.builder(sourceFile, coptsFilter, actionName, dotdFile) @@ -388,11 +380,11 @@ boolean shouldScanIncludes() { return shouldScanIncludes; } - public boolean useInMemoryDotdFiles() { + boolean useInMemoryDotdFiles() { return cppConfiguration.getInmemoryDotdFiles(); } - public boolean enabledCppCompileResourcesEstimation() { + boolean enabledCppCompileResourcesEstimation() { return cppConfiguration.getExperimentalCppCompileResourcesEstimation(); } @@ -420,6 +412,7 @@ public ImmutableSet getMandatoryOutputs() { // discarded as orphans. // This is strictly better than marking all transitive modules as inputs, which would also // effectively disable orphan detection for .pcm files. + Artifact outputFile = getPrimaryOutput(); if (outputFile.isFileType(CppFileTypes.CPP_MODULE)) { return ImmutableSet.of(outputFile); } @@ -436,7 +429,7 @@ public NestedSet getAdditionalInputs() { } /** Clears the discovered {@link #additionalInputs}. */ - public void clearAdditionalInputs() { + private void clearAdditionalInputs() { additionalInputs = null; } @@ -514,7 +507,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution throw new ActionExecutionException(message, this, /*catastrophe=*/ false, code); } commandLineKey = computeCommandLineKey(options); - List systemIncludeDirs = getSystemIncludeDirs(options); + ImmutableList systemIncludeDirs = getSystemIncludeDirs(options); boolean siblingLayout = actionExecutionContext .getOptions() @@ -559,7 +552,8 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution } if (useHeaderModules) { - boolean separate = outputFile.equals(ccCompilationContext.getSeparateHeaderModule(usePic)); + boolean separate = + getPrimaryOutput().equals(ccCompilationContext.getSeparateHeaderModule(usePic)); usedModules = ccCompilationContext.computeUsedModules(usePic, additionalInputs.toSet(), separate); } @@ -599,7 +593,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution additionalInputs = NestedSetBuilder.fromNestedSet(additionalInputs).addTransitive(discoveredModules).build(); - if (outputFile.isFileType(CppFileTypes.CPP_MODULE)) { + if (getPrimaryOutput().isFileType(CppFileTypes.CPP_MODULE)) { this.discoveredModules = discoveredModules; } usedModules = null; @@ -638,21 +632,11 @@ public Artifact getPrimaryInput() { return getSourceFile(); } - @Override - public Artifact getPrimaryOutput() { - return getOutputFile(); - } - /** Returns the path of the c/cc source for gcc. */ public final Artifact getSourceFile() { return compileCommandLine.getSourceFile(); } - /** Returns the path where gcc should put its result. */ - public Artifact getOutputFile() { - return outputFile; - } - @Override @Nullable public Artifact getGrepIncludes() { @@ -729,7 +713,7 @@ List getSystemIncludeDirs() throws CommandLineExpansionException { return getSystemIncludeDirs(getCompilerOptions()); } - private List getSystemIncludeDirs(List compilerOptions) { + private ImmutableList getSystemIncludeDirs(List compilerOptions) { // TODO(bazel-team): parsing the command line flags here couples us to gcc- and clang-cl-style // compiler command lines; use a different way to specify system includes (for example through a // system_includes attribute in cc_toolchain); note that that would disallow users from @@ -819,6 +803,7 @@ public Artifact getMainIncludeScannerSource() { @Override public ImmutableList getIncludeScannerSources() { if (getSourceFile().isFileType(CppFileTypes.CPP_MODULE_MAP)) { + Artifact outputFile = getPrimaryOutput(); boolean isSeparate = outputFile.equals(ccCompilationContext.getSeparateHeaderModule(usePic)); // Expected 0 args, but got 1. Preconditions.checkState( @@ -883,7 +868,7 @@ public List getArguments() throws CommandLineExpansionException { } @Override - public Sequence getStarlarkArgv() throws EvalException, InterruptedException { + public Sequence getStarlarkArgv() throws EvalException { try { return StarlarkList.immutableCopyOf( compileCommandLine.getArguments( @@ -895,7 +880,7 @@ public Sequence getStarlarkArgv() throws EvalException, InterruptedExcep } @Override - public Sequence getStarlarkArgs() throws EvalException { + public Sequence getStarlarkArgs() { ImmutableSet directoryInputs = getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet()); @@ -916,10 +901,6 @@ public Sequence getStarlarkArgs() throws EvalException { return StarlarkList.immutableCopyOf(ImmutableList.of(args)); } - public ParamFileActionInput getParamFileActionInput() { - return paramFileActionInput; - } - @Override public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyContext) throws CommandLineExpansionException, InterruptedException { @@ -931,7 +912,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont for (String option : options) { info.addCompilerOption(option); } - info.setOutputFile(outputFile.getExecPathString()); + info.setOutputFile(getPrimaryOutput().getExecPathString()); info.setSourceFile(getSourceFile().getExecPathString()); if (inputsDiscovered()) { info.addAllSourcesAndHeaders(Artifact.toExecPaths(getInputs().toList())); @@ -954,7 +935,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont return super.getExtraActionInfo(actionKeyContext) .setExtension(CppCompileInfo.cppCompileInfo, info.build()); } catch (CommandLineExpansionException e) { - throw new AssertionError("CppCompileAction command line expansion cannot fail."); + throw new AssertionError("CppCompileAction command line expansion cannot fail", e); } } @@ -1035,7 +1016,7 @@ public void validateInclusions( // Copy the nested sets to hash sets for fast contains checking, but do so lazily. // Avoid immutable sets here to limit memory churn. - Set looseHdrsDirs = ccCompilationContext.getLooseHdrsDirs().toSet(); + ImmutableSet looseHdrsDirs = ccCompilationContext.getLooseHdrsDirs().toSet(); for (Artifact input : inputsForValidation.toList()) { if (!validateInclude( actionExecutionContext, allowedIncludes, looseHdrsDirs, ignoreDirs, input)) { @@ -1236,7 +1217,7 @@ private static CcToolchainVariables calculateModuleVariable( return variableBuilder.build(); } - public CcToolchainVariables getOverwrittenVariables() { + CcToolchainVariables getOverwrittenVariables() { if (useHeaderModules) { // TODO(cmita): Avoid keeping state in CppCompileAction. // There are two cases for when this method might be called: @@ -1268,7 +1249,7 @@ public NestedSet getAllowedDerivedInputs() { // The separate module is an allowed input to all compiles of this context except for its own // compile. Artifact separateModule = ccCompilationContext.getSeparateHeaderModule(usePic); - if (separateModule != null && !separateModule.equals(outputFile)) { + if (separateModule != null && !separateModule.equals(getPrimaryOutput())) { builder.add(separateModule); } return builder.build(); @@ -1284,7 +1265,7 @@ public NestedSet getAllowedDerivedInputs() { @Override public synchronized void updateInputs(NestedSet inputs) { super.updateInputs(inputs); - if (outputFile.isFileType(CppFileTypes.CPP_MODULE)) { + if (getPrimaryOutput().isFileType(CppFileTypes.CPP_MODULE)) { discoveredModules = NestedSetBuilder.wrap( Order.STABLE_ORDER, @@ -1321,7 +1302,7 @@ public NestedSet getDeclaredIncludeSrcs() { * estimation we are using form C + K * inputs, where C and K selected in such way, that more than * 95% of actions used less than C + K * inputs MB of memory during execution. */ - public static ResourceSet estimateResourceConsumptionLocal( + static ResourceSet estimateResourceConsumptionLocal( boolean enabled, String mnemonic, OS os, int inputs) { if (!enabled) { return AbstractAction.DEFAULT_RESOURCE_SET; @@ -1375,7 +1356,7 @@ public void computeKey( executionInfo, getCommandLineKey(), ccCompilationContext.getDeclaredIncludeSrcs(), - getMandatoryInputs(), + mandatoryInputs, additionalPrunableHeaders, ccCompilationContext.getLooseHdrsDirs(), builtInIncludeDirectories, @@ -1605,7 +1586,7 @@ e, createFailureDetail("OutErr copy failure", Code.COPY_OUT_ERR_FAILURE)), } @Nullable - protected byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException { + private byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException { if (getDotdFile() != null) { InputStream in = spawnResult.getInMemoryOutput(getDotdFile()); if (in != null) { @@ -1620,12 +1601,11 @@ protected byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalEx return null; } - protected boolean shouldParseShowIncludes() { + boolean shouldParseShowIncludes() { return featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES); } - protected Spawn createSpawn(Path execRoot, Map clientEnv) - throws ActionExecutionException { + Spawn createSpawn(Path execRoot, Map clientEnv) throws ActionExecutionException { // Intentionally not adding {@link CppCompileAction#inputsForInvalidation}, those are not needed // for execution. NestedSetBuilder inputsBuilder = @@ -1633,8 +1613,8 @@ protected Spawn createSpawn(Path execRoot, Map clientEnv) if (discoversInputs()) { inputsBuilder.addTransitive(getAdditionalInputs()); } - if (getParamFileActionInput() != null) { - inputsBuilder.add(getParamFileActionInput()); + if (paramFileActionInput != null) { + inputsBuilder.add(paramFileActionInput); } NestedSet inputs = inputsBuilder.build(); @@ -1669,15 +1649,13 @@ protected Spawn createSpawn(Path execRoot, Map clientEnv) ImmutableList.copyOf(getArguments()), getEffectiveEnvironment(clientEnv), executionInfo.buildOrThrow(), - /*runfilesSupplier=*/ null, - /*filesetMappings=*/ ImmutableMap.of(), + /* runfilesSupplier= */ null, + /* filesetMappings= */ ImmutableMap.of(), inputs, - /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /* 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, + /* mandatoryOutputs= */ gcnoFile != null ? ImmutableSet.of(gcnoFile) : null, () -> estimateResourceConsumptionLocal( enabledCppCompileResourcesEstimation(), @@ -1739,7 +1717,7 @@ public NestedSet discoverInputsFromDotdFiles( siblingRepositoryLayout); } - public DependencySet processDepset( + private DependencySet processDepset( ActionExecutionContext actionExecutionContext, Path execRoot, byte[] dotDContents) throws ActionExecutionException { try { @@ -1756,7 +1734,7 @@ public DependencySet processDepset( } } - public List getPermittedSystemIncludePrefixes(Path execRoot) { + private List getPermittedSystemIncludePrefixes(Path execRoot) { List systemIncludePrefixes = new ArrayList<>(); for (PathFragment includePath : getBuiltInIncludeDirectories()) { if (includePath.isAbsolute()) { @@ -1773,20 +1751,16 @@ public List getPermittedSystemIncludePrefixes(Path execRoot) { */ private void ensureCoverageNotesFileExists(ActionExecutionContext actionExecutionContext) throws ActionExecutionException { - if (!hasCoverageArtifact) { + if (gcnoFile == null) { return; } - Artifact gcnoArtifact = getOutputs().iterator().next(); - if (!gcnoArtifact.isFileType(CppFileTypes.COVERAGE_NOTES)) { + if (!gcnoFile.isFileType(CppFileTypes.COVERAGE_NOTES)) { BugReport.sendBugReport( new IllegalStateException( - "In coverage mode but gcno artifact is not first output: " - + gcnoArtifact - + ", " - + this)); + "In coverage mode but gcno artifact is not correct type: " + gcnoFile + ", " + this)); return; } - Path outputPath = actionExecutionContext.getInputPath(gcnoArtifact); + Path outputPath = actionExecutionContext.getInputPath(gcnoFile); if (outputPath.exists()) { return; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java index 28acc9a9f8f769..ff84c65738d84a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java @@ -55,7 +55,7 @@ public final class CppCompileActionTemplate extends ActionKeyCacher private final NestedSet allInputs; /** - * Creates an CppCompileActionTemplate. + * Creates a CppCompileActionTemplate. * * @param sourceTreeArtifact the TreeArtifact that contains source files to compile. * @param outputTreeArtifact the TreeArtifact that contains compilation outputs. @@ -170,7 +170,6 @@ protected void computeKey( cppCompileActionBuilder.getCoptsFilter(), CppActionNames.CPP_COMPILE, dotdTreeArtifact, - diagnosticsTreeArtifact, cppCompileActionBuilder.getFeatureConfiguration(), cppCompileActionBuilder.getVariables()); CppCompileAction.computeKey( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java index f30a84ccd4e1ab..93577a75924c15 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java @@ -17,6 +17,7 @@ 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.common.collect.Sets; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.ActionEnvironment; @@ -44,7 +45,6 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.IOException; -import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -77,8 +77,7 @@ public LtoBackendAction( NestedSet inputs, @Nullable BitcodeFiles allBitcodeFiles, @Nullable Artifact importsFile, - Collection outputs, - Artifact primaryOutput, + ImmutableSet outputs, ActionOwner owner, CommandLines argv, CommandLineLimits commandLineLimits, @@ -93,7 +92,6 @@ public LtoBackendAction( NestedSetBuilder.emptySet(Order.STABLE_ORDER), inputs, outputs, - primaryOutput, AbstractAction.DEFAULT_RESOURCE_SET, argv, commandLineLimits, @@ -261,8 +259,7 @@ protected SpawnAction createSpawnAction( ActionOwner owner, NestedSet tools, NestedSet inputsAndTools, - ImmutableList outputs, - Artifact primaryOutput, + ImmutableSet outputs, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -278,7 +275,6 @@ protected SpawnAction createSpawnAction( bitcodeFiles, imports, outputs, - primaryOutput, owner, commandLines, commandLineLimits, diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java index e56f470774da7e..bf58ad33c023bf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java @@ -16,7 +16,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -55,7 +54,6 @@ public GenRuleAction( tools, inputs, outputs, - Iterables.getFirst(outputs, null), AbstractAction.DEFAULT_RESOURCE_SET, commandLines, CommandLineLimits.UNLIMITED, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java index 492db60c121c38..f37c6a6183397a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java @@ -433,7 +433,6 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), /* inputs= */ allInputs, /* outputs= */ outputs.build(), - /* primaryOutput= */ outputJar, /* resourceSetOrBuilder= */ AbstractAction.DEFAULT_RESOURCE_SET, /* commandLines= */ CommandLines.builder() .addCommandLine(executableLine)