Skip to content

Commit

Permalink
Include the full absolute paths of runfiles in action keys
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fmeum authored and copybara-github committed Aug 18, 2023
1 parent 5658b43 commit c9d7ff9
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 11 deletions.
27 changes: 21 additions & 6 deletions src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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);

Expand All @@ -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)
Expand All @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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());
Expand All @@ -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. */
Expand Down Expand Up @@ -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;
}
},

/**
Expand Down Expand Up @@ -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;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

/**
Expand Down
46 changes: 46 additions & 0 deletions src/test/shell/integration/runfiles_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"

0 comments on commit c9d7ff9

Please sign in to comment.