From c9d7ff9384fa06e9fe676c2c8935c675f2238c57 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 18 Aug 2023 15:01:32 -0700 Subject: [PATCH] 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 --- .../devtools/build/lib/analysis/Runfiles.java | 27 ++++++++--- .../lib/analysis/SourceManifestAction.java | 19 +++++++- .../analysis/actions/SymlinkTreeAction.java | 2 +- .../analysis/SourceManifestActionTest.java | 16 ++++++- src/test/shell/integration/runfiles_test.sh | 46 +++++++++++++++++++ 5 files changed, 99 insertions(+), 11 deletions(-) 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 94ddcc5b7d80c7..91fcc2724fa9dc 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 @@ public void fingerprint(Fingerprint fp) { args.accept(symlink.getArtifact().getExecPathString()); }; + private static final CommandLineItem.ExceptionlessMapFn + RUNFILES_AND_ABSOLUTE_PATH_MAP_FN = + (artifact, args) -> { + args.accept(artifact.getRunfilesPathString()); + args.accept(artifact.getPath().getPathString()); + }; + private static final CommandLineItem.ExceptionlessMapFn 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. * - *

Using "foo" will put runfiles under <target>.runfiles/foo.

+ *

Using "foo" will put runfiles under <target>.runfiles/foo. * *

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()); } } 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 e469c0bbefe5a8..61dd5fc1da0be3 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 @@ 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; + } } } } 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 65a66241f1b40b..1c9270af0967b3 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 @@ 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) { 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 a6b4e4f17eafd3..793b43bab02670 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 @@ 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; + } } /** diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index 2cdc1848aa8e59..568f3bff6cccec 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/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 < hello_world.sh <