From 4c4bbec9336eb8b943879a77bb782f771d21ab7a Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 4 Sep 2024 07:50:47 -0700 Subject: [PATCH] Report prunable inputs in `aquery` output. The input sets reported by `aquery` are rather subtly defined for actions that discover inputs: 1. Prunable headers are unconditionally absent before action execution, and present after action execution if they're deemed reachable by include scanning or if scanning is disabled. 2. If the action produces an unused inputs list (or the native C++ equivalent based on the .d file), inputs thus marked are absent after action execution. The difference in treatment between prunable headers and other inputs is, in my opinion, a case of implementation details leaking into the conceptual model: from a user's perspective, it's more natural to describe actions as starting with every header in the inputs and discarding the unreachable ones during execution, mirroring unused inputs; but depsets don't support subtraction, so implementation-wise we found it convenient to start with no headers and later add the reachable ones. https://github.com/bazelbuild/bazel/commit/6b912c56a00a1c17517f11ebc8509fd963df0d1d partially addressed this by means of a separate "scheduling dependencies" output field containing the full set of prunable headers. However, this field can't be the target of an `inputs` predicate, limiting the utility of `aquery` as a tool to investigate scheduling dependencies. On the other hand, there's currently no workaround for revealing inputs explicitly marked as unused after they've been pruned (short of a `clean` command). To comprehensively address these issues, this CL introduces a new `--include_pruned_inputs` flag, which gates the inclusion of pruned inputs into the existing inputs field. The implementation treats prunable headers identically to other inputs: initially present, but possibly absent after execution. The flag defaults to enabled, as user feedback suggests that it's a more intuitive default. The previously introduced `--include_scheduling_dependencies` flag, which hasn't yet made it into a stable Bazel release, is deleted. Fixes #23154. RELNOTES[INC]: The `aquery` command now reports all potential inputs of actions that support input discovery, including the input headers of C++ compilation actions and those explicitly marked as unused through the `unused_inputs_list` argument to `ctx.actions.run`. Set `--noinclude_pruned_inputs` to omit pruned inputs from `aquery` output when running it after action execution. RELNOTES[INC]: This is not a release note, but a reminder to remove the note for `--include_scheduling_dependencies`, which was introduced in the 8.x tree but won't make it into the final release. PiperOrigin-RevId: 670969727 Change-Id: Ibe442ef3e061004c5951df9fce1c1613cd6e7647 --- .../build/lib/actions/AbstractAction.java | 43 +-- .../lib/actions/ActionAnalysisMetadata.java | 15 +- .../lib/actions/ActionExecutionMetadata.java | 3 +- .../analysis/actions/SpawnActionTemplate.java | 5 + .../lib/analysis/actions/StarlarkAction.java | 2 +- .../build/lib/analysis/extra/ExtraAction.java | 5 + .../build/lib/buildtool/AqueryProcessor.java | 2 +- .../build/lib/buildtool/BuildTool.java | 2 +- .../includescanning/SpawnIncludeScanner.java | 5 + ...tionGraphProtoOutputFormatterCallback.java | 2 +- ...onGraphSummaryOutputFormatterCallback.java | 2 +- ...ctionGraphTextOutputFormatterCallback.java | 21 +- .../lib/query2/aquery/AqueryOptions.java | 15 +- .../build/lib/query2/aquery/AqueryUtils.java | 41 ++- .../build/lib/rules/cpp/CppCompileAction.java | 2 +- .../rules/cpp/CppCompileActionTemplate.java | 5 + .../build/lib/rules/cpp/LtoBackendAction.java | 2 +- .../rules/cpp/LtoBackendActionTemplate.java | 5 + .../actiongraph/v2/ActionGraphDump.java | 29 +- src/main/protobuf/analysis_v2.proto | 10 +- .../lib/actions/ResourceManagerTest.java | 5 + .../build/lib/actions/util/TestAction.java | 5 + .../build/lib/exec/util/FakeOwner.java | 5 + .../ActionTemplateExpansionFunctionTest.java | 5 + .../SequencedSkyframeExecutorTest.java | 5 + .../build/lib/testutil/FakeResourceOwner.java | 5 + src/test/shell/integration/aquery_test.sh | 256 +++++++++++++++++- src/test/shell/unittest.bash | 30 ++ 28 files changed, 430 insertions(+), 102 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 2536826c2c6006..3d837acf0e8fd5 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 @@ -125,8 +125,8 @@ public final boolean inputsKnown() { /** * {@inheritDoc} * - *

Should be overridden along with {@link #discoverInputs}, {@link #inputsDiscovered}, and - * {@link #setInputsDiscovered} by actions that do input discovery. + *

Should be overridden along with {@link #discoverInputs}, {@link #inputsDiscovered}, {@link + * #setInputsDiscovered} and {@link #getOriginalInputs} by actions that do input discovery. */ @Override public boolean discoversInputs() { @@ -137,8 +137,7 @@ public boolean discoversInputs() { @Nullable public NestedSet discoverInputs(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { - throw new IllegalStateException("discoverInputs cannot be called for " + this.prettyPrint() - + " since it does not discover inputs"); + throw new IllegalStateException("Not an input-discovering action: " + this); } @Override @@ -147,12 +146,9 @@ public final void resetDiscoveredInputs() { if (!inputsKnown()) { return; } - NestedSet originalInputs = getOriginalInputs(); - if (originalInputs != null) { - synchronized (this) { - inputs = originalInputs; - setInputsDiscovered(false); - } + synchronized (this) { + inputs = getOriginalInputs(); + setInputsDiscovered(false); } } @@ -168,7 +164,7 @@ public final void resetDiscoveredInputs() { @ForOverride @GuardedBy("this") protected boolean inputsDiscovered() { - throw new IllegalStateException("Must be overridden by input-discovering actions: " + this); + throw new IllegalStateException("Must be overridden by input-discovering action: " + this); } /** @@ -178,26 +174,19 @@ protected boolean inputsDiscovered() { @ForOverride @GuardedBy("this") protected void setInputsDiscovered(boolean inputsDiscovered) { - throw new IllegalStateException("Must be overridden by input-discovering actions: " + this); + throw new IllegalStateException("Must be overridden by input-discovering action: " + this); } - /** - * Returns this action's original inputs, prior to {@linkplain #discoverInputs input - * discovery}. - * - *

Input-discovering actions which are able to reconstitute their original inputs may override - * this, allowing for memory savings. - */ - @Nullable - @ForOverride - protected NestedSet getOriginalInputs() { - return null; + @Override + public NestedSet getOriginalInputs() { + checkState(!discoversInputs(), "Must be overridden by input-discovering action"); + return getInputs(); } @Override public NestedSet getAllowedDerivedInputs() { throw new IllegalStateException( - "Method must be overridden for actions that may have unknown inputs."); + "Must be overridden for action that may have unknown inputs: " + this); } @Override @@ -217,7 +206,7 @@ public NestedSet getSchedulingDependencies() { */ @Override public synchronized void updateInputs(NestedSet inputs) { - checkState(discoversInputs(), "Can't update inputs unless discovering: %s %s", this, inputs); + checkState(discoversInputs(), "Not an input-discovering action: %s", this); this.inputs = inputs; setInputsDiscovered(true); } @@ -295,9 +284,7 @@ public Artifact getOriginalPrimaryInput() { // The default behavior is to return the first input artifact of the original input list (before // input discovery). // Call through the method, not the field, because it may be overridden. - NestedSet originalInputs = getOriginalInputs(); - NestedSet inputs = originalInputs == null ? getInputs() : originalInputs; - return Iterables.getFirst(inputs.toList(), null); + return Iterables.getFirst(getOriginalInputs().toList(), 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 0fd11465525322..de39c328468178 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 @@ -128,12 +128,19 @@ String getKey(ActionKeyContext actionKeyContext, @Nullable ArtifactExpander arti /** * Returns the input Artifacts that this Action depends upon. May be empty. * - *

For actions that do input discovery or input pruning, a different result may be returned - * before and after action execution, because input discovery may add additional artifacts from - * {@link #getSchedulingDependencies}, and input pruning may remove them. + *

For actions that do input discovery, a different result may be returned before and after + * action execution, because input discovery may add or remove inputs. The original input set may + * be retrieved from {@link ActionExecutionMetadata#getOriginalInputs}. */ NestedSet getInputs(); + /** + * Returns this action's original inputs prior to input discovery. + * + *

Unlike {@link #getInputs}, the same result is returned before and after action execution. + */ + NestedSet getOriginalInputs(); + /** * Returns the input Artifacts that must be built before the action can be executed, but are not * dependencies of the action in the action cache. @@ -167,7 +174,7 @@ String getKey(ActionKeyContext actionKeyContext, @Nullable ArtifactExpander arti * other files as well. For example C(++) compilation may perform include file header scanning. * This needs to be mirrored by the extra_action rule. Called by {@link * com.google.devtools.build.lib.analysis.extra.ExtraAction} at execution time for actions that - * return true for {link #discoversInputs()}. + * return true for {link ActionExecutionMetadata#discoversInputs}. * * @param actionExecutionContext Services in the scope of the action, like the Out/Err streams. * @throws ActionExecutionException only when code called from this method throws that exception. 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 eb9cc3d0e68897..606491accd4451 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 @@ -79,7 +79,8 @@ default String getProgressMessage(RepositoryMapping mainRepositoryMapping) { String describeKey(); /** - * Returns true iff the {@link #getInputs} set is known to be complete. + * Returns true iff the {@link #getInputs} set has been updated taking input discovery into + * account. * *

For most actions, this always returns true. For actions which {@linkplain #discoversInputs * discover inputs} (e.g. C++ compilation), inputs are dynamically discovered from the previous diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java index 90c91a3518de7a..dac19db7813b39 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java @@ -206,6 +206,11 @@ public NestedSet getInputs() { return allInputs; } + @Override + public NestedSet getOriginalInputs() { + return getInputs(); + } + @Override public NestedSet getSchedulingDependencies() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); 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 7288670b9ff7c7..c6df48534c8e04 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 @@ -256,7 +256,7 @@ public boolean discoversInputs() { } @Override - protected NestedSet getOriginalInputs() { + public NestedSet getOriginalInputs() { return allStarlarkActionInputs; } 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 b1c4c6391a0736..d2a3f128926fbb 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 @@ -136,6 +136,11 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution Order.STABLE_ORDER, Sets.difference(getInputs().toSet(), oldInputs.toSet())); } + @Override + public NestedSet getOriginalInputs() { + return shadowedAction.getOriginalInputs(); + } + @Override public NestedSet getSchedulingDependencies() { return shadowedAction.getSchedulingDependencies(); diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java index d938f7ae112285..be2787e860ca38 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java @@ -87,7 +87,7 @@ public BlazeCommandResult dumpActionGraphFromSkyframe(CommandEnvironment env) { new ActionGraphDump( aqueryOptions.includeCommandline, aqueryOptions.includeArtifacts, - aqueryOptions.includeSchedulingDependencies, + aqueryOptions.includePrunedInputs, actionFilters, aqueryOptions.includeParamFiles, aqueryOptions.includeFileWriteContents, diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index 79c78797b9ec0e..e78715b40620d2 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java @@ -589,7 +589,7 @@ private void dumpSkyframeStateAfterBuild( new ActionGraphDump( /* includeActionCmdLine= */ false, /* includeArtifacts= */ true, - /* includeSchedulingDependencies= */ true, + /* includePrunedInputs= */ true, /* actionFilters= */ null, /* includeParamFiles= */ false, /* includeFileWriteContents= */ false, diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java index 190ee1c2e4f754..c963770e5827a6 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java @@ -202,6 +202,11 @@ public NestedSet getInputs() { return actionExecutionMetadata.getInputs(); } + @Override + public NestedSet getOriginalInputs() { + return actionExecutionMetadata.getInputs(); + } + @Override public NestedSet getSchedulingDependencies() { throw new UnsupportedOperationException(); diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java index b328d234b429b1..7f608e1f0b1b26 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java @@ -75,7 +75,7 @@ public class ActionGraphProtoOutputFormatterCallback extends AqueryThreadsafeCal new ActionGraphDump( options.includeCommandline, options.includeArtifacts, - options.includeSchedulingDependencies, + options.includePrunedInputs, this.actionFilters, options.includeParamFiles, options.includeFileWriteContents, diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphSummaryOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphSummaryOutputFormatterCallback.java index 69a74f0c2eb160..bf7c6919f529fe 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphSummaryOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphSummaryOutputFormatterCallback.java @@ -82,7 +82,7 @@ public void processOutput(Iterable partialResult) } private void processAction(ActionAnalysisMetadata action) throws InterruptedException { - if (!AqueryUtils.matchesAqueryFilters(action, actionFilters)) { + if (!AqueryUtils.matchesAqueryFilters(action, actionFilters, options.includePrunedInputs)) { return; } diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java index 836e5849e76634..1e557686996817 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.query2.aquery; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.devtools.build.lib.query2.aquery.AqueryUtils.getActionInputs; import static com.google.devtools.build.lib.util.StringUtil.reencodeInternalToExternal; import static java.nio.charset.StandardCharsets.UTF_8; @@ -37,6 +38,7 @@ import com.google.devtools.build.lib.analysis.starlark.UnresolvedSymlinkAction; import com.google.devtools.build.lib.buildeventstream.BuildEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.AspectDescriptor; import com.google.devtools.build.lib.packages.LabelPrinter; @@ -122,7 +124,7 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream) getParamFileNameToContentMap().put(paramFileName, fileContent); } - if (!AqueryUtils.matchesAqueryFilters(action, actionFilters)) { + if (!AqueryUtils.matchesAqueryFilters(action, actionFilters, options.includePrunedInputs)) { return; } @@ -199,26 +201,17 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream) } if (options.includeArtifacts) { + NestedSet inputs = getActionInputs(action, options.includePrunedInputs); + stringBuilder .append(" Inputs: [") .append( - action.getInputs().toList().stream() + inputs.toList().stream() .map(input -> escapeBytestringUtf8(input.getExecPathString())) .sorted() .collect(Collectors.joining(", "))) .append("]\n"); - if (options.includeSchedulingDependencies && !action.getSchedulingDependencies().isEmpty()) { - stringBuilder - .append(" SchedulingDependencies: [") - .append( - action.getSchedulingDependencies().toList().stream() - .map(input -> escapeBytestringUtf8(input.getExecPathString())) - .sorted() - .collect(Collectors.joining(", "))) - .append("]\n"); - } - stringBuilder .append(" Outputs: [") .append( @@ -279,7 +272,7 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream) if (options.includeParamFiles) { // Assumption: if an Action takes a param file as an input, it will be used // to provide params to the command. - for (Artifact input : action.getInputs().toList()) { + for (Artifact input : getActionInputs(action, options.includePrunedInputs).toList()) { String inputFileName = input.getExecPathString(); if (getParamFileNameToContentMap().containsKey(inputFileName)) { stringBuilder diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryOptions.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryOptions.java index 845477c0a639b9..699ee950d2635f 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryOptions.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryOptions.java @@ -43,20 +43,19 @@ public class AqueryOptions extends CommonQueryOptions { defaultValue = "true", documentationCategory = OptionDocumentationCategory.QUERY, effectTags = {OptionEffectTag.TERMINAL_OUTPUT}, - help = - "Includes names of the action inputs and outputs in the output " + "(potentially large).") + help = "Includes names of the action inputs and outputs in the output (potentially large).") public boolean includeArtifacts; @Option( - name = "include_scheduling_dependencies", - defaultValue = "false", + name = "include_pruned_inputs", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.QUERY, effectTags = {OptionEffectTag.TERMINAL_OUTPUT}, help = - "Includes names of the scheduling dependencies of actions " - + "(potentially large). " - + "Only takes effect if --include_artifacts is also set.") - public boolean includeSchedulingDependencies; + "Includes action inputs that were pruned during action execution. Only affects actions" + + " that discover inputs and have been executed in a previous invocation. Only takes" + + " effect if --include_artifacts is also set.") + public boolean includePrunedInputs; @Option( name = "include_param_files", diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryUtils.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryUtils.java index 0eba0fd1da05db..abfc2f04d0942b 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryUtils.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryUtils.java @@ -19,26 +19,59 @@ import com.google.common.collect.Streams; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction; import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import java.io.IOException; /** Utility class for Aquery */ public class AqueryUtils { + private AqueryUtils() {} + + /** + * Returns the set of action inputs according to the --include_pruned_inputs flag. + * + *

This may differ from {@link ActionAnalysisMetadata#getInputs} for actions that discover + * inputs. + * + * @param action the analysis metadata of an action + * @param includePrunedInputs the value of the --include_pruned_inputs flag + */ + public static NestedSet getActionInputs( + ActionAnalysisMetadata action, boolean includePrunedInputs) { + if (includePrunedInputs + || (action instanceof ActionExecutionMetadata actionExecutionMetadata + && !actionExecutionMetadata.inputsKnown())) { + // getInputs() is potentially missing inputs that will be added by discovery (if the action + // hasn't yet executed) and inputs that have been removed by discovery (if the action has + // already executed). Instead, assemble the inputs from getOriginalInputs() and + // getSchedulingDependencies(), which also include those added or removed by discovery. + return NestedSetBuilder.stableOrder() + .addTransitive(action.getOriginalInputs()) + .addTransitive(action.getSchedulingDependencies()) + .build(); + } + return action.getInputs(); + } + /** * Return true if the given {@code action} matches the filters specified in {@code actionFilters}. * * @param action the analysis metadata of an action * @param actionFilters the filters parsed from the query expression + * @param includePrunedInputs the value of the --include_pruned_inputs flag * @return whether the action matches the filtering patterns */ public static boolean matchesAqueryFilters( - ActionAnalysisMetadata action, AqueryActionFilter actionFilters) { - NestedSet inputs = action.getInputs(); + ActionAnalysisMetadata action, + AqueryActionFilter actionFilters, + boolean includePrunedInputs) { + NestedSet inputs = getActionInputs(action, includePrunedInputs); Iterable outputs = action.getOutputs(); String mnemonic = action.getMnemonic(); @@ -49,7 +82,7 @@ public static boolean matchesAqueryFilters( } if (actionFilters.hasFilterForFunction(INPUTS)) { - Boolean containsFile = + boolean containsFile = inputs.toList().stream() .anyMatch( artifact -> @@ -62,7 +95,7 @@ public static boolean matchesAqueryFilters( } if (actionFilters.hasFilterForFunction(OUTPUTS)) { - Boolean containsFile = + boolean containsFile = Streams.stream(outputs) .anyMatch( artifact -> 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 533b2d96c225b7..8ba94f289cbaeb 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 @@ -616,7 +616,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution } @Override - protected final NestedSet getOriginalInputs() { + public final NestedSet getOriginalInputs() { return mandatoryInputs; } 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 518d9d17e94b47..6968a2651ce303 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 @@ -346,6 +346,11 @@ public NestedSet getInputs() { .build(); } + @Override + public NestedSet getOriginalInputs() { + return getInputs(); + } + @Override public NestedSet getSchedulingDependencies() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); 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 5546f87bb8591b..2f50b6a6e62f98 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 @@ -223,7 +223,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution } @Override - protected NestedSet getOriginalInputs() { + public NestedSet getOriginalInputs() { return mandatoryInputs; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTemplate.java index 01021d282ce32b..6c40c222a35981 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTemplate.java @@ -408,6 +408,11 @@ public NestedSet getInputs() { return allInputs; } + @Override + public NestedSet getOriginalInputs() { + return allInputs; + } + @Override public ImmutableSet getOutputs() { ImmutableSet.Builder builder = ImmutableSet.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java index 50fbeb1304871e..9449bbc0d7cf30 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.actiongraph.v2; +import static com.google.devtools.build.lib.query2.aquery.AqueryUtils.getActionInputs; + import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -68,7 +70,7 @@ public class ActionGraphDump { @Nullable private final AqueryActionFilter actionFilters; private final boolean includeActionCmdLine; private final boolean includeArtifacts; - private final boolean includeSchedulingDependencies; + private final boolean includePrunedInputs; private final boolean includeParamFiles; private final boolean includeFileWriteContents; private final AqueryOutputHandler aqueryOutputHandler; @@ -79,7 +81,7 @@ public class ActionGraphDump { public ActionGraphDump( boolean includeActionCmdLine, boolean includeArtifacts, - boolean includeSchedulingDependencies, + boolean includePrunedInputs, AqueryActionFilter actionFilters, boolean includeParamFiles, boolean includeFileWriteContents, @@ -89,7 +91,7 @@ public ActionGraphDump( /* actionGraphTargets= */ ImmutableList.of("..."), includeActionCmdLine, includeArtifacts, - includeSchedulingDependencies, + includePrunedInputs, actionFilters, includeParamFiles, includeFileWriteContents, @@ -101,7 +103,7 @@ public ActionGraphDump( List actionGraphTargets, boolean includeActionCmdLine, boolean includeArtifacts, - boolean includeSchedulingDependencies, + boolean includePrunedInputs, AqueryActionFilter actionFilters, boolean includeParamFiles, boolean includeFileWriteContents, @@ -110,7 +112,7 @@ public ActionGraphDump( this.actionGraphTargets = ImmutableSet.copyOf(actionGraphTargets); this.includeActionCmdLine = includeActionCmdLine; this.includeArtifacts = includeArtifacts; - this.includeSchedulingDependencies = includeSchedulingDependencies; + this.includePrunedInputs = includePrunedInputs; this.actionFilters = actionFilters; this.includeParamFiles = includeParamFiles; this.includeFileWriteContents = includeFileWriteContents; @@ -150,7 +152,8 @@ private void dumpSingleAction(ConfiguredTarget configuredTarget, ActionAnalysisM getParamFileNameToContentMap().put(paramFileExecPath, fileContent); } - if (actionFilters != null && !AqueryUtils.matchesAqueryFilters(action, actionFilters)) { + if (actionFilters != null + && !AqueryUtils.matchesAqueryFilters(action, actionFilters, includePrunedInputs)) { return; } @@ -217,7 +220,7 @@ private void dumpSingleAction(ConfiguredTarget configuredTarget, ActionAnalysisM if (includeParamFiles) { // Assumption: if an Action takes a params file as an input, it will be used // to provide params to the command. - for (Artifact input : action.getInputs().toList()) { + for (Artifact input : getActionInputs(action, includePrunedInputs).toList()) { String inputFileExecPath = input.getExecPathString(); if (getParamFileNameToContentMap().containsKey(inputFileExecPath)) { AnalysisProtosV2.ParamFile paramFile = @@ -257,19 +260,13 @@ private void dumpSingleAction(ConfiguredTarget configuredTarget, ActionAnalysisM } if (includeArtifacts) { - // Store inputs - NestedSet inputs = action.getInputs(); + // Store inputs. + NestedSet inputs = getActionInputs(action, includePrunedInputs); if (!inputs.isEmpty()) { actionBuilder.addInputDepSetIds(knownNestedSets.dataToIdAndStreamOutputProto(inputs)); } - NestedSet schedulingDependencies = action.getSchedulingDependencies(); - if (includeSchedulingDependencies && !schedulingDependencies.isEmpty()) { - actionBuilder.addInputDepSetIds( - knownNestedSets.dataToIdAndStreamOutputProto(schedulingDependencies)); - } - - // store outputs + // Store outputs. for (Artifact artifact : action.getOutputs()) { actionBuilder.addOutputIds(knownArtifacts.dataToIdAndStreamOutputProto(artifact)); } diff --git a/src/main/protobuf/analysis_v2.proto b/src/main/protobuf/analysis_v2.proto index 83f420359a5a8e..89ad91c4101ff5 100644 --- a/src/main/protobuf/analysis_v2.proto +++ b/src/main/protobuf/analysis_v2.proto @@ -80,14 +80,10 @@ message Action { repeated KeyValuePair environment_variables = 7; // The set of input dep sets that the action depends upon. If the action does - // input discovery, the contents of this set might change during execution. + // input discovery and `--include_pruned_outputs` is disabled, the contents of + // this set might differ before and after execution. repeated uint32 input_dep_set_ids = 8; - // The set of scheduling dependency dep sets the action depends on. - // Only set of both --include_artifacts and --include_scheduling_dependencies - // is set to true. - repeated uint32 scheduling_dep_dep_set_ids = 20; - // The list of Artifact IDs that represent the output files that this action // will generate. repeated uint32 output_ids = 9; @@ -129,6 +125,8 @@ message Action { // If FileWrite actions should make their output executable. // (ctx.actions.write(is_executable=True)) bool is_executable = 19; + + reserved 20; } // Represents a single target (without configuration information) that is diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java index dcb5c4ec8b184f..0d2db28283f27e 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java @@ -860,6 +860,11 @@ public NestedSet getInputs() { throw new IllegalStateException(); } + @Override + public NestedSet getOriginalInputs() { + throw new IllegalStateException(); + } + @Override public NestedSet getSchedulingDependencies() { throw new IllegalStateException(); diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java index 38144fec8b88d3..dd4a5f9c3fa6c0 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java @@ -105,6 +105,11 @@ protected void setInputsDiscovered(boolean inputsDiscovered) { this.inputsDiscovered = inputsDiscovered; } + @Override + public NestedSet getOriginalInputs() { + return mandatoryInputs; + } + @Override public NestedSet getAllowedDerivedInputs() { return NestedSetBuilder.wrap(Order.STABLE_ORDER, optionalInputs); diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java index d11baababdc3d3..2f52090552d434 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java @@ -136,6 +136,11 @@ public NestedSet getInputs() { throw new UnsupportedOperationException(); } + @Override + public NestedSet getOriginalInputs() { + throw new UnsupportedOperationException(); + } + @Override public NestedSet getSchedulingDependencies() { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java index 990bb9738e2f5f..db3df56ec78cdc 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java @@ -506,6 +506,11 @@ public NestedSet getInputs() { return NestedSetBuilder.create(Order.STABLE_ORDER, inputTreeArtifact); } + @Override + public NestedSet getOriginalInputs() { + return getInputs(); + } + @Override public NestedSet getSchedulingDependencies() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index 0ce35b40b60189..ca07e206da9dbe 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -1458,6 +1458,11 @@ public NestedSet getInputs() { return NestedSetBuilder.create(Order.STABLE_ORDER, inputArtifact); } + @Override + public NestedSet getOriginalInputs() { + return getInputs(); + } + @Override public NestedSet getSchedulingDependencies() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); diff --git a/src/test/java/com/google/devtools/build/lib/testutil/FakeResourceOwner.java b/src/test/java/com/google/devtools/build/lib/testutil/FakeResourceOwner.java index 1a19428793587d..0aef5076f60ca6 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/FakeResourceOwner.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/FakeResourceOwner.java @@ -117,6 +117,11 @@ public NestedSet getInputs() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); } + @Override + public NestedSet getOriginalInputs() { + return NestedSetBuilder.emptySet(Order.STABLE_ORDER); + } + @Override public NestedSet getSchedulingDependencies() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); diff --git a/src/test/shell/integration/aquery_test.sh b/src/test/shell/integration/aquery_test.sh index 5a0d7901463c7d..38a31f51a684c2 100755 --- a/src/test/shell/integration/aquery_test.sh +++ b/src/test/shell/integration/aquery_test.sh @@ -243,33 +243,261 @@ EOF assert_not_contains "Outputs: \[" output } -function test_aquery_include_scheduling_dependencies() { +function test_prunable_headers() { local pkg="${FUNCNAME[0]}" mkdir -p "$pkg" || fail "mkdir -p $pkg" cat > "$pkg/BUILD" <<'EOF' -cc_binary(name="b", srcs=["b.cc"], deps=[":l"]) -cc_library(name="l", hdrs=["library_header.h"]) +cc_binary( + name = "foo", + srcs = ["foo.cc"], + deps = [":bar"], +) +cc_library( + name = "bar", + hdrs = ["used.h", "useless.h"], +) +EOF + cat > "$pkg/foo.cc" <<'EOF' +#include "used.h" +int main() { used(); return 0; } +EOF + cat > "$pkg/used.h" <<'EOF' +inline void used() {} +EOF + cat > "$pkg/useless.h" <<'EOF' +inline void useless() {} EOF - bazel aquery \ - --include_artifacts \ - --include_scheduling_dependencies \ - "mnemonic(CppCompile,//$pkg:b)" > output 2> "$TEST_log" \ + # Test --include_pruned_inputs before build. + # Expect used.h and useless.h to be considered inputs. + + bazel aquery "mnemonic(CppCompile,//$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_contains "Inputs:.*used.h" output + assert_contains "Inputs:.*useless.h" output + + bazel aquery "inputs(.*used.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery "inputs(.*useless.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + # Test --include_pruned_inputs after build. + # Expect used.h and useless.h to be considered inputs. + + bazel build "//$pkg:foo" || fail "Expected success" + + bazel aquery "mnemonic(CppCompile,//$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_contains "Inputs:.*used.h" output + assert_contains "Inputs:.*useless.h" output + + bazel aquery "inputs(.*used.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery "inputs(.*useless.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + if [[ "${PRODUCT_NAME}" == "bazel" ]]; then + # Include scanning not supported. + return 0 + fi + + bazel clean + + # Test --noinclude_pruned_inputs before build. + # Expect used.h and useless.h to be considered inputs. + + bazel aquery --noinclude_pruned_inputs \ + "mnemonic(CppCompile,//$pkg:foo)" > output 2> "$TEST_log" \ || fail "Expected success" cat output >> "$TEST_log" - assert_contains "SchedulingDependencies:.*library_header.h" output + assert_contains "Inputs:.*used.h" output + assert_contains "Inputs:.*useless.h" output - bazel aquery \ - --include_artifacts \ - --include_scheduling_dependencies \ - --output=jsonproto \ - "mnemonic(CppCompile,//$pkg:b)" > output 2> "$TEST_log" \ + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*used.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*useless.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + # Test --noinclude_pruned_inputs after build. + # Expect used.h to be considered an input, but not useless.h. + + bazel build "//$pkg:foo" || fail "Expected success" + + bazel aquery --noinclude_pruned_inputs \ + "mnemonic(CppCompile,//$pkg:foo)" > output 2> "$TEST_log" \ || fail "Expected success" cat output >> "$TEST_log" - assert_contains "library_header.h" output + + assert_contains "Inputs:.*used.h" output + assert_not_contains "Inputs:.*useless.h" output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*used.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*useless.h, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_empty_file output } +function test_unused_inputs() { + local pkg="${FUNCNAME[0]}" + mkdir -p "$pkg" || fail "mkdir -p $pkg" + cat > "$pkg/defs.bzl" < "$pkg/BUILD" <<'EOF' +load(":defs.bzl", "foo") +sh_binary( + name = "tool", + srcs = ["tool.sh"], +) +foo( + name = "foo", + used = "used.txt", + unused = "useless.txt", + out = "out.txt", +) +EOF + cat > "$pkg/tool.sh" <<'EOF' +#!/bin/bash +echo "$1" > "$2" +EOF + chmod +x "$pkg/tool.sh" + touch "$pkg/used.txt" "$pkg/useless.txt" + + # Test --include_pruned_inputs before build. + # Expect used.txt and useless.txt to be considered inputs. + + bazel aquery "mnemonic(Action,//$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_contains "Inputs:.*used.txt" output + assert_contains "Inputs:.*useless.txt" output + + bazel aquery "inputs(.*used.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery "inputs(.*useless.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + # Test --include_pruned_inputs after build. + # Expect used.txt and useless.txt to be considered inputs. + + bazel build "//$pkg:foo" || fail "Expected success" + + bazel aquery "mnemonic(Action,//$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_contains "Inputs:.*used.txt" output + assert_contains "Inputs:.*useless.txt" output + + bazel aquery "inputs(.*used.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery "inputs(.*useless.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel clean + + # Test --noinclude_pruned_inputs before build. + # Expect used.txt and useless.txt to be considered inputs. + + bazel aquery --noinclude_pruned_inputs \ + "mnemonic(Action,//$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + cat output >> "$TEST_log" + + assert_contains "Inputs:.*used.txt" output + assert_contains "Inputs:.*useless.txt" output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*used.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*useless.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + # Test --noinclude_pruned_inputs after build. + # Expect used.h to be considered an input, but not useless.h. + + bazel build "//$pkg:foo" || fail "Expected success" + + bazel aquery --noinclude_pruned_inputs \ + "mnemonic(Action,//$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + cat output >> "$TEST_log" + + assert_contains "Inputs:.*used.txt" output + assert_not_contains "Inputs:.*useless.txt" output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*used.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_nonempty_file output + + bazel aquery --noinclude_pruned_inputs \ + "inputs(.*useless.txt, //$pkg:foo)" > output 2> "$TEST_log" \ + || fail "Expected success" + + assert_empty_file output +} function test_aquery_starlark_env() { local pkg="${FUNCNAME[0]}" diff --git a/src/test/shell/unittest.bash b/src/test/shell/unittest.bash index 9cdc011a4bae2b..2b92e634e474d0 100644 --- a/src/test/shell/unittest.bash +++ b/src/test/shell/unittest.bash @@ -514,6 +514,36 @@ function assert_contains_n() { return 1 } +# Usage: assert_empty_file [error-message] +# Asserts that the file exists and is empty. On failure copies the file to +# undeclared outputs, prints the specified (optional) error message, and returns +# non-zero. +function assert_empty_file() { + local file=$1 + local message=${2:-"Expected '$file' to exist and be empty"} + if [[ -f "$file" && ! -s "$file" ]]; then + return 0 + fi + + fail "$message" $(__copy_to_undeclared_outputs "$1") + return 1 +} + +# Usage: assert_nonempty_file [error-message] +# Asserts that the file exists and is nonempty. On failure copies the file to +# undeclared outputs, prints the specified (optional) error message, and returns +# non-zero. +function assert_nonempty_file() { + local file=$1 + local message=${2:-"Expected '$file' to exist and be nonempty"} + if [[ -f "$file" && -s "$file" ]]; then + return 0 + fi + + fail "$message" $(__copy_to_undeclared_outputs "$1") + return 1 +} + # Copies $1 to undeclared outputs with a unique path and logs the mapping # between the original file and the new file. function __copy_to_undeclared_outputs() {