Revert 7f6e649

Internal breakages

PiperOrigin-RevId: 721700415
Change-Id: I67b3192df6a0fbd2f9e39e220e7796286aaa110e

Author: hvadehra

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

@@ -56,8 +56,7 @@ public void addToFingerprint(
       CoreOptions.OutputPathsMode outputPathsMode,
       Fingerprint fingerprint)
       throws CommandLineExpansionException, InterruptedException {
-    for (String s :
-        arguments(/* artifactExpander= */ null, PathMapper.forActionKey(outputPathsMode))) {
+    for (String s : arguments()) {
       fingerprint.addString(s);
     }
   }

src/main/java/com/google/devtools/build/lib/actions/BUILD

@@ -311,7 +311,6 @@ java_library(
         "ArtifactRoot.java",
         "Artifacts.java",
         "PathMapper.java",
-        "PathMapperConstants.java",
     ],
     deps = [
         ":action_lookup_data",
@@ -321,7 +320,6 @@ java_library(
         ":commandline_item",
         ":package_roots",
         "//src/main/java/com/google/devtools/build/docgen/annot",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset:fingerprint_cache",

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

@@ -14,10 +14,11 @@
 
 package com.google.devtools.build.lib.actions;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn;
 import com.google.devtools.build.lib.actions.CommandLineItem.MapFn;
-import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.errorprone.annotations.CheckReturnValue;
@@ -44,7 +45,7 @@ public interface PathMapper {
    * {@link #storeIn(StarlarkSemantics)}.
    */
   static PathMapper loadFrom(StarlarkSemantics semantics) {
-    return semantics.get(PathMapperConstants.SEMANTICS_KEY);
+    return semantics.get(SEMANTICS_KEY);
   }
 
   /**
@@ -64,11 +65,10 @@ static PathMapper loadFrom(StarlarkSemantics semantics) {
   default StarlarkSemantics storeIn(StarlarkSemantics semantics) {
     // This in particular covers the case where the semantics do not have a path mapper yet and this
     // is NOOP.
-    if (semantics.get(PathMapperConstants.SEMANTICS_KEY) == this) {
+    if (semantics.get(SEMANTICS_KEY) == this) {
       return semantics;
     }
-    return new StarlarkSemantics(
-        semantics.toBuilder().set(PathMapperConstants.SEMANTICS_KEY, this).build()) {
+    return new StarlarkSemantics(semantics.toBuilder().set(SEMANTICS_KEY, this).build()) {
       // The path mapper doesn't affect which fields or methods are available on any given Starlark
       // object; it just affects the behavior of certain methods on Artifact. We thus preserve the
       // original semantics as a cache key. Otherwise, even if PathMapper#equals returned true for
@@ -82,13 +82,6 @@ public StarlarkSemantics getStarlarkClassDescriptorCacheKey() {
     };
   }
 
-  /** Returns the instance to use during action key computation. */
-  static PathMapper forActionKey(CoreOptions.OutputPathsMode outputPathsMode) {
-    return outputPathsMode == CoreOptions.OutputPathsMode.OFF
-        ? NOOP
-        : PathMapperConstants.FOR_FINGERPRINTING;
-  }
-
   /**
    * Returns the exec path with the path mapping applied.
    *
@@ -149,7 +142,7 @@ default FileRootApi mapRoot(Artifact artifact) {
     if (root.isSourceRoot()) {
       // Source roots' paths are never mapped, but we still need to wrap them in a
       // MappedArtifactRoot to ensure correct Starlark comparison behavior.
-      return PathMapperConstants.mappedSourceRoots.get(root);
+      return mappedSourceRoots.get(root);
     }
     // It would *not* be correct to just apply #map to the exec path of the root: The root part of
     // the mapped exec path of this artifact may depend on its complete exec path as well as on e.g.
@@ -199,6 +192,15 @@ public FileRootApi mapRoot(Artifact artifact) {
         }
       };
 
+  StarlarkSemantics.Key<PathMapper> SEMANTICS_KEY =
+      new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP);
+
+  // Not meant for use outside this interface.
+  LoadingCache<ArtifactRoot, MappedArtifactRoot> mappedSourceRoots =
+      Caffeine.newBuilder()
+          .weakKeys()
+          .build(sourceRoot -> new MappedArtifactRoot(sourceRoot.getExecPath()));
+
   /** A {@link FileRootApi} returned by {@link PathMapper#mapRoot(Artifact)}. */
   @StarlarkBuiltin(
       name = "mapped_root",

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

@@ -40,6 +40,49 @@
  * PathMapper}).
  */
 public final class PathMappers {
+  private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out");
+  private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out");
+
+  /**
+   * A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally
+   * enabled for an action.
+   *
+   * <p>When computing an action key, the following approaches to taking path mapping into account
+   * do <b>not</b> work:
+   *
+   * <ul>
+   *   <li>Using the actual path mapper is prohibitive since constructing it requires checking for
+   *       collisions among the action input's paths when computing the action key, which flattens
+   *       the input depsets of all actions that opt into path mapping and also increases CPU usage.
+   *   <li>Unconditionally using {@link StrippingPathMapper} can result in stale action keys when an
+   *       action is opted out of path mapping at execution time due to input path collisions after
+   *       stripping. See path_mapping_test for an example.
+   *   <li>Using {@link PathMapper#NOOP} does not distinguish between map_each results built from
+   *       strings and those built from {@link
+   *       com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}.
+   *       While the latter will be mapped at execution time, the former won't, resulting in the
+   *       same digest for actions that behave differently at execution time. This is covered by
+   *       tests in StarlarkRuleImplementationFunctionsTest.
+   * </ul>
+   *
+   * <p>Instead, we use a special path mapping instance that preserves the equality relations
+   * between the original config segments, but prepends a fixed string to distinguish hard-coded
+   * path strings from mapped paths. This relies on actions using path mapping to be "root
+   * agnostic": they must not contain logic that depends on any particular (output) root path.
+   */
+  private static final PathMapper FOR_FINGERPRINTING =
+      execPath -> {
+        if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) {
+          // This is not an output path.
+          return execPath;
+        }
+        String execPathString = execPath.getPathString();
+        int startOfConfigSegment = execPathString.indexOf('/') + 1;
+        return PathFragment.createAlreadyNormalized(
+            execPathString.substring(0, startOfConfigSegment)
+                + "pm-"
+                + execPathString.substring(startOfConfigSegment));
+      };
 
   // TODO: Remove actions from this list by adding ExecutionRequirements.SUPPORTS_PATH_MAPPING to
   //  their execution info instead.
@@ -93,6 +136,13 @@ public static void addToFingerprint(
     }
   }
 
+  /** Returns the instance to use during action key computation. */
+  public static PathMapper forActionKey(OutputPathsMode outputPathsMode) {
+    return outputPathsMode == OutputPathsMode.OFF
+        ? PathMapper.NOOP
+        : PathMappers.FOR_FINGERPRINTING;
+  }
+
   /**
    * Actions that support path mapping should call this method when creating their {@link Spawn}.
    *

src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java

@@ -41,6 +41,7 @@
 import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.actions.PathMapper;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
+import com.google.devtools.build.lib.analysis.actions.PathMappers;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -574,7 +575,7 @@ private int addToFingerprint(
             maybeExpandDirectories(
                 artifactExpander,
                 arguments.subList(argi, argi + count),
-                PathMapper.forActionKey(outputPathsMode));
+                PathMappers.forActionKey(outputPathsMode));
         argi += count;
         if (mapEach != null) {
           // TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase)
@@ -589,7 +590,7 @@ private int addToFingerprint(
               fingerprint::addString,
               location,
               artifactExpander,
-              PathMapper.forActionKey(outputPathsMode),
+              PathMappers.forActionKey(outputPathsMode),
               starlarkSemantics);
         } else {
           for (Object value : maybeExpandedValues) {
@@ -1204,7 +1205,7 @@ public void expandToCommandLine(Object object, Consumer<String> args)
           args,
           location,
           artifactExpander,
-          PathMapper.forActionKey(outputPathsMode),
+          PathMappers.forActionKey(outputPathsMode),
           starlarkSemantics);
     }
 
@@ -1214,7 +1215,7 @@ private List<Object> maybeExpandDirectory(Object object) throws CommandLineExpan
       }
 
       return VectorArg.expandDirectories(
-          artifactExpander, ImmutableList.of(object), PathMapper.forActionKey(outputPathsMode));
+          artifactExpander, ImmutableList.of(object), PathMappers.forActionKey(outputPathsMode));
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java

@@ -111,40 +111,24 @@ public CcToolchainVariables getCcToolchainVariables(Dict<?, ?> buildVariables)
 
     CcToolchainVariables.Builder ccToolchainVariables = CcToolchainVariables.builder();
     for (var entry : buildVariables.entrySet()) {
-      String key = (String) entry.getKey();
-      Object value = entry.getValue();
-      switch (value) {
-        case String s -> ccToolchainVariables.addStringVariable(key, s);
-        case Artifact a -> ccToolchainVariables.addArtifactVariable(key, a);
-        case Boolean b -> ccToolchainVariables.addBooleanValue(key, b);
-        case Iterable<?> values -> {
-          if (key.equals("libraries_to_link")) {
-            SequenceBuilder sb = new SequenceBuilder();
-            for (var v : (Iterable<VariableValue>) values) {
-              sb.addValue(v);
-            }
-            ccToolchainVariables.addCustomBuiltVariable(key, sb);
-          } else {
-            ccToolchainVariables.addStringSequenceVariable(key, (Iterable<String>) values);
+      if (entry.getValue() instanceof String) {
+        ccToolchainVariables.addStringVariable((String) entry.getKey(), (String) entry.getValue());
+      } else if (entry.getValue() instanceof Boolean) {
+        ccToolchainVariables.addBooleanValue((String) entry.getKey(), (Boolean) entry.getValue());
+      } else if (entry.getValue() instanceof Iterable<?>) {
+        if (entry.getKey().equals("libraries_to_link")) {
+          SequenceBuilder sb = new SequenceBuilder();
+          for (var value : (Iterable<?>) entry.getValue()) {
+            sb.addValue((VariableValue) value);
           }
+          ccToolchainVariables.addCustomBuiltVariable((String) entry.getKey(), sb);
+        } else {
+          ccToolchainVariables.addStringSequenceVariable(
+              (String) entry.getKey(), (Iterable<String>) entry.getValue());
         }
-        case Depset depset -> {
-          Class<?> type = depset.getElementClass();
-          // Type doesn't matter for empty depsets.
-          if (type == String.class || type == null) {
-            ccToolchainVariables.addStringSequenceVariable(key, depset.getSet(String.class));
-          } else if (type == Artifact.class) {
-            ccToolchainVariables.addArtifactSequenceVariable(key, depset.getSet(Artifact.class));
-          } else if (type == PathFragment.class) {
-            ccToolchainVariables.addPathFragmentSequenceVariable(
-                key, depset.getSet(PathFragment.class));
-          } else {
-            throw new IllegalStateException("Unexpected depset element type: %s".formatted(type));
-          }
-        }
-        default ->
-            throw new IllegalStateException(
-                "Unexpected value: %s (%s)".formatted(value, value.getClass()));
+      } else if (entry.getValue() instanceof Depset) {
+        ccToolchainVariables.addStringSequenceVariable(
+            (String) entry.getKey(), ((Depset) entry.getValue()).getSet(String.class));
       }
     }
     return ccToolchainVariables.build();

src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java

@@ -412,18 +412,18 @@ private boolean canBeExpanded(
       }
       if (expandIfTrue != null
           && (!variables.isAvailable(expandIfTrue, expander)
-              || !variables.getVariable(expandIfTrue, pathMapper).isTruthy())) {
+              || !variables.getVariable(expandIfTrue).isTruthy())) {
         return false;
       }
       if (expandIfFalse != null
           && (!variables.isAvailable(expandIfFalse, expander)
-              || variables.getVariable(expandIfFalse, pathMapper).isTruthy())) {
+              || variables.getVariable(expandIfFalse).isTruthy())) {
         return false;
       }
       if (expandIfEqual != null
           && (!variables.isAvailable(expandIfEqual.variable, expander)
               || !variables
-                  .getVariable(expandIfEqual.variable, pathMapper)
+                  .getVariable(expandIfEqual.variable)
                   .getStringValue(expandIfEqual.variable, pathMapper)
                   .equals(expandIfEqual.value))) {
         return false;