Skip to content

Commit

Permalink
[7.1.0] Use execution info instead of hard-coded mnemonics for Java p…
Browse files Browse the repository at this point in the history
…ath mapping (#21461)

By removing Java rules from the hard-coded mnemonics allowlist for path
mapping, users can rely on `--modify_execution_info` to selectively
disable path mapping for them just like for Starlark actions.

This requires fixing two minor inconsistencies in how execution info is
populated for Java actions:
* In `JavaCompilationHelper`, `buildKeepingLast` is used instead of
`buildOrThrow` to prevent a crash when a target sets
`supports-path-mapping` via `tags`.
* In `JavaHeaderCompileAction`, `--experimental_inmemory_jdeps_files` no
longer causes all other execution info to be lost.

Fixes #21092

Closes #21093.

Commit
f8337c7

PiperOrigin-RevId: 609064092
Change-Id: I6803e34a6861f19d185542e707b00029ee018a0a

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
  • Loading branch information
iancha1992 and fmeum committed Feb 21, 2024
1 parent 22948f3 commit f788fed
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 23 deletions.
Expand Up @@ -36,7 +36,8 @@
* PathMapper}).
*/
public final class PathMappers {
// TODO: Replace with a command-line flag.
// TODO: Remove actions from this list by adding ExecutionRequirements.SUPPORTS_PATH_MAPPING to
// their execution info instead.
private static final ImmutableSet<String> SUPPORTED_MNEMONICS =
ImmutableSet.of(
"AndroidLint",
Expand All @@ -45,8 +46,6 @@ public final class PathMappers {
"DejetifySrcs",
"Desugar",
"DexBuilder",
"Javac",
"JavacTurbine",
"Jetify",
"JetifySrcs",
"LinkAndroidResources",
Expand All @@ -56,8 +55,6 @@ public final class PathMappers {
"StarlarkAARGenerator",
"StarlarkMergeCompiledAndroidResources",
"StarlarkRClassGenerator",
"Turbine",
"JavaResourceJar",
"Mock action");

/**
Expand Down
Expand Up @@ -418,24 +418,26 @@ private boolean separateResourceJar(
}

private ImmutableMap<String, String> getExecutionInfo() throws RuleErrorException {
ImmutableMap.Builder<String, String> executionInfo = ImmutableMap.builder();
ImmutableMap.Builder<String, String> workerInfo = ImmutableMap.builder();
ImmutableMap.Builder<String, String> modifiableExecutionInfo = ImmutableMap.builder();
modifiableExecutionInfo.put(ExecutionRequirements.SUPPORTS_PATH_MAPPING, "1");
if (javaToolchain.getJavacSupportsWorkers()) {
workerInfo.put(ExecutionRequirements.SUPPORTS_WORKERS, "1");
modifiableExecutionInfo.put(ExecutionRequirements.SUPPORTS_WORKERS, "1");
}
if (javaToolchain.getJavacSupportsMultiplexWorkers()) {
workerInfo.put(ExecutionRequirements.SUPPORTS_MULTIPLEX_WORKERS, "1");
modifiableExecutionInfo.put(ExecutionRequirements.SUPPORTS_MULTIPLEX_WORKERS, "1");
}
if (javaToolchain.getJavacSupportsWorkerCancellation()) {
workerInfo.put(ExecutionRequirements.SUPPORTS_WORKER_CANCELLATION, "1");
modifiableExecutionInfo.put(ExecutionRequirements.SUPPORTS_WORKER_CANCELLATION, "1");
}
ImmutableMap.Builder<String, String> executionInfo = ImmutableMap.builder();
executionInfo.putAll(
getConfiguration()
.modifiedExecutionInfo(workerInfo.buildOrThrow(), JavaCompileActionBuilder.MNEMONIC));
.modifiedExecutionInfo(
modifiableExecutionInfo.buildOrThrow(), JavaCompileActionBuilder.MNEMONIC));
executionInfo.putAll(
TargetUtils.getExecutionInfo(ruleContext.getRule(), ruleContext.isAllowTagsPropagation()));

return executionInfo.buildOrThrow();
return executionInfo.buildKeepingLast();
}

/** Returns the bootclasspath explicit set in attributes if present, or else the default. */
Expand Down
Expand Up @@ -171,8 +171,6 @@ public JavaCompileAction(
}
this.tools = tools;
this.compilationType = compilationType;
// TODO(djasper): The only thing that is conveyed through the executionInfo is whether worker
// mode is enabled or not. Investigate whether we can store just that.
this.executionInfo =
configuration.modifiedExecutionInfo(executionInfo, compilationType.mnemonic);
this.executableLine = executableLine;
Expand Down
Expand Up @@ -77,6 +77,8 @@
*/
public final class JavaHeaderCompileAction extends SpawnAction {

private static final String DIRECT_CLASSPATH_MNEMONIC = "Turbine";

private final boolean insertDependencies;
private final NestedSet<Artifact> additionalArtifactsForPathMapping;

Expand Down Expand Up @@ -466,14 +468,22 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
}
}

ImmutableMap<String, String> executionInfo =
TargetUtils.getExecutionInfo(ruleContext.getRule(), ruleContext.isAllowTagsPropagation());
ImmutableMap.Builder<String, String> executionInfo = ImmutableMap.builder();
executionInfo.putAll(
ruleContext
.getConfiguration()
.modifiedExecutionInfo(
ImmutableMap.of(ExecutionRequirements.SUPPORTS_PATH_MAPPING, "1"),
JavaCompileActionBuilder.MNEMONIC));
executionInfo.putAll(
TargetUtils.getExecutionInfo(
ruleContext.getRule(), ruleContext.isAllowTagsPropagation()));
if (javaConfiguration.inmemoryJdepsFiles()) {
executionInfo =
ImmutableMap.of(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString());
executionInfo.put(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString());
}

if (useDirectClasspath) {
NestedSet<Artifact> classpath;
NestedSet<Artifact> additionalArtifactsForPathMapping;
Expand Down Expand Up @@ -513,10 +523,12 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
/* env= */ actionEnvironment,
/* executionInfo= */ ruleContext
.getConfiguration()
.modifiedExecutionInfo(executionInfo, "Turbine"),
.modifiedExecutionInfo(
executionInfo.buildKeepingLast(), DIRECT_CLASSPATH_MNEMONIC),
/* progressMessage= */ progressMessage,
/* runfilesSupplier= */ EmptyRunfilesSupplier.INSTANCE,
/* mnemonic= */ "Turbine",

/* mnemonic= */ DIRECT_CLASSPATH_MNEMONIC,
/* outputPathsMode= */ PathMappers.getOutputPathsMode(
ruleContext.getConfiguration()),
// If classPathMode == BAZEL, also make sure to inject the dependencies to be
Expand Down Expand Up @@ -563,7 +575,7 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
/* transitiveInputs= */ classpathEntries,
/* directJars= */ directJars,
/* outputs= */ outputs.build(),
/* executionInfo= */ executionInfo,
/* executionInfo= */ executionInfo.buildKeepingLast(),
/* extraActionInfoSupplier= */ null,
/* executableLine= */ executableLine,
/* flagLine= */ commandLine.build(),
Expand Down
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ParamFileInfo;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.analysis.RuleContext;
Expand All @@ -39,6 +40,8 @@ public class ResourceJarActionBuilder {

private static final ParamFileInfo PARAM_FILE_INFO =
ParamFileInfo.builder(ParameterFileType.SHELL_QUOTED).build();
private static final ImmutableMap<String, String> EXECUTION_INFO =
ImmutableMap.of(ExecutionRequirements.SUPPORTS_PATH_MAPPING, "1");

private Artifact outputJar;
private Map<PathFragment, Artifact> resources = ImmutableMap.of();
Expand Down Expand Up @@ -125,6 +128,7 @@ public void build(JavaSemantics semantics, RuleContext ruleContext, String execG
.addCommandLine(command.build(), PARAM_FILE_INFO)
.setProgressMessage("Building Java resource jar")
.setMnemonic(MNEMONIC)
.setExecutionInfo(EXECUTION_INFO)
.setExecGroup(execGroup)
.build(ruleContext));
}
Expand Down

0 comments on commit f788fed

Please sign in to comment.