From 0d98bf5197732fe7a467f52b4317da70cbb58542 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 12 Apr 2023 01:38:21 -0700 Subject: [PATCH] Automated rollback of commit 4a2e51b3d7b7a27d243e8cc233d60fbbaeeb6190. *** Reason for rollback *** Breaks targets on the nightly TGP. Reproduction: blaze build //third_party/bazel_rules/rules_kotlin/tests/android/java/com/google/jni:AndroidJniTest --compilation_mode=opt --flaky_test_attempts=2 --fat_apk_cpu=x86 --android_platforms=//buildenv/platforms/android:x86 --incompatible_enable_android_toolchain_resolution=1 --collect_code_coverage=1 --instrumentation_filter=//java/com/google/android/samples/apps/topeka[/:],//third_party/bazel_rules/rules_kotlin[/:],//tools/build_defs/kotlin[/:] TGP link: [] *** Original change description *** Make `getPrimaryOutput()` always return the first artifact in `getOutputs()`. This is already the case everywhere except `CppCompileAction` but was not documented, leading to `SpawnAction` unnecessarily storing a field for the primary output when it's already just the first element in its outputs. This change saves 8 bytes per `SpawnAction` and `GenRuleAction`, and moves other `SpawnAction` subclasses closer to an 8-byte threshold. `CppCompileAction` had been using the coverage artifact (if pr *** RELNOTES: None. PiperOrigin-RevId: 523634597 Change-Id: I0aa70851fe4d403afabc56e808546d6638a9f2b7 --- .../build/lib/actions/AbstractAction.java | 6 +- .../lib/actions/ActionAnalysisMetadata.java | 3 +- .../lib/actions/ActionExecutionMetadata.java | 6 +- .../lib/analysis/actions/SpawnAction.java | 39 ++++-- .../lib/analysis/actions/StarlarkAction.java | 16 +-- .../build/lib/analysis/extra/ExtraAction.java | 8 +- .../lib/analysis/test/TestRunnerAction.java | 7 +- .../lib/rules/cpp/CcCompilationHelper.java | 14 +-- .../build/lib/rules/cpp/CppCompileAction.java | 112 +++++++++++------- .../rules/cpp/CppCompileActionTemplate.java | 3 +- .../build/lib/rules/cpp/LtoBackendAction.java | 10 +- .../lib/rules/genrule/GenRuleAction.java | 2 + .../java/JavaHeaderCompileActionBuilder.java | 1 + 13 files changed, 149 insertions(+), 78 deletions(-) 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 aa60baf0feafac..ab1d50fcef0920 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,8 +309,10 @@ public Artifact getPrimaryInput() { } @Override - public final Artifact getPrimaryOutput() { - return Iterables.get(outputs, 0); + 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); } @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 b7df6037af557d..8a513508b917a1 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,8 +186,7 @@ NestedSet getInputFilesForExtraAction(ActionExecutionContext actionExe Artifact getPrimaryInput(); /** - * Returns the "primary" output of this action, which is the same as the first artifact in {@link - * #getOutputs}. + * Returns the "primary" output of this action. * *

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 df9172da59b4fd..4d21092df79b82 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}. 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(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. * *

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 b9332e5e0ff096..666489a875e80e 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,7 +14,6 @@ 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; @@ -104,7 +103,7 @@ public interface ExtraActionInfoSupplier { private static final String GUID = "ebd6fce3-093e-45ee-adb6-bf513b602f0d"; - private static final Interner> executionInfoInterner = + public static final Interner> executionInfoInterner = BlazeInterners.newWeakInterner(); private final CommandLines commandLines; @@ -119,6 +118,7 @@ public interface ExtraActionInfoSupplier { private final ImmutableMap executionInfo; private final ExtraActionInfoSupplier extraActionInfoSupplier; + private final Artifact primaryOutput; private final Consumer>> resultConsumer; private final boolean stripOutputPaths; @@ -132,6 +132,7 @@ 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 @@ -147,6 +148,7 @@ public SpawnAction( NestedSet tools, NestedSet inputs, Iterable outputs, + Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -159,6 +161,7 @@ public SpawnAction( tools, inputs, outputs, + primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -185,6 +188,7 @@ 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 @@ -202,6 +206,7 @@ public SpawnAction( NestedSet tools, NestedSet inputs, Iterable outputs, + Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -216,6 +221,7 @@ public SpawnAction( Consumer>> resultConsumer, boolean stripOutputPaths) { super(owner, tools, inputs, runfilesSupplier, outputs, env); + this.primaryOutput = primaryOutput; this.resourceSetOrBuilder = resourceSetOrBuilder; this.executionInfo = executionInfo.isEmpty() @@ -232,6 +238,11 @@ public SpawnAction( this.stripOutputPaths = stripOutputPaths; } + @Override + public Artifact getPrimaryOutput() { + return primaryOutput; + } + @VisibleForTesting public CommandLines getCommandLines() { return commandLines; @@ -250,10 +261,12 @@ public List getArguments() throws CommandLineExpansionException, Interru } @Override - public Sequence getStarlarkArgs() { + public Sequence getStarlarkArgs() throws EvalException { ImmutableList.Builder result = ImmutableList.builder(); ImmutableSet directoryInputs = - getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet()); + getInputs().toList().stream() + .filter(artifact -> artifact.isDirectory()) + .collect(ImmutableSet.toImmutableSet()); for (CommandLineAndParamFileInfo commandLine : commandLines.getCommandLines()) { result.add(Args.forRegisteredAction(commandLine, directoryInputs)); @@ -523,7 +536,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. */ - private SpawnInfo getExtraActionSpawnInfo() + protected SpawnInfo getExtraActionSpawnInfo() throws CommandLineExpansionException, InterruptedException { SpawnInfo.Builder info = SpawnInfo.newBuilder(); Spawn spawn = getSpawnForExtraAction(); @@ -588,7 +601,7 @@ private ActionSpawn( throws CommandLineExpansionException { super( arguments, - ImmutableMap.of(), + ImmutableMap.of(), parent.getExecutionInfo(), parent.getRunfilesSupplier(), parent, @@ -792,7 +805,8 @@ private SpawnAction buildSpawnAction( owner, tools, inputsAndTools, - ImmutableSet.copyOf(outputs), + ImmutableList.copyOf(outputs), + outputs.get(0), resourceSetOrBuilder, commandLines, commandLineLimits, @@ -812,7 +826,8 @@ protected SpawnAction createSpawnAction( ActionOwner owner, NestedSet tools, NestedSet inputsAndTools, - ImmutableSet outputs, + ImmutableList outputs, + Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -828,6 +843,7 @@ protected SpawnAction createSpawnAction( tools, inputsAndTools, outputs, + primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -933,6 +949,13 @@ 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 e44d64bbd0b4f7..69eb744b5ae8a3 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,8 +18,6 @@ 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; @@ -83,6 +81,7 @@ 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. @@ -104,6 +103,7 @@ private StarlarkAction( NestedSet tools, NestedSet inputs, Iterable outputs, + Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -123,6 +123,7 @@ private StarlarkAction( ? createInputs(shadowedAction.get().getInputs(), inputs) : inputs, outputs, + primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -375,7 +376,8 @@ protected SpawnAction createSpawnAction( ActionOwner owner, NestedSet tools, NestedSet inputsAndTools, - ImmutableSet outputs, + ImmutableList outputs, + Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -401,6 +403,7 @@ protected SpawnAction createSpawnAction( tools, inputsAndTools, outputs, + primaryOutput, resourceSetOrBuilder, commandLines, commandLineLimits, @@ -412,7 +415,7 @@ protected SpawnAction createSpawnAction( mnemonic, unusedInputsList, shadowedAction, - stripOutputPaths(mnemonic, inputsAndTools, outputs, configuration)); + stripOutputPaths(mnemonic, inputsAndTools, primaryOutput, configuration)); } /** @@ -432,7 +435,7 @@ protected SpawnAction createSpawnAction( private static boolean stripOutputPaths( String mnemonic, NestedSet inputs, - ImmutableSet outputs, + Artifact primaryOutput, BuildConfigurationValue configuration) { ImmutableList qualifyingMnemonics = ImmutableList.of( @@ -453,8 +456,7 @@ private static boolean stripOutputPaths( CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class); return coreOptions.outputPathsMode == OutputPathsMode.STRIP && qualifyingMnemonics.contains(mnemonic) - && PathStripper.isPathStrippable( - inputs, Iterables.get(outputs, 0).getExecPath().subFragment(0, 1)); + && PathStripper.isPathStrippable(inputs, primaryOutput.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 a10bb4e9c5e97c..28ef8c35003fa0 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,8 +14,10 @@ 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; @@ -54,6 +56,9 @@ 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, @@ -73,6 +78,7 @@ 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, @@ -122,7 +128,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 2206435b04f3e8..8e628c3ed5800d 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 historically been set with the run number, but we should + // The above TEST_RANDOM_SEED has histroically 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,6 +923,11 @@ 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 caa067d69d1c16..2a182d5872c3b2 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.getPrimaryOutput(); + Artifact objectFile = compileAction.getOutputFile(); 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.getPrimaryOutput(); + Artifact tokenFile = compileAction.getOutputFile(); 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.getPrimaryOutput()); + directOutputs.add(picAction.getOutputFile()); if (addObject) { - result.addPicObjectFile(picAction.getPrimaryOutput()); + result.addPicObjectFile(picAction.getOutputFile()); if (bitcodeOutput) { result.addLtoBitcodeFile( - picAction.getPrimaryOutput(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel)); + picAction.getOutputFile(), 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.getPrimaryOutput(); + Artifact objectFile = compileAction.getOutputFile(); 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.getPrimaryOutput(), sdAction.getPrimaryOutput()); + return ImmutableList.of(dAction.getOutputFile(), sdAction.getOutputFile()); } /** 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 82d2010058dd8e..3d093729a3cba1 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 static final String CPP_COMPILE_MNEMONIC = "CppCompile"; - @VisibleForTesting static final String OBJC_COMPILE_MNEMONIC = "ObjcCompile"; + @VisibleForTesting public static final String CPP_COMPILE_MNEMONIC = "CppCompile"; + @VisibleForTesting public static final String OBJC_COMPILE_MNEMONIC = "ObjcCompile"; - @Nullable private final Artifact gcnoFile; + final Artifact outputFile; private final Artifact sourceFile; private final CppConfiguration cppConfiguration; private final NestedSet mandatoryInputs; @@ -144,7 +144,8 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable private final boolean shouldScanIncludes; private final boolean usePic; private final boolean useHeaderModules; - private final boolean needsIncludeValidation; + final boolean needsIncludeValidation; + private final boolean hasCoverageArtifact; private final CcCompilationContext ccCompilationContext; private final ImmutableList builtinIncludeFiles; @@ -272,7 +273,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable .addTransitive(inputsForInvalidation) .build(), collectOutputs( - Preconditions.checkNotNull(outputFile, "outputFile"), + outputFile, dotdFile, diagnosticsFile, gcnoFile, @@ -280,7 +281,8 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable ltoIndexingFile, additionalOutputs), env); - this.gcnoFile = gcnoFile; + Preconditions.checkNotNull(outputFile); + this.outputFile = outputFile; this.sourceFile = sourceFile; this.shareable = shareable; this.cppConfiguration = cppConfiguration; @@ -302,6 +304,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable coptsFilter, actionName, dotdFile, + diagnosticsFile, featureConfiguration, variables); this.executionInfo = executionInfo; @@ -321,10 +324,11 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable .getParentDirectory() .getChild(outputFile.getFilename() + ".params"); } + this.hasCoverageArtifact = gcnoFile != null; } private static ImmutableSet collectOutputs( - Artifact outputFile, + @Nullable Artifact outputFile, @Nullable Artifact dotdFile, @Nullable Artifact diagnosticsFile, @Nullable Artifact gcnoFile, @@ -332,11 +336,14 @@ private static ImmutableSet collectOutputs( @Nullable Artifact ltoIndexingFile, ImmutableList additionalOutputs) { ImmutableSet.Builder outputs = ImmutableSet.builder(); - outputs.add(outputFile); + // 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); + } if (dotdFile != null) { outputs.add(dotdFile); } @@ -357,6 +364,7 @@ static CompileCommandLine buildCommandLine( CoptsFilter coptsFilter, String actionName, Artifact dotdFile, + Artifact diagnosticsFile, FeatureConfiguration featureConfiguration, CcToolchainVariables variables) { return CompileCommandLine.builder(sourceFile, coptsFilter, actionName, dotdFile) @@ -380,11 +388,11 @@ boolean shouldScanIncludes() { return shouldScanIncludes; } - boolean useInMemoryDotdFiles() { + public boolean useInMemoryDotdFiles() { return cppConfiguration.getInmemoryDotdFiles(); } - boolean enabledCppCompileResourcesEstimation() { + public boolean enabledCppCompileResourcesEstimation() { return cppConfiguration.getExperimentalCppCompileResourcesEstimation(); } @@ -412,7 +420,6 @@ 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); } @@ -429,7 +436,7 @@ public NestedSet getAdditionalInputs() { } /** Clears the discovered {@link #additionalInputs}. */ - private void clearAdditionalInputs() { + public void clearAdditionalInputs() { additionalInputs = null; } @@ -507,7 +514,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution throw new ActionExecutionException(message, this, /*catastrophe=*/ false, code); } commandLineKey = computeCommandLineKey(options); - ImmutableList systemIncludeDirs = getSystemIncludeDirs(options); + List systemIncludeDirs = getSystemIncludeDirs(options); boolean siblingLayout = actionExecutionContext .getOptions() @@ -552,8 +559,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution } if (useHeaderModules) { - boolean separate = - getPrimaryOutput().equals(ccCompilationContext.getSeparateHeaderModule(usePic)); + boolean separate = outputFile.equals(ccCompilationContext.getSeparateHeaderModule(usePic)); usedModules = ccCompilationContext.computeUsedModules(usePic, additionalInputs.toSet(), separate); } @@ -593,7 +599,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution additionalInputs = NestedSetBuilder.fromNestedSet(additionalInputs).addTransitive(discoveredModules).build(); - if (getPrimaryOutput().isFileType(CppFileTypes.CPP_MODULE)) { + if (outputFile.isFileType(CppFileTypes.CPP_MODULE)) { this.discoveredModules = discoveredModules; } usedModules = null; @@ -632,11 +638,21 @@ 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() { @@ -713,7 +729,7 @@ List getSystemIncludeDirs() throws CommandLineExpansionException { return getSystemIncludeDirs(getCompilerOptions()); } - private ImmutableList getSystemIncludeDirs(List compilerOptions) { + private List 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 @@ -803,7 +819,6 @@ 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( @@ -868,7 +883,7 @@ public List getArguments() throws CommandLineExpansionException { } @Override - public Sequence getStarlarkArgv() throws EvalException { + public Sequence getStarlarkArgv() throws EvalException, InterruptedException { try { return StarlarkList.immutableCopyOf( compileCommandLine.getArguments( @@ -880,7 +895,7 @@ public Sequence getStarlarkArgv() throws EvalException { } @Override - public Sequence getStarlarkArgs() { + public Sequence getStarlarkArgs() throws EvalException { ImmutableSet directoryInputs = getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet()); @@ -901,6 +916,10 @@ public Sequence getStarlarkArgs() { return StarlarkList.immutableCopyOf(ImmutableList.of(args)); } + public ParamFileActionInput getParamFileActionInput() { + return paramFileActionInput; + } + @Override public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyContext) throws CommandLineExpansionException, InterruptedException { @@ -912,7 +931,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont for (String option : options) { info.addCompilerOption(option); } - info.setOutputFile(getPrimaryOutput().getExecPathString()); + info.setOutputFile(outputFile.getExecPathString()); info.setSourceFile(getSourceFile().getExecPathString()); if (inputsDiscovered()) { info.addAllSourcesAndHeaders(Artifact.toExecPaths(getInputs().toList())); @@ -935,7 +954,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", e); + throw new AssertionError("CppCompileAction command line expansion cannot fail."); } } @@ -1016,7 +1035,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. - ImmutableSet looseHdrsDirs = ccCompilationContext.getLooseHdrsDirs().toSet(); + Set looseHdrsDirs = ccCompilationContext.getLooseHdrsDirs().toSet(); for (Artifact input : inputsForValidation.toList()) { if (!validateInclude( actionExecutionContext, allowedIncludes, looseHdrsDirs, ignoreDirs, input)) { @@ -1217,7 +1236,7 @@ private static CcToolchainVariables calculateModuleVariable( return variableBuilder.build(); } - CcToolchainVariables getOverwrittenVariables() { + public CcToolchainVariables getOverwrittenVariables() { if (useHeaderModules) { // TODO(cmita): Avoid keeping state in CppCompileAction. // There are two cases for when this method might be called: @@ -1249,7 +1268,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(getPrimaryOutput())) { + if (separateModule != null && !separateModule.equals(outputFile)) { builder.add(separateModule); } return builder.build(); @@ -1265,7 +1284,7 @@ public NestedSet getAllowedDerivedInputs() { @Override public synchronized void updateInputs(NestedSet inputs) { super.updateInputs(inputs); - if (getPrimaryOutput().isFileType(CppFileTypes.CPP_MODULE)) { + if (outputFile.isFileType(CppFileTypes.CPP_MODULE)) { discoveredModules = NestedSetBuilder.wrap( Order.STABLE_ORDER, @@ -1302,7 +1321,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. */ - static ResourceSet estimateResourceConsumptionLocal( + public static ResourceSet estimateResourceConsumptionLocal( boolean enabled, String mnemonic, OS os, int inputs) { if (!enabled) { return AbstractAction.DEFAULT_RESOURCE_SET; @@ -1356,7 +1375,7 @@ public void computeKey( executionInfo, getCommandLineKey(), ccCompilationContext.getDeclaredIncludeSrcs(), - mandatoryInputs, + getMandatoryInputs(), additionalPrunableHeaders, ccCompilationContext.getLooseHdrsDirs(), builtInIncludeDirectories, @@ -1586,7 +1605,7 @@ e, createFailureDetail("OutErr copy failure", Code.COPY_OUT_ERR_FAILURE)), } @Nullable - private byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException { + protected byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException { if (getDotdFile() != null) { InputStream in = spawnResult.getInMemoryOutput(getDotdFile()); if (in != null) { @@ -1601,11 +1620,12 @@ private byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExec return null; } - boolean shouldParseShowIncludes() { + protected boolean shouldParseShowIncludes() { return featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES); } - Spawn createSpawn(Path execRoot, Map clientEnv) throws ActionExecutionException { + protected Spawn createSpawn(Path execRoot, Map clientEnv) + throws ActionExecutionException { // Intentionally not adding {@link CppCompileAction#inputsForInvalidation}, those are not needed // for execution. NestedSetBuilder inputsBuilder = @@ -1613,8 +1633,8 @@ Spawn createSpawn(Path execRoot, Map clientEnv) throws ActionExe if (discoversInputs()) { inputsBuilder.addTransitive(getAdditionalInputs()); } - if (paramFileActionInput != null) { - inputsBuilder.add(paramFileActionInput); + if (getParamFileActionInput() != null) { + inputsBuilder.add(getParamFileActionInput()); } NestedSet inputs = inputsBuilder.build(); @@ -1649,13 +1669,15 @@ Spawn createSpawn(Path execRoot, Map clientEnv) throws ActionExe 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= */ gcnoFile != null ? ImmutableSet.of(gcnoFile) : null, + /*mandatoryOutputs=*/ hasCoverageArtifact + ? ImmutableSet.copyOf(getOutputs().asList().subList(1, getOutputs().size())) + : null, () -> estimateResourceConsumptionLocal( enabledCppCompileResourcesEstimation(), @@ -1717,7 +1739,7 @@ public NestedSet discoverInputsFromDotdFiles( siblingRepositoryLayout); } - private DependencySet processDepset( + public DependencySet processDepset( ActionExecutionContext actionExecutionContext, Path execRoot, byte[] dotDContents) throws ActionExecutionException { try { @@ -1734,7 +1756,7 @@ private DependencySet processDepset( } } - private List getPermittedSystemIncludePrefixes(Path execRoot) { + public List getPermittedSystemIncludePrefixes(Path execRoot) { List systemIncludePrefixes = new ArrayList<>(); for (PathFragment includePath : getBuiltInIncludeDirectories()) { if (includePath.isAbsolute()) { @@ -1751,16 +1773,20 @@ private List getPermittedSystemIncludePrefixes(Path execRoot) { */ private void ensureCoverageNotesFileExists(ActionExecutionContext actionExecutionContext) throws ActionExecutionException { - if (gcnoFile == null) { + if (!hasCoverageArtifact) { return; } - if (!gcnoFile.isFileType(CppFileTypes.COVERAGE_NOTES)) { + Artifact gcnoArtifact = getOutputs().iterator().next(); + if (!gcnoArtifact.isFileType(CppFileTypes.COVERAGE_NOTES)) { BugReport.sendBugReport( new IllegalStateException( - "In coverage mode but gcno artifact is not correct type: " + gcnoFile + ", " + this)); + "In coverage mode but gcno artifact is not first output: " + + gcnoArtifact + + ", " + + this)); return; } - Path outputPath = actionExecutionContext.getInputPath(gcnoFile); + Path outputPath = actionExecutionContext.getInputPath(gcnoArtifact); 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 ff84c65738d84a..28acc9a9f8f769 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 a CppCompileActionTemplate. + * Creates an CppCompileActionTemplate. * * @param sourceTreeArtifact the TreeArtifact that contains source files to compile. * @param outputTreeArtifact the TreeArtifact that contains compilation outputs. @@ -170,6 +170,7 @@ 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 93577a75924c15..f30a84ccd4e1ab 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,7 +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.common.collect.Sets; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.ActionEnvironment; @@ -45,6 +44,7 @@ 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,7 +77,8 @@ public LtoBackendAction( NestedSet inputs, @Nullable BitcodeFiles allBitcodeFiles, @Nullable Artifact importsFile, - ImmutableSet outputs, + Collection outputs, + Artifact primaryOutput, ActionOwner owner, CommandLines argv, CommandLineLimits commandLineLimits, @@ -92,6 +93,7 @@ public LtoBackendAction( NestedSetBuilder.emptySet(Order.STABLE_ORDER), inputs, outputs, + primaryOutput, AbstractAction.DEFAULT_RESOURCE_SET, argv, commandLineLimits, @@ -259,7 +261,8 @@ protected SpawnAction createSpawnAction( ActionOwner owner, NestedSet tools, NestedSet inputsAndTools, - ImmutableSet outputs, + ImmutableList outputs, + Artifact primaryOutput, ResourceSetOrBuilder resourceSetOrBuilder, CommandLines commandLines, CommandLineLimits commandLineLimits, @@ -275,6 +278,7 @@ 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 42d7f85546e7c9..c6998db601a5da 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,6 +16,7 @@ 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; @@ -54,6 +55,7 @@ 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 f37c6a6183397a..492db60c121c38 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,6 +433,7 @@ 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)