Skip to content

Commit

Permalink
Report prunable inputs in aquery output.
Browse files Browse the repository at this point in the history
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.

bazelbuild@6b912c5 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 bazelbuild#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
  • Loading branch information
tjgq authored and copybara-github committed Sep 4, 2024
1 parent 4ab411c commit 4c4bbec
Show file tree
Hide file tree
Showing 28 changed files with 430 additions and 102 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ public final boolean inputsKnown() {
/**
* {@inheritDoc}
*
* <p>Should be overridden along with {@link #discoverInputs}, {@link #inputsDiscovered}, and
* {@link #setInputsDiscovered} by actions that do input discovery.
* <p>Should be overridden along with {@link #discoverInputs}, {@link #inputsDiscovered}, {@link
* #setInputsDiscovered} and {@link #getOriginalInputs} by actions that do input discovery.
*/
@Override
public boolean discoversInputs() {
Expand All @@ -137,8 +137,7 @@ public boolean discoversInputs() {
@Nullable
public NestedSet<Artifact> 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
Expand All @@ -147,12 +146,9 @@ public final void resetDiscoveredInputs() {
if (!inputsKnown()) {
return;
}
NestedSet<Artifact> originalInputs = getOriginalInputs();
if (originalInputs != null) {
synchronized (this) {
inputs = originalInputs;
setInputsDiscovered(false);
}
synchronized (this) {
inputs = getOriginalInputs();
setInputsDiscovered(false);
}
}

Expand All @@ -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);
}

/**
Expand All @@ -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 <em>original</em> inputs, prior to {@linkplain #discoverInputs input
* discovery}.
*
* <p>Input-discovering actions which are able to reconstitute their original inputs may override
* this, allowing for memory savings.
*/
@Nullable
@ForOverride
protected NestedSet<Artifact> getOriginalInputs() {
return null;
@Override
public NestedSet<Artifact> getOriginalInputs() {
checkState(!discoversInputs(), "Must be overridden by input-discovering action");
return getInputs();
}

@Override
public NestedSet<Artifact> 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
Expand All @@ -217,7 +206,7 @@ public NestedSet<Artifact> getSchedulingDependencies() {
*/
@Override
public synchronized void updateInputs(NestedSet<Artifact> 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);
}
Expand Down Expand Up @@ -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<Artifact> originalInputs = getOriginalInputs();
NestedSet<Artifact> inputs = originalInputs == null ? getInputs() : originalInputs;
return Iterables.getFirst(inputs.toList(), null);
return Iterables.getFirst(getOriginalInputs().toList(), null);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,19 @@ String getKey(ActionKeyContext actionKeyContext, @Nullable ArtifactExpander arti
/**
* Returns the input Artifacts that this Action depends upon. May be empty.
*
* <p>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.
* <p>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<Artifact> getInputs();

/**
* Returns this action's original inputs prior to input discovery.
*
* <p>Unlike {@link #getInputs}, the same result is returned before and after action execution.
*/
NestedSet<Artifact> 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.
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ public NestedSet<Artifact> getInputs() {
return allInputs;
}

@Override
public NestedSet<Artifact> getOriginalInputs() {
return getInputs();
}

@Override
public NestedSet<Artifact> getSchedulingDependencies() {
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ public boolean discoversInputs() {
}

@Override
protected NestedSet<Artifact> getOriginalInputs() {
public NestedSet<Artifact> getOriginalInputs() {
return allStarlarkActionInputs;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecution
Order.STABLE_ORDER, Sets.difference(getInputs().toSet(), oldInputs.toSet()));
}

@Override
public NestedSet<Artifact> getOriginalInputs() {
return shadowedAction.getOriginalInputs();
}

@Override
public NestedSet<Artifact> getSchedulingDependencies() {
return shadowedAction.getSchedulingDependencies();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public BlazeCommandResult dumpActionGraphFromSkyframe(CommandEnvironment env) {
new ActionGraphDump(
aqueryOptions.includeCommandline,
aqueryOptions.includeArtifacts,
aqueryOptions.includeSchedulingDependencies,
aqueryOptions.includePrunedInputs,
actionFilters,
aqueryOptions.includeParamFiles,
aqueryOptions.includeFileWriteContents,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ private void dumpSkyframeStateAfterBuild(
new ActionGraphDump(
/* includeActionCmdLine= */ false,
/* includeArtifacts= */ true,
/* includeSchedulingDependencies= */ true,
/* includePrunedInputs= */ true,
/* actionFilters= */ null,
/* includeParamFiles= */ false,
/* includeFileWriteContents= */ false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ public NestedSet<Artifact> getInputs() {
return actionExecutionMetadata.getInputs();
}

@Override
public NestedSet<Artifact> getOriginalInputs() {
return actionExecutionMetadata.getInputs();
}

@Override
public NestedSet<Artifact> getSchedulingDependencies() {
throw new UnsupportedOperationException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public void processOutput(Iterable<ConfiguredTargetValue> partialResult)
}

private void processAction(ActionAnalysisMetadata action) throws InterruptedException {
if (!AqueryUtils.matchesAqueryFilters(action, actionFilters)) {
if (!AqueryUtils.matchesAqueryFilters(action, actionFilters, options.includePrunedInputs)) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -199,26 +201,17 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream)
}

if (options.includeArtifacts) {
NestedSet<Artifact> 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(
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>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<Artifact> 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.<Artifact>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<Artifact> inputs = action.getInputs();
ActionAnalysisMetadata action,
AqueryActionFilter actionFilters,
boolean includePrunedInputs) {
NestedSet<Artifact> inputs = getActionInputs(action, includePrunedInputs);
Iterable<Artifact> outputs = action.getOutputs();
String mnemonic = action.getMnemonic();

Expand All @@ -49,7 +82,7 @@ public static boolean matchesAqueryFilters(
}

if (actionFilters.hasFilterForFunction(INPUTS)) {
Boolean containsFile =
boolean containsFile =
inputs.toList().stream()
.anyMatch(
artifact ->
Expand All @@ -62,7 +95,7 @@ public static boolean matchesAqueryFilters(
}

if (actionFilters.hasFilterForFunction(OUTPUTS)) {
Boolean containsFile =
boolean containsFile =
Streams.stream(outputs)
.anyMatch(
artifact ->
Expand Down
Loading

0 comments on commit 4c4bbec

Please sign in to comment.