Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include the full absolute paths of runfiles in action keys #19171

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -215,7 +215,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 @@ -155,6 +155,11 @@ public int unconsumedInputs() {
public boolean isRemotable() {
return false;
}

@Override
public boolean emitsAbsolutePaths() {
return false;
}
}

/**
Expand Down
47 changes: 47 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,52 @@ 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"
TEST_FOLDER_2="${CURRENT_DIRECTORY}/test/test2"

mkdir -p "${OUTPUT_BASE}"
mkdir -p "${TEST_FOLDER_1}"
mkdir -p "${TEST_FOLDER_2}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: symlinking $TEST_FOLDER_2 to $TEST_FOLDER_1 makes it more obvious that the two are the same

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am copying the contents now. I think that symlinking wouldn't work as the "same workspace, same output root" case didn't result in any issues even before this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that Bazel doesn't resolve symlinks in the name of the workspace, but you're right -- copying is the obviously correct option.


cd "${TEST_FOLDER_1}"
touch WORKSPACE
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

cp "${TEST_FOLDER_1}"/* "${TEST_FOLDER_2}/"

cd "${TEST_FOLDER_1}"
bazel --output_base="${OUTPUT_BASE}" build //...

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 //...

assert_not_contains "${TEST_FOLDER_1}" "${MANIFEST_PATH}"
assert_contains "${TEST_FOLDER_2}" "${MANIFEST_PATH}"
}

run_suite "runfiles"
Loading