From 9f7aaf5c092128fd2a2feea69ae5ab99c95bf0e5 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 15 Aug 2024 13:03:42 +0200 Subject: [PATCH] WIP: Don't overwrite jdeps --- .../build/lib/actions/AbstractAction.java | 2 +- .../devtools/build/lib/actions/BaseSpawn.java | 2 +- .../analysis/actions/CustomCommandLine.java | 12 +- .../lib/analysis/actions/PathMappers.java | 5 +- .../lib/analysis/actions/SpawnAction.java | 2 +- .../devtools/build/lib/rules/java/BUILD | 1 + .../lib/rules/java/JavaCompileAction.java | 124 +++++++++++++----- .../rules/java/JavaCompileActionBuilder.java | 1 - .../rules/java/JavaHeaderCompileAction.java | 1 + 9 files changed, 101 insertions(+), 49 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..9754906baeb79f 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 @@ -668,7 +668,7 @@ public PlatformInfo getExecutionPlatform() { * Returns artifacts that should be subject to path mapping (see {@link Spawn#getPathMapper()}, * but aren't inputs of the action. */ - public NestedSet getAdditionalArtifactsForPathMapping() { + public NestedSet getAdditionalArtifactsForPathMapping() { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java index d11f81c6184dd9..bce88ac5867421 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java @@ -81,7 +81,7 @@ public NestedSet getInputFiles() { } @Override - public Collection getOutputFiles() { + public Collection getOutputFiles() { return action.getOutputs(); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index cec01d75d6e8b3..a4605197271e21 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -864,26 +864,26 @@ public Builder addPath(@CompileTimeConstant String arg, @Nullable PathFragment v } /** - * Adds an artifact by calling {@link Artifact#getExecPath}. + * Adds an artifact by calling {@link ActionInput#getExecPath}. * - *

Prefer this over manually calling {@link Artifact#getExecPath}, as it avoids storing a + *

Prefer this over manually calling {@link ActionInput#getExecPath}, as it avoids storing a * copy of the artifact path string. */ @CanIgnoreReturnValue - public Builder addExecPath(@Nullable Artifact value) { + public Builder addExecPath(@Nullable ActionInput value) { return addObjectInternal(value); } /** - * Adds an artifact by calling {@link Artifact#getExecPath}. + * Adds an artifact by calling {@link ActionInput#getExecPath}. * - *

Prefer this over manually calling {@link Artifact#getExecPath}, as it avoids storing a + *

Prefer this over manually calling {@link ActionInput#getExecPath}, as it avoids storing a * copy of the artifact path string. * *

If the value is null, neither the arg nor the value is added. */ @CanIgnoreReturnValue - public Builder addExecPath(@CompileTimeConstant String arg, @Nullable Artifact value) { + public Builder addExecPath(@CompileTimeConstant String arg, @Nullable ActionInput value) { return addObjectInternal(arg, value); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java index f64be1edebc72c..ae4e81773e1405 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java @@ -14,12 +14,11 @@ package com.google.devtools.build.lib.analysis.actions; - import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionKeyContext; -import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactExpander; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineLimits; @@ -118,7 +117,7 @@ public final class PathMappers { public static void addToFingerprint( String mnemonic, Map executionInfo, - NestedSet additionalArtifactsForPathMapping, + NestedSet additionalArtifactsForPathMapping, ActionKeyContext actionKeyContext, OutputPathsMode outputPathsMode, Fingerprint fingerprint) 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 d857f13e59105b..bd6bc5a218ea52 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 @@ -575,7 +575,7 @@ public NestedSet getInputFiles() { } @Override - public Collection getOutputFiles() { + public Collection getOutputFiles() { return reportOutputs ? super.getOutputFiles() : ImmutableSet.of(); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD index d151967d0220a1..9d1e4049420acd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD @@ -101,6 +101,7 @@ java_library( ], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", "//src/main/java/com/google/devtools/build/lib/actions:artifact_expander", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 06068df7d8c76d..e3855fa86f887e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.java; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.flogger.LazyArgs.lazy; import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps; @@ -38,6 +39,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; @@ -56,7 +58,6 @@ import com.google.devtools.build.lib.actions.ParameterFile; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.ResourceSet; -import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; @@ -86,6 +87,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -296,6 +298,10 @@ static class ReducedClasspath { } } + static ActionInput getMappedOutputDepsProto(Artifact outputDepsProto) { + return ActionInputHelper.fromPath(outputDepsProto.getExecPathString() + ".mapped"); + } + private JavaSpawn getReducedSpawn( ActionExecutionContext actionExecutionContext, ReducedClasspath reducedClasspath, @@ -305,6 +311,8 @@ private JavaSpawn getReducedSpawn( PathMapper pathMapper = PathMappers.create( this, PathMappers.getOutputPathsMode(configuration), /* isStarlarkAction= */ false); + ActionInput spawnOutputDepsProto = getMappedOutputDepsProto(outputDepsProto); + classpathLine.addExecPath("--output_deps_proto", spawnOutputDepsProto); if (fallback) { classpathLine.addExecPaths("--classpath", transitiveInputs); @@ -336,12 +344,24 @@ private JavaSpawn getReducedSpawn( .addTransitive(mandatoryInputs) .addTransitive(fallback ? transitiveInputs : reducedClasspath.reducedJars) .build(); + var outputs = + getOutputs().stream() + .map( + output -> { + if (output.equals(outputDepsProto)) { + return spawnOutputDepsProto; + } + return output; + }) + .collect(toImmutableList()); return new JavaSpawn( expandedCommandLines, getEffectiveEnvironment(actionExecutionContext.getClientEnv()), getExecutionInfo(), inputs, - /* onlyMandatoryOutput= */ fallback ? null : outputDepsProto, + outputs, + spawnOutputDepsProto, + /* outputDepsProtoIsOnlyMandatoryOutput= */ !fallback, pathMapper); } @@ -374,7 +394,9 @@ private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) // possible by stripping prefixes on the executor. .addTransitive(dependencyArtifacts) .build(), - /* onlyMandatoryOutput= */ null, + getOutputs(), + outputDepsProto, + /* outputDepsProtoIsOnlyMandatoryOutput= */ false, pathMapper); } @@ -398,7 +420,7 @@ e, createFailureDetail(message, Code.REDUCED_CLASSPATH_FALLBACK_CLEANUP_FAILURE) public ActionResult execute(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { ReducedClasspath reducedClasspath; - Spawn spawn; + JavaSpawn spawn; try { if (classpathMode == JavaClasspathMode.BAZEL) { JavaCompileActionContext context = @@ -432,7 +454,11 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) } Deps.Dependencies dependencies = - readFullOutputDeps(primaryResults, actionExecutionContext, spawn.getPathMapper()); + readFullOutputDeps( + primaryResults, + actionExecutionContext, + spawn.getPathMapper(), + spawn.getOutputDepsProto()); if (compilationType == CompilationType.TURBINE) { actionExecutionContext @@ -489,13 +515,21 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) .getContext(JavaCompileActionContext.class) .insertDependencies( outputDepsProto, - readFullOutputDeps(fallbackResults, actionExecutionContext, spawn.getPathMapper())); + readFullOutputDeps( + fallbackResults, + actionExecutionContext, + spawn.getPathMapper(), + spawn.getOutputDepsProto())); } else if (!spawn.getPathMapper().isNoop()) { // As a side effect, readFullOutputDeps rewrites the on-disk .jdeps file from mapped to // unmapped paths. To make path mapping fully transparent to consumers of this action's // output, we ensure that the file always contains unmapped paths. var unused = - readFullOutputDeps(fallbackResults, actionExecutionContext, spawn.getPathMapper()); + readFullOutputDeps( + fallbackResults, + actionExecutionContext, + spawn.getPathMapper(), + spawn.getOutputDepsProto()); } return ActionResult.create( ImmutableList.copyOf(Iterables.concat(primaryResults, fallbackResults))); @@ -595,15 +629,19 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont private final class JavaSpawn extends BaseSpawn { private final NestedSet inputs; - private final Artifact onlyMandatoryOutput; + private final ImmutableList outputs; + private final ActionInput outputDepsProto; private final PathMapper pathMapper; + private final boolean outputDepsProtoIsOnlyMandatoryOutput; JavaSpawn( CommandLines.ExpandedCommandLines expandedCommandLines, Map environment, Map executionInfo, NestedSet inputs, - @Nullable Artifact onlyMandatoryOutput, + Collection outputs, + ActionInput outputDepsProto, + boolean outputDepsProtoIsOnlyMandatoryOutput, PathMapper pathMapper) { super( expandedCommandLines.arguments(), @@ -611,11 +649,13 @@ private final class JavaSpawn extends BaseSpawn { executionInfo, JavaCompileAction.this, LOCAL_RESOURCES); - this.onlyMandatoryOutput = onlyMandatoryOutput; + this.outputDepsProto = outputDepsProto; + this.outputDepsProtoIsOnlyMandatoryOutput = outputDepsProtoIsOnlyMandatoryOutput; this.inputs = NestedSetBuilder.fromNestedSet(inputs) .addAll(expandedCommandLines.getParamFiles()) .build(); + this.outputs = ImmutableList.copyOf(outputs); this.pathMapper = pathMapper; } @@ -624,15 +664,24 @@ public NestedSet getInputFiles() { return inputs; } + @Override + public Collection getOutputFiles() { + return outputs; + } + @Override public boolean isMandatoryOutput(ActionInput output) { - return onlyMandatoryOutput == null || onlyMandatoryOutput.equals(output); + return !outputDepsProtoIsOnlyMandatoryOutput || outputDepsProto.equals(output); } @Override public PathMapper getPathMapper() { return pathMapper; } + + public ActionInput getOutputDepsProto() { + return outputDepsProto; + } } @VisibleForTesting @@ -709,6 +758,15 @@ public NestedSet getPossibleInputsForTesting() { return null; } + @Override + public NestedSet getAdditionalArtifactsForPathMapping() { + NestedSetBuilder builder = NestedSetBuilder.stableOrder(); + if (outputDepsProto != null) { + builder.add(getMappedOutputDepsProto(outputDepsProto)); + } + return builder.build(); + } + /** * Locally rewrites a .jdeps file to replace missing config prefixes. * @@ -738,6 +796,7 @@ public NestedSet getPossibleInputsForTesting() { * * @param spawnResult the executor action that created the possibly stripped .jdeps output * @param outputDepsProto path to the .jdeps output + * @param mappedOutputDepsProto path to the .jdeps.mapped output of the spawn * @param actionInputs all inputs to the current action * @param additionalArtifactsForPathMapping any additional artifacts that may be referenced in the * .jdeps file by path @@ -747,29 +806,28 @@ public NestedSet getPossibleInputsForTesting() { static Deps.Dependencies createFullOutputDeps( SpawnResult spawnResult, Artifact outputDepsProto, + ActionInput mappedOutputDepsProto, NestedSet actionInputs, - NestedSet additionalArtifactsForPathMapping, + NestedSet additionalArtifactsForPathMapping, ActionExecutionContext actionExecutionContext, PathMapper pathMapper) throws IOException { Deps.Dependencies executorJdeps = - readExecutorJdeps(spawnResult, outputDepsProto, actionExecutionContext); + readExecutorJdeps(spawnResult, mappedOutputDepsProto, actionExecutionContext); - if (pathMapper.isNoop()) { - return executorJdeps; - } - - // No paths to rewrite. - if (executorJdeps.getDependencyCount() == 0) { + if (pathMapper.isNoop() || executorJdeps.getDependencyCount() == 0) { + actionExecutionContext + .getInputPath(outputDepsProto) + .createSymbolicLink(actionExecutionContext.getInputPath(mappedOutputDepsProto)); return executorJdeps; } // For each of the action's generated inputs, revert its mapped path back to its original path. BiMap mappedToOriginalPath = HashBiMap.create(); - for (Artifact actionInput : + for (ActionInput actionInput : Iterables.concat(actionInputs.toList(), additionalArtifactsForPathMapping.toList())) { - if (actionInput.isSourceArtifact()) { + if (actionInput instanceof Artifact artifact && artifact.isSourceArtifact()) { continue; } String mappedPath = pathMapper.getMappedExecPathString(actionInput); @@ -802,7 +860,7 @@ static Deps.Dependencies createFullOutputDeps( String.format( "Missing original path for mapped path %s in %s%njdeps: %s%npath map: %s", pathOnExecutor, - outputDepsProto.getExecPath(), + mappedOutputDepsProto.getExecPath(), executorJdeps, mappedToOriginalPath)); } @@ -811,19 +869,11 @@ static Deps.Dependencies createFullOutputDeps( } Deps.Dependencies fullOutputDeps = fullDepsBuilder.build(); - // Write the updated proto back to the filesystem. If the executor produced in-memory-only - // outputs (see getInMemoryOutput above), the filesystem version doesn't exist and we can skip - // this. Note that in-memory and filesystem outputs aren't necessarily mutually exclusive. + // Write the updated proto back to the filesystem. Path fsPath = actionExecutionContext.getInputPath(outputDepsProto); - if (fsPath.exists()) { - // Make sure to clear the output store cache if it has an entry from before the rewrite. - actionExecutionContext - .getOutputMetadataStore() - .resetOutputs(ImmutableList.of(outputDepsProto)); - fsPath.setWritable(true); - try (var outputStream = fsPath.getOutputStream()) { - fullOutputDeps.writeTo(outputStream); - } + fsPath.setWritable(true); + try (var outputStream = fsPath.getOutputStream()) { + fullOutputDeps.writeTo(outputStream); } return fullOutputDeps; @@ -831,7 +881,7 @@ static Deps.Dependencies createFullOutputDeps( private static Deps.Dependencies readExecutorJdeps( SpawnResult spawnResult, - Artifact outputDepsProto, + ActionInput outputDepsProto, ActionExecutionContext actionExecutionContext) throws IOException { InputStream inMemoryOutput = spawnResult.getInMemoryOutput(outputDepsProto); @@ -847,13 +897,15 @@ private static Deps.Dependencies readExecutorJdeps( private Deps.Dependencies readFullOutputDeps( List results, ActionExecutionContext actionExecutionContext, - PathMapper pathMapper) + PathMapper pathMapper, + ActionInput mappedOutputDepsProto) throws ActionExecutionException { SpawnResult result = Iterables.getOnlyElement(results); try { return createFullOutputDeps( result, outputDepsProto, + mappedOutputDepsProto, getInputs(), getAdditionalArtifactsForPathMapping(), actionExecutionContext, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index f27ec299aa0675..2c00d9246bd7e1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -282,7 +282,6 @@ private CustomCommandLine buildParamFileContents(ImmutableList javacOpts if (compressJar) { result.add("--compress_jar"); } - result.addExecPath("--output_deps_proto", outputs.depsProto()); result.addExecPaths("--bootclasspath", bootClassPath.bootclasspath()); if (bootClassPath.systemPath().isPresent()) { result.addPath("--system", bootClassPath.systemPath().get()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java index 4b1e299dbc80d1..db51982ca082c9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java @@ -143,6 +143,7 @@ protected void afterExecute( JavaCompileAction.createFullOutputDeps( spawnResult, outputDepsProto, + JavaCompileAction.getMappedOutputDepsProto(outputDepsProto), getInputs(), getAdditionalArtifactsForPathMapping(), context,