Include the full absolute paths of runfiles in action keys

Both `SourceManifestAction` and `SymlinkTreeAction`, the only users of `Runfiles#fingerprint`, emit absolute paths to runfiles artifacts in their output. This requires also including these paths in the action key computation to prevent incorrect incremental builds if `--output_base` changes.

Fixes #17267

Closes #19171.

PiperOrigin-RevId: 558256343
Change-Id: I123b66da8752e77267f7a086d016af81af0d21a4

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

@@ -100,6 +100,13 @@ public void fingerprint(Fingerprint fp) {
         args.accept(symlink.getArtifact().getExecPathString());
       };
 
+  private static final CommandLineItem.ExceptionlessMapFn<Artifact>
+      RUNFILES_AND_ABSOLUTE_PATH_MAP_FN =
+          (artifact, args) -> {
+            args.accept(artifact.getRunfilesPathString());
+            args.accept(artifact.getPath().getPathString());
+          };
+
   private static final CommandLineItem.ExceptionlessMapFn<Artifact> RUNFILES_AND_EXEC_PATH_MAP_FN =
       (artifact, args) -> {
         args.accept(artifact.getRunfilesPathString());
@@ -109,7 +116,7 @@ public void fingerprint(Fingerprint fp) {
   /**
    * The directory to put all runfiles under.
    *
-   * <p>Using "foo" will put runfiles under &lt;target&gt;.runfiles/foo.</p>
+   * <p>Using "foo" will put runfiles under &lt;target&gt;.runfiles/foo.
    *
    * <p>This is either set to the workspace name, or is empty.
    */
@@ -1083,15 +1090,19 @@ public Runfiles mergeAll(Sequence<?> sequence, StarlarkThread thread) throws Eva
     }
   }
 
-  /** Fingerprint this {@link Runfiles} tree. */
-  public void fingerprint(ActionKeyContext actionKeyContext, Fingerprint fp) {
+  /** Fingerprint this {@link Runfiles} tree, including the absolute paths of artifacts. */
+  public void fingerprint(
+      ActionKeyContext actionKeyContext, Fingerprint fp, boolean digestAbsolutePaths) {
     fp.addInt(conflictPolicy.ordinal());
     fp.addBoolean(legacyExternalRunfiles);
     fp.addPath(suffix);
 
     actionKeyContext.addNestedSetToFingerprint(SYMLINK_ENTRY_MAP_FN, fp, symlinks);
     actionKeyContext.addNestedSetToFingerprint(SYMLINK_ENTRY_MAP_FN, fp, rootSymlinks);
-    actionKeyContext.addNestedSetToFingerprint(RUNFILES_AND_EXEC_PATH_MAP_FN, fp, artifacts);
+    actionKeyContext.addNestedSetToFingerprint(
+        digestAbsolutePaths ? RUNFILES_AND_ABSOLUTE_PATH_MAP_FN : RUNFILES_AND_EXEC_PATH_MAP_FN,
+        fp,
+        artifacts);
 
     emptyFilesSupplier.fingerprint(fp);
 
@@ -1100,7 +1111,7 @@ public void fingerprint(ActionKeyContext actionKeyContext, Fingerprint fp) {
   }
 
   /** Describes the inputs {@link #fingerprint} uses to aid describeKey() descriptions. */
-  String describeFingerprint() {
+  String describeFingerprint(boolean digestAbsolutePaths) {
     return String.format("conflictPolicy: %s\n", conflictPolicy)
         + String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles)
         + String.format("suffix: %s\n", suffix)
@@ -1110,7 +1121,11 @@ String describeFingerprint() {
             "rootSymlinks: %s\n", describeNestedSetFingerprint(SYMLINK_ENTRY_MAP_FN, rootSymlinks))
         + String.format(
             "artifacts: %s\n",
-            describeNestedSetFingerprint(RUNFILES_AND_EXEC_PATH_MAP_FN, artifacts))
+            describeNestedSetFingerprint(
+                digestAbsolutePaths
+                    ? RUNFILES_AND_ABSOLUTE_PATH_MAP_FN
+                    : RUNFILES_AND_EXEC_PATH_MAP_FN,
+                artifacts))
         + String.format("emptyFilesSupplier: %s\n", emptyFilesSupplier.getClass().getName());
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java

@@ -95,6 +95,9 @@ void writeEntry(
      * @return
      */
     boolean isRemotable();
+
+    /** Whether the manifest includes absolute paths to artifacts. */
+    boolean emitsAbsolutePaths();
   }
 
   /** The strategy we use to write manifest entries. */
@@ -254,7 +257,7 @@ protected void computeKey(
       Fingerprint fp) {
     fp.addString(GUID);
     fp.addBoolean(remotableSourceManifestActions);
-    runfiles.fingerprint(actionKeyContext, fp);
+    runfiles.fingerprint(actionKeyContext, fp, manifestWriter.emitsAbsolutePaths());
     fp.addBoolean(repoMappingManifest != null);
     if (repoMappingManifest != null) {
       fp.addPath(repoMappingManifest.getExecPath());
@@ -265,7 +268,9 @@ protected void computeKey(
   public String describeKey() {
     return String.format(
         "GUID: %s\nremotableSourceManifestActions: %s\nrunfiles: %s\n",
-        GUID, remotableSourceManifestActions, runfiles.describeFingerprint());
+        GUID,
+        remotableSourceManifestActions,
+        runfiles.describeFingerprint(manifestWriter.emitsAbsolutePaths()));
   }
 
   /** Supported manifest writing strategies. */
@@ -311,6 +316,11 @@ public boolean isRemotable() {
         // There is little gain to remoting these, since they include absolute path names inline.
         return false;
       }
+
+      @Override
+      public boolean emitsAbsolutePaths() {
+        return true;
+      }
     },
 
     /**
@@ -346,6 +356,11 @@ public boolean isRemotable() {
         // Source-only symlink manifest has root-relative paths and does not include absolute paths.
         return true;
       }
+
+      @Override
+      public boolean emitsAbsolutePaths() {
+        return false;
+      }
     }
   }
 }

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

@@ -209,7 +209,7 @@ protected void computeKey(
     // safe to add more fields in the future.
     fp.addBoolean(runfiles != null);
     if (runfiles != null) {
-      runfiles.fingerprint(actionKeyContext, fp);
+      runfiles.fingerprint(actionKeyContext, fp, /* digestAbsolutePaths= */ true);
     }
     fp.addBoolean(repoMappingManifest != null);
     if (repoMappingManifest != null) {

src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java

@@ -148,13 +148,25 @@ public int unconsumedInputs() {
       return expectedSequence.size();
     }
 
-    @Override public String getMnemonic() { return null; }
-    @Override public String getRawProgressMessage() { return null; }
+    @Override
+    public String getMnemonic() {
+      return null;
+    }
+
+    @Override
+    public String getRawProgressMessage() {
+      return null;
+    }
 
     @Override
     public boolean isRemotable() {
       return false;
     }
+
+    @Override
+    public boolean emitsAbsolutePaths() {
+      return false;
+    }
   }
 
   /**

src/test/shell/integration/runfiles_test.sh

@@ -427,5 +427,51 @@ EOF
   expect_log_once "Runfiles must not contain middleman artifacts"
 }
 
+function test_manifest_action_reruns_on_output_base_change() {
+  CURRENT_DIRECTORY=$(pwd)
+  if $is_windows; then
+    CURRENT_DIRECTORY=$(cygpath -m "${CURRENT_DIRECTORY}")
+  fi
+
+  if $is_windows; then
+    MANIFEST_PATH=bazel-bin/hello_world.exe.runfiles_manifest
+  else
+    MANIFEST_PATH=bazel-bin/hello_world.runfiles_manifest
+  fi
+
+  OUTPUT_BASE="${CURRENT_DIRECTORY}/test/outputs/__main__"
+  TEST_FOLDER_1="${CURRENT_DIRECTORY}/test/test1/$(basename ${CURRENT_DIRECTORY})"
+  TEST_FOLDER_2="${CURRENT_DIRECTORY}/test/test2/$(basename ${CURRENT_DIRECTORY})"
+
+  mkdir -p "${OUTPUT_BASE}"
+  mkdir -p "${TEST_FOLDER_1}"
+  mkdir -p "${TEST_FOLDER_2}"
+
+  cat > BUILD <<EOF
+sh_binary(
+    name = "hello_world",
+    srcs = ["hello_world.sh"],
+)
+EOF
+  cat > hello_world.sh <<EOF
+echo "Hello World"
+EOF
+  chmod +x hello_world.sh
+
+  for d in $(ls -a | grep -v '^test$' | grep -v '^\.*$'); do
+    cp -R "${CURRENT_DIRECTORY}/${d}" "${TEST_FOLDER_1}"
+    cp -R "${CURRENT_DIRECTORY}/${d}" "${TEST_FOLDER_2}"
+  done
+
+  cd "${TEST_FOLDER_1}"
+  bazel --output_base="${OUTPUT_BASE}" build //:hello_world
+  assert_contains "${TEST_FOLDER_1}" "${MANIFEST_PATH}"
+  assert_not_contains "${TEST_FOLDER_2}" "${MANIFEST_PATH}"
+
+  cd "${TEST_FOLDER_2}"
+  bazel --output_base="${OUTPUT_BASE}" build //:hello_world
+  assert_not_contains "${TEST_FOLDER_1}" "${MANIFEST_PATH}"
+  assert_contains "${TEST_FOLDER_2}" "${MANIFEST_PATH}"
+}
 
 run_suite "runfiles"