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
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
index 94ddcc5..91fcc27 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
@@ -100,6 +100,13 @@
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 @@
/**
* The directory to put all runfiles under.
*
- * <p>Using "foo" will put runfiles under <target>.runfiles/foo.</p>
+ * <p>Using "foo" will put runfiles under <target>.runfiles/foo.
*
* <p>This is either set to the workspace name, or is empty.
*/
@@ -1083,15 +1090,19 @@
}
}
- /** 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 @@
}
/** 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 @@
"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());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
index e469c0b..61dd5fc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
@@ -95,6 +95,9 @@
* @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 @@
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 @@
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 @@
// 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 @@
// Source-only symlink manifest has root-relative paths and does not include absolute paths.
return true;
}
+
+ @Override
+ public boolean emitsAbsolutePaths() {
+ return false;
+ }
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
index 65a6624..1c9270a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
@@ -209,7 +209,7 @@
// 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) {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java
index a6b4e4f..793b43b 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java
@@ -148,13 +148,25 @@
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;
+ }
}
/**
diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh
index 2cdc184..568f3bf 100755
--- a/src/test/shell/integration/runfiles_test.sh
+++ b/src/test/shell/integration/runfiles_test.sh
@@ -427,5 +427,51 @@
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"
