Revert "[7.1.0] Also path map transitive header jar paths with direct classpath optimization"

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -666,12 +666,4 @@ public ImmutableMap<String, String> 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<Artifact> getAdditionalArtifactsForPathMapping() {
-    return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
-  }
 }

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

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.Artifact.DerivedArtifact;
@@ -27,8 +25,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;
@@ -88,15 +87,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<PathMapper> tryCreate(AbstractAction action) {
+  static Optional<PathMapper> 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));
     }
@@ -249,7 +243,7 @@ private static boolean isOutputPath(PathFragment pathFragment, PathFragment outp
    * <p>This method checks b).
    */
   private static boolean isPathStrippable(
-      Iterable<? extends ActionInput> actionInputs, PathFragment outputRoot) {
+      NestedSet<? extends ActionInput> 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".
     //
@@ -260,15 +254,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<PathFragment, ActionInput> rootRelativePaths = new HashMap<>();
-    for (ActionInput input : actionInputs) {
+    HashSet<PathFragment> 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;
       }
     }

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;
@@ -294,6 +293,7 @@ static class ReducedClasspath {
     }
   }
 
+
   private JavaSpawn getReducedSpawn(
       ActionExecutionContext actionExecutionContext,
       ReducedClasspath reducedClasspath,
@@ -724,15 +724,14 @@ public NestedSet<Artifact> 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<Artifact> artifactsToPathMap,
+      NestedSet<Artifact> actionInputs,
       ActionExecutionContext actionExecutionContext,
       PathMapper pathMapper)
       throws IOException {
@@ -744,50 +743,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<String, PathFragment> mappedToOriginalPath = HashBiMap.create();
-    for (Artifact actionInput : artifactsToPathMap) {
+    Map<String, PathFragment> 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());
@@ -835,7 +820,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(
@@ -847,7 +832,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) {

src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java

@@ -78,7 +78,6 @@
 public final class JavaHeaderCompileAction extends SpawnAction {
 
   private final boolean insertDependencies;
-  private final NestedSet<Artifact> additionalArtifactsForPathMapping;
 
   private JavaHeaderCompileAction(
       ActionOwner owner,
@@ -93,8 +92,7 @@ private JavaHeaderCompileAction(
       RunfilesSupplier runfilesSupplier,
       String mnemonic,
       OutputPathsMode outputPathsMode,
-      boolean insertDependencies,
-      NestedSet<Artifact> additionalArtifactsForPathMapping) {
+      boolean insertDependencies) {
     super(
         owner,
         tools,
@@ -109,12 +107,6 @@ private JavaHeaderCompileAction(
         mnemonic,
         outputPathsMode);
     this.insertDependencies = insertDependencies;
-    this.additionalArtifactsForPathMapping = additionalArtifactsForPathMapping;
-  }
-
-  @Override
-  public NestedSet<Artifact> getAdditionalArtifactsForPathMapping() {
-    return additionalArtifactsForPathMapping;
   }
 
   @Override
@@ -125,12 +117,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);
@@ -476,20 +463,10 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
       }
       if (useDirectClasspath) {
         NestedSet<Artifact> classpath;
-        NestedSet<Artifact> 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);
 
@@ -522,8 +499,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;
       }
 

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"