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 8d10786346dea6..2bf6773d73d219 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 @@ -657,12 +657,4 @@ 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 f0d52883152cef..3ebba4796c5213 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,7 +15,6 @@ 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; @@ -64,8 +63,8 @@ public final class PathMappers { * Actions that support path mapping should call this method from {@link * Action#getKey(ActionKeyContext, ArtifactExpander)}. * - *

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

Compared to {@link #create(Action, 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 @@ -104,12 +103,22 @@ public static void addToFingerprint( * OutputPathsMode, Fingerprint)} from {@link Action#getKey(ActionKeyContext, ArtifactExpander)} * to ensure correct incremental builds. * - * @param action the {@link AbstractAction} for which a {@link Spawn} is to be created + * @param action the {@link Action} 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(AbstractAction action, OutputPathsMode outputPathsMode) { + 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) { if (getEffectiveOutputPathsMode( outputPathsMode, action.getMnemonic(), action.getExecutionInfo()) != OutputPathsMode.STRIP) { @@ -119,8 +128,8 @@ public static PathMapper create(AbstractAction action, OutputPathsMode outputPat } /** - * Helper method to simplify calling {@link #create(SpawnAction, OutputPathsMode)} for actions - * that store the configuration directly. + * Helper method to simplify calling {@link #create(Action, 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 9f7dd36652157c..b453d1afbc7241 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,8 +15,6 @@ 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.ActionInputHelper.BasicActionInput; @@ -28,8 +26,9 @@ import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -89,15 +88,10 @@ 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(AbstractAction action) { + static Optional tryCreate(Action 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); - // 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)) { + if (isPathStrippable(action.getInputs(), outputRoot)) { return Optional.of( create(action.getMnemonic(), action instanceof StarlarkAction, outputRoot)); } @@ -256,7 +250,7 @@ private static boolean isOutputPath(PathFragment pathFragment, PathFragment outp *

This method checks b). */ private static boolean isPathStrippable( - Iterable actionInputs, PathFragment outputRoot) { + NestedSet 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". // @@ -267,15 +261,15 @@ private static boolean isPathStrippable( // with configurations). While this would help more action instances qualify, it also blocks // 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. - HashMap rootRelativePaths = new HashMap<>(); - for (ActionInput input : actionInputs) { + HashSet rootRelativePaths = new HashSet<>(); + for (ActionInput input : actionInputs.toList()) { if (!isOutputPath(input, outputRoot)) { continue; } // For "bazel-out/k8-fastbuild/foo/bar", get "foo/bar". - if (!rootRelativePaths - .computeIfAbsent(input.getExecPath().subFragment(2), k -> input) - .equals(input)) { + if (!rootRelativePaths.add(input.getExecPath().subFragment(2))) { + // TODO(bazel-team): don't fail on duplicate inputs, i.e. when the same exact exec path + // (including config prefix) is included twice. return false; } } 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 c876299fed39b9..d68b3a7187a1bc 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,8 +25,6 @@ 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; @@ -84,6 +82,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -290,6 +289,7 @@ static class ReducedClasspath { } } + private JavaSpawn getReducedSpawn( ActionExecutionContext actionExecutionContext, ReducedClasspath reducedClasspath, @@ -720,15 +720,14 @@ public NestedSet getPossibleInputsForTesting() { * * @param spawnResult the executor action that created the possibly stripped .jdeps output * @param outputDepsProto path to the .jdeps output - * @param artifactsToPathMap all inputs to the current action plus any additional artifacts that - * may be referenced in the .jdeps file by path + * @param actionInputs all inputs to the current action * @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, - Iterable artifactsToPathMap, + NestedSet actionInputs, ActionExecutionContext actionExecutionContext, PathMapper pathMapper) throws IOException { @@ -740,50 +739,36 @@ 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. - BiMap mappedToOriginalPath = HashBiMap.create(); - for (Artifact actionInput : artifactsToPathMap) { + Map mappedToOriginalPath = new HashMap<>(); + for (Artifact actionInput : actionInputs.toList()) { if (actionInput.isSourceArtifact()) { continue; } String mappedPath = pathMapper.getMappedExecPathString(actionInput); - 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))); + 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; } } + // 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()); - // 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)); + 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; } dep.setPath( originalPath == null ? pathOnExecutor.getPathString() : originalPath.getPathString()); @@ -831,7 +816,7 @@ private Deps.Dependencies readFullOutputDeps( SpawnResult result = Iterables.getOnlyElement(results); try { return createFullOutputDeps( - result, outputDepsProto, getInputs().toList(), actionExecutionContext, pathMapper); + result, outputDepsProto, getInputs(), actionExecutionContext, pathMapper); } catch (IOException e) { throw ActionExecutionException.fromExecException( new EnvironmentalExecException( 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 0a8996ccbf390e..b37facf9e5c57b 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 @@ -76,7 +76,6 @@ public final class JavaHeaderCompileAction extends SpawnAction { private final boolean insertDependencies; - private final NestedSet additionalArtifactsForPathMapping; private JavaHeaderCompileAction( ActionOwner owner, @@ -90,8 +89,7 @@ private JavaHeaderCompileAction( CharSequence progressMessage, String mnemonic, OutputPathsMode outputPathsMode, - boolean insertDependencies, - NestedSet additionalArtifactsForPathMapping) { + boolean insertDependencies) { super( owner, tools, @@ -105,12 +103,6 @@ private JavaHeaderCompileAction( mnemonic, outputPathsMode); this.insertDependencies = insertDependencies; - this.additionalArtifactsForPathMapping = additionalArtifactsForPathMapping; - } - - @Override - public NestedSet getAdditionalArtifactsForPathMapping() { - return additionalArtifactsForPathMapping; } @Override @@ -121,12 +113,7 @@ protected void afterExecute( try { Deps.Dependencies fullOutputDeps = JavaCompileAction.createFullOutputDeps( - spawnResult, - outputDepsProto, - Iterables.concat( - getInputs().toList(), getAdditionalArtifactsForPathMapping().toList()), - context, - pathMapper); + spawnResult, outputDepsProto, getInputs(), context, pathMapper); JavaCompileActionContext javaContext = context.getContext(JavaCompileActionContext.class); if (insertDependencies && javaContext != null) { javaContext.insertDependencies(outputDepsProto, fullOutputDeps); @@ -472,20 +459,10 @@ 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); @@ -517,8 +494,7 @@ 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, - additionalArtifactsForPathMapping)); + /* insertDependencies= */ classpathMode == JavaClasspathMode.BAZEL)); return; } diff --git a/src/test/shell/integration/config_stripped_outputs_test.sh b/src/test/shell/integration/config_stripped_outputs_test.sh index da111c4ec0eab5..8498a0c79afcb9 100755 --- a/src/test/shell/integration/config_stripped_outputs_test.sh +++ b/src/test/shell/integration/config_stripped_outputs_test.sh @@ -328,55 +328,4 @@ 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" - # When compiling C, the direct classpath optimization in Turbine embeds information about the - # dependency D into the header jar, which then results in the path to Ds header jar being included - # in the jdeps file for B. The full compilation action for A requires the header jar for D and - # thus the path to it in the jdeps file of B has to be unmapped properly for the reduced classpath - # created for A to contain it. - cat > "$pkg/java/hello/A.java" <<'EOF' -package hello; -public class A extends 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 \ - //"$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"