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 b782561fa03cce..06dc062e51d573 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 @@ -661,4 +661,12 @@ public ImmutableMap getExecProperties() { public PlatformInfo getExecutionPlatform() { return owner.getExecutionPlatform(); } + + /** + * Returns artifacts that should be subject to path mapping (see {@link Spawn#getPathMapper()}, + * but aren't inputs of the action. + */ + public NestedSet getAdditionalArtifactsForPathMapping() { + return NestedSetBuilder.emptySet(Order.STABLE_ORDER); + } } 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 3ebba4796c5213..f0d52883152cef 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 @@ -15,6 +15,7 @@ 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.ActionKeyContext; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; @@ -63,8 +64,8 @@ public final class PathMappers { * Actions that support path mapping should call this method from {@link * Action#getKey(ActionKeyContext, ArtifactExpander)}. * - *

Compared to {@link #create(Action, OutputPathsMode)}, this method does not flatten nested - * sets and thus can't result in memory regressions. + *

Compared to {@link #create(AbstractAction, OutputPathsMode)}, this method does not flatten + * nested sets and thus can't result in memory regressions. * * @param mnemonic the mnemonic of the action * @param executionInfo the execution info of the action @@ -103,22 +104,12 @@ public static void addToFingerprint( * OutputPathsMode, Fingerprint)} from {@link Action#getKey(ActionKeyContext, ArtifactExpander)} * to ensure correct incremental builds. * - * @param action the {@link Action} for which a {@link Spawn} is to be created + * @param action the {@link AbstractAction} for which a {@link Spawn} is to be created * @param outputPathsMode the value of {@link CoreOptions#outputPathsMode} * @return a {@link PathMapper} that maps paths of the action's inputs and outputs. May be {@link * PathMapper#NOOP} if path mapping is not applicable to the action. */ - public static PathMapper create(Action action, OutputPathsMode outputPathsMode) { - if (getEffectiveOutputPathsMode( - outputPathsMode, action.getMnemonic(), action.getExecutionInfo()) - != OutputPathsMode.STRIP) { - return PathMapper.NOOP; - } - return StrippingPathMapper.tryCreate(action).orElse(PathMapper.NOOP); - } - - public static PathMapper createPathMapperForTesting( - Action action, OutputPathsMode outputPathsMode) { + public static PathMapper create(AbstractAction action, OutputPathsMode outputPathsMode) { if (getEffectiveOutputPathsMode( outputPathsMode, action.getMnemonic(), action.getExecutionInfo()) != OutputPathsMode.STRIP) { @@ -128,8 +119,8 @@ public static PathMapper createPathMapperForTesting( } /** - * Helper method to simplify calling {@link #create(Action, OutputPathsMode)} for actions that - * store the configuration directly. + * Helper method to simplify calling {@link #create(SpawnAction, OutputPathsMode)} for actions + * that store the configuration directly. * * @param configuration the configuration * @return the value of diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java index cfdd6d54b8e475..a5952a1e712284 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java @@ -15,6 +15,8 @@ package com.google.devtools.build.lib.analysis.actions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +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.Artifact.DerivedArtifact; @@ -85,10 +87,15 @@ public final class StrippingPathMapper { * @param action the action to potentially strip paths from * @return a {@link StrippingPathMapper} if the action supports it, else {@link Optional#empty()}. */ - static Optional tryCreate(Action action) { + static Optional tryCreate(AbstractAction action) { // This is expected to always be "bazel-out", but we don't want to hardcode it here. PathFragment outputRoot = action.getPrimaryOutput().getExecPath().subFragment(0, 1); - if (isPathStrippable(action.getInputs(), outputRoot)) { + // Additional artifacts to map are not part of the action's inputs, but may still lead to + // path collisions after stripping. It is thus important to include them in this check. + if (isPathStrippable( + Iterables.concat( + action.getInputs().toList(), action.getAdditionalArtifactsForPathMapping().toList()), + outputRoot)) { return Optional.of( create(action.getMnemonic(), action instanceof StarlarkAction, outputRoot)); } @@ -241,7 +248,7 @@ private static boolean isOutputPath(PathFragment pathFragment, PathFragment outp *

This method checks b). */ private static boolean isPathStrippable( - NestedSet actionInputs, PathFragment outputRoot) { + Iterable actionInputs, PathFragment outputRoot) { // For qualifying action types, check that no inputs or outputs would clash if paths were // removed, e.g. "bazel-out/k8-fastbuild/foo" and "bazel-out/host/foo". // @@ -253,7 +260,7 @@ private static boolean isPathStrippable( // caching the same action in host and target configurations. This could be mitigated by // stripping the host prefix *only* when the entire action is in the host configuration. HashSet rootRelativePaths = new HashSet<>(); - for (ActionInput input : actionInputs.toList()) { + for (ActionInput input : actionInputs) { if (!isOutputPath(input, outputRoot)) { continue; } 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 8a8508910404c4..60f5761ee56377 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 @@ -25,6 +25,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.Strings; +import com.google.common.collect.BiMap; +import com.google.common.collect.HashBiMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -87,6 +89,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.UUID; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -298,7 +301,6 @@ static class ReducedClasspath { } } - private JavaSpawn getReducedSpawn( ActionExecutionContext actionExecutionContext, ReducedClasspath reducedClasspath, @@ -729,14 +731,15 @@ public NestedSet getPossibleInputsForTesting() { * * @param spawnResult the executor action that created the possibly stripped .jdeps output * @param outputDepsProto path to the .jdeps output - * @param actionInputs all inputs to the current action + * @param artifactsToPathMap all inputs to the current action plus any additional artifacts that + * may be referenced in the .jdeps file by path * @param actionExecutionContext the action execution context * @return the full deps proto (also written to disk to satisfy the action's declared output) */ static Deps.Dependencies createFullOutputDeps( SpawnResult spawnResult, Artifact outputDepsProto, - NestedSet actionInputs, + Iterable artifactsToPathMap, ActionExecutionContext actionExecutionContext, PathMapper pathMapper) throws IOException { @@ -748,36 +751,50 @@ static Deps.Dependencies createFullOutputDeps( return executorJdeps; } + // No paths to rewrite. + if (executorJdeps.getDependencyCount() == 0) { + return executorJdeps; + } + // For each of the action's generated inputs, revert its mapped path back to its original path. - Map mappedToOriginalPath = new HashMap<>(); - for (Artifact actionInput : actionInputs.toList()) { + BiMap mappedToOriginalPath = HashBiMap.create(); + for (Artifact actionInput : artifactsToPathMap) { if (actionInput.isSourceArtifact()) { continue; } String mappedPath = pathMapper.getMappedExecPathString(actionInput); - if (mappedToOriginalPath.put(mappedPath, actionInput.getExecPath()) != null) { - // If an entry already exists, that means different inputs reduce to the same stripped path. - // That also means PathStripper would exempt this action from path stripping, so the - // executor-produced .jdeps already includes full paths. No need to update it. - return executorJdeps; + PathFragment previousPath = mappedToOriginalPath.put(mappedPath, actionInput.getExecPath()); + if (previousPath != null && !previousPath.equals(actionInput.getExecPath())) { + throw new IllegalStateException( + String.format( + "Duplicate mapped path %s derived from %s and %s", + mappedPath, actionInput.getExecPath(), mappedToOriginalPath.get(mappedPath))); } } - // No paths to rewrite. - if (executorJdeps.getDependencyCount() == 0) { - return executorJdeps; - } - // Rewrite the .jdeps proto with full paths. PathFragment outputRoot = outputDepsProto.getExecPath().subFragment(0, 1); Deps.Dependencies.Builder fullDepsBuilder = Deps.Dependencies.newBuilder(executorJdeps); for (Deps.Dependency.Builder dep : fullDepsBuilder.getDependencyBuilderList()) { PathFragment pathOnExecutor = PathFragment.create(dep.getPath()); PathFragment originalPath = mappedToOriginalPath.get(pathOnExecutor.getPathString()); - if (originalPath == null && pathOnExecutor.subFragment(0, 1).equals(outputRoot)) { - // The mapped path -> full path map failed, which means the paths weren't mapped. Fast- - // return the original jdeps to save unnecessary CPU time. - return executorJdeps; + // Source files, which do not lie under the output root, are not mapped. It is also possible + // that a jdeps file contains a reference to a transitive classpath element that isn't an + // input to the current action (see + // https://github.com/google/turbine/commit/f9f2decee04a3c651671f7488a7c9d7952df88c8), just an + // additional artifact marked for path mapping, and itself wasn't built with path mapping + // enabled (e .g. due to path collisions). In that case, the path will already be unmapped and + // we can leave it as is. For entirely unexpected paths, we still report an error. + if (originalPath == null + && pathOnExecutor.subFragment(0, 1).equals(outputRoot) + && !mappedToOriginalPath.containsValue(pathOnExecutor)) { + throw new IllegalStateException( + String.format( + "Missing original path for mapped path %s in %s%njdeps: %s%npath map: %s", + pathOnExecutor, + outputDepsProto.getExecPath(), + executorJdeps, + mappedToOriginalPath)); } dep.setPath( originalPath == null ? pathOnExecutor.getPathString() : originalPath.getPathString()); @@ -825,7 +842,7 @@ private Deps.Dependencies readFullOutputDeps( SpawnResult result = Iterables.getOnlyElement(results); try { return createFullOutputDeps( - result, outputDepsProto, getInputs(), actionExecutionContext, pathMapper); + result, outputDepsProto, getInputs().toList(), actionExecutionContext, pathMapper); } catch (IOException e) { throw ActionExecutionException.fromExecException( new EnvironmentalExecException( @@ -837,7 +854,7 @@ e, createFailureDetail(".jdeps read IOException", Code.JDEPS_READ_IO_EXCEPTION)) private ActionExecutionException createActionExecutionException(Exception e, Code detailedCode) { DetailedExitCode detailedExitCode = DetailedExitCode.of(createFailureDetail(Strings.nullToEmpty(e.getMessage()), detailedCode)); - return new ActionExecutionException(e, this, /*catastrophe=*/ false, detailedExitCode); + return new ActionExecutionException(e, this, /* catastrophe= */ false, detailedExitCode); } private static FailureDetail createFailureDetail(String message, Code detailedCode) { 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 50194c94c22048..79e06552c2be48 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 @@ -78,6 +78,7 @@ public final class JavaHeaderCompileAction extends SpawnAction { private final boolean insertDependencies; + private final NestedSet additionalArtifactsForPathMapping; private JavaHeaderCompileAction( ActionOwner owner, @@ -92,7 +93,8 @@ private JavaHeaderCompileAction( RunfilesSupplier runfilesSupplier, String mnemonic, OutputPathsMode outputPathsMode, - boolean insertDependencies) { + boolean insertDependencies, + NestedSet additionalArtifactsForPathMapping) { super( owner, tools, @@ -107,6 +109,12 @@ private JavaHeaderCompileAction( mnemonic, outputPathsMode); this.insertDependencies = insertDependencies; + this.additionalArtifactsForPathMapping = additionalArtifactsForPathMapping; + } + + @Override + public NestedSet getAdditionalArtifactsForPathMapping() { + return additionalArtifactsForPathMapping; } @Override @@ -117,7 +125,12 @@ protected void afterExecute( try { Deps.Dependencies fullOutputDeps = JavaCompileAction.createFullOutputDeps( - spawnResult, outputDepsProto, getInputs(), context, pathMapper); + spawnResult, + outputDepsProto, + Iterables.concat( + getInputs().toList(), getAdditionalArtifactsForPathMapping().toList()), + context, + pathMapper); JavaCompileActionContext javaContext = context.getContext(JavaCompileActionContext.class); if (insertDependencies && javaContext != null) { javaContext.insertDependencies(outputDepsProto, fullOutputDeps); @@ -463,10 +476,20 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException } if (useDirectClasspath) { NestedSet classpath; + NestedSet additionalArtifactsForPathMapping; if (!directJars.isEmpty() || classpathEntries.isEmpty()) { classpath = directJars; + // When using the direct classpath optimization, Turbine generates .jdeps entries based on + // the transitive dependency information packages into META-INF/TRANSITIVE. When path + // mapping is used, these entries may have been subject to it when they were generated. + // Since the contents of that directory are not unmapped, we need to instead unmap the + // paths emitted in the .jdeps file, which requires knowing the full list of artifact + // paths even if they aren't inputs to the current action. + // https://github.com/google/turbine/commit/f9f2decee04a3c651671f7488a7c9d7952df88c8 + additionalArtifactsForPathMapping = classpathEntries; } else { classpath = classpathEntries; + additionalArtifactsForPathMapping = NestedSetBuilder.emptySet(Order.STABLE_ORDER); } mandatoryInputsBuilder.addTransitive(classpath); @@ -499,7 +522,8 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException // If classPathMode == BAZEL, also make sure to inject the dependencies to be // available to downstream actions. Else just do enough work to locally create the // full .jdeps from the .stripped .jdeps produced on the executor. - /* insertDependencies= */ classpathMode == JavaClasspathMode.BAZEL)); + /* insertDependencies= */ classpathMode == JavaClasspathMode.BAZEL, + additionalArtifactsForPathMapping)); return; } diff --git a/src/test/shell/integration/config_stripped_outputs_test.sh b/src/test/shell/integration/config_stripped_outputs_test.sh index 8498a0c79afcb9..9a7a68e1f2fbcc 100755 --- a/src/test/shell/integration/config_stripped_outputs_test.sh +++ b/src/test/shell/integration/config_stripped_outputs_test.sh @@ -194,6 +194,7 @@ function test_inmemory_jdeps_support() { assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libd-hjar.jdeps" } +<<<<<<< HEAD function test_multiple_configs() { local -r pkg="${FUNCNAME[0]}" mkdir -p "$pkg/rules" @@ -328,4 +329,53 @@ EOF assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/libbase_lib-hjar.jdeps" } +function test_direct_classpath() { + local -r pkg="${FUNCNAME[0]}" + mkdir -p "$pkg/java/hello/" || fail "Expected success" + cat > "$pkg/java/hello/A.java" <<'EOF' +package hello; +public class A { + public void f(B b) {} +} +EOF + cat > "$pkg/java/hello/B.java" <<'EOF' +package hello; +public class B extends C {} +EOF + cat > "$pkg/java/hello/C.java" <<'EOF' +package hello; +public class C extends D {} +EOF + cat > "$pkg/java/hello/D.java" <<'EOF' +package hello; +public class D {} +EOF + cat > "$pkg/java/hello/BUILD" <<'EOF' +java_library(name='a', srcs=['A.java'], deps = [':b']) +java_library(name='b', srcs=['B.java'], deps = [':c']) +java_library(name='c', srcs=['C.java'], deps = [':d']) +java_library(name='d', srcs=['D.java']) +EOF + + bazel build --experimental_java_classpath=bazel \ + --experimental_output_paths=strip \ + --experimental_inmemory_jdeps_files \ + //"$pkg"/java/hello:a -s 2>"$TEST_log" \ + || fail "Expected success" + + # java_library .jar compilation: + assert_paths_stripped "$TEST_log" "$pkg/java/hello/liba.jar-0.params" + # java_library header jar compilation: + assert_paths_stripped "$TEST_log" "bin/$pkg/java/hello/libb-hjar.jar" + # jdeps files should contain the original paths since they are read by downstream actions that may + # not use path mapping. + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/liba.jdeps" + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libb.jdeps" + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libb-hjar.jdeps" + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libc.jdeps" + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libc-hjar.jdeps" + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libd.jdeps" + assert_contains_no_stripped_path "${bazel_bin}/$pkg/java/hello/libd-hjar.jdeps" +} + run_suite "Tests stripping config prefixes from output paths for better action caching"