Skip to content

Commit

Permalink
Extend Android path stripping testing by integrating aquery output.
Browse files Browse the repository at this point in the history
## Description
aquery prints command lines by calling CommandAction.getArguments(): https://github.com/bazelbuild/bazel/blob/ee9727df980f268bfb2b05637d199d55963fb6d8/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java#L262.

Java commands (like JavaCompileAction) pass this to CustomCommandLine, which for path stripped actions is a PathStrippingCustomCommandLine that knows to apply path stripping appropriately. For these actions this change is a noop.

Starlark action construction (including Android actions) is more complicated: the StarlarkCustomCommandLine equivalent is set before path stripping can be known. StarlarkCustomCommandLine reads Starlark code before the action is instantiated and its full inputs and outputs are known. We need that info to set path stripping.

So when Bazel requests a Starlark action's command, it passes the stripPath bit as a parameter to StarlarkCustomCommandLine's arguments() call (see this cl's change to SpawnAction - that calls CommandLines.allArguments() which calls StarlarkCustomCommandLine.arguments()).

This change ensures that call properly passes the bit.

This lets config_stripped_outputs_test use aquery to identify path stripping in more actions, so more tests are added here.

## Wider API consequences
This change affects *any* caller to SpawnAction.getArguments(). This includes callers like ActionExecutedEvent: https://github.com/bazelbuild/bazel/blob/bc2f0519a4e6f0ac3ea42ff50da2c9919c5b6c0e/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java#L250. It's unclear if we want this caller to strip paths.

It also affects some action key computations, like https://github.com/bazelbuild/bazel/blob/cb01bde9369f8197cb54c34b14aa41f0dcc903cf/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java#L225. This seems safe according to the contract for action keys: https://github.com/bazelbuild/bazel/blob/e4c1c434d49062449c7a83dd753fe01923766b1d/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java#L64-L84. Note that the affected callers are noops since those actions don't use path stripping.

There are also a few debugging callers, similar in spirit to aquery.

All this is more complicated than I'd prefer (i.e. the different logic for constructing Java vs. Starlark actions). I'd consider better API consistency down the line but for now I think the current balance is manageable.

bazelbuild#6526

PiperOrigin-RevId: 445970260
  • Loading branch information
gregestren authored and copybara-github committed May 2, 2022
1 parent ecc293d commit 9ad0a73
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,15 @@ ExpandedCommandLines expand(
*/
public ImmutableList<String> allArguments()
throws CommandLineExpansionException, InterruptedException {
return allArguments(CommandAdjuster.NOOP);
}

/** Variation of {@link #allArguments()} that supports output path stripping. */
public ImmutableList<String> allArguments(CommandAdjuster stripPaths)
throws CommandLineExpansionException, InterruptedException {
ImmutableList.Builder<String> arguments = ImmutableList.builder();
for (CommandLineAndParamFileInfo pair : getCommandLines()) {
arguments.addAll(pair.commandLine.arguments());
arguments.addAll(pair.commandLine.arguments(/*artifactExpander=*/ null, stripPaths));
}
return arguments.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1291,9 +1291,9 @@ public ImmutableList<String> arguments()
}

@Override
public ImmutableList<String> arguments(ArtifactExpander artifactExpander)
public ImmutableList<String> arguments(@Nullable ArtifactExpander artifactExpander)
throws CommandLineExpansionException, InterruptedException {
return argumentsInternal(Preconditions.checkNotNull(artifactExpander));
return argumentsInternal(artifactExpander);
}

private ImmutableList<String> argumentsInternal(@Nullable ArtifactExpander artifactExpander)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.ParamFileInfo;
import com.google.devtools.build.lib.actions.PathStripper;
import com.google.devtools.build.lib.actions.PathStripper.CommandAdjuster;
import com.google.devtools.build.lib.actions.ResourceSetOrBuilder;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.Spawn;
Expand Down Expand Up @@ -250,9 +250,16 @@ public CommandLines getCommandLines() {
return commandLines;
}

private CommandAdjuster getPathStripper() {
return CommandAdjuster.create(
stripOutputPaths,
this instanceof StarlarkAction ? getMnemonic() : null,
getPrimaryOutput().getExecPath().subFragment(0, 1));
}

@Override
public List<String> getArguments() throws CommandLineExpansionException, InterruptedException {
return ImmutableList.copyOf(commandLines.allArguments());
return commandLines.allArguments(getPathStripper());
}

@Override
Expand Down Expand Up @@ -421,10 +428,7 @@ protected Spawn getSpawn(
commandLines.expand(
artifactExpander,
getPrimaryOutput().getExecPath(),
PathStripper.CommandAdjuster.create(
stripOutputPaths,
this instanceof StarlarkAction ? getMnemonic() : null,
getPrimaryOutput().getExecPath().subFragment(0, 1)),
getPathStripper(),
commandLineLimits);
return new ActionSpawn(
ImmutableList.copyOf(expandedCommandLines.arguments()),
Expand Down

0 comments on commit 9ad0a73

Please sign in to comment.