Include the originating target root in Runfiles fingerprinting under `--incompatible_prefer_depending_configuration_runfiles`. Without this, caching would be incorrect with the flag enabled because the key for actions like SouceManifestAction would not change. PiperOrigin-RevId: 972177774 Change-Id: Ic94cfc92f5cbf0baf3f346981d9009f1c54659e0
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 f6cbe18..0657e76 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
@@ -918,8 +918,26 @@ /** Fingerprint this {@link Runfiles} tree, including the absolute paths of artifacts. */ public void fingerprint( ActionKeyContext actionKeyContext, Fingerprint fp, boolean digestAbsolutePaths) { + fingerprint(actionKeyContext, fp, digestAbsolutePaths, null); + } + + /** + * Fingerprint this {@link Runfiles} tree, including the absolute paths of artifacts. + * + * @param originatingTargetRoot the root of the originating target configuration, if available. + * This is used to match runfiles to the configuration of the depending rule in the event of + * conflicts. + */ + public void fingerprint( + ActionKeyContext actionKeyContext, + Fingerprint fp, + boolean digestAbsolutePaths, + @Nullable ArtifactRoot originatingTargetRoot) { fp.addInt(conflictPolicy.ordinal()); fp.addString(prefix); + if (originatingTargetRoot != null) { + fp.addString(originatingTargetRoot.getExecPathString()); + } actionKeyContext.addNestedSetToFingerprint( digestAbsolutePaths ? SYMLINK_ENTRY_ABSOLUTE_PATH_MAP_FN : SYMLINK_ENTRY_EXEC_PATH_MAP_FN, @@ -939,9 +957,17 @@ /** Describes the inputs {@link #fingerprint} uses to aid describeKey() descriptions. */ String describeFingerprint(boolean digestAbsolutePaths) { + return describeFingerprint(digestAbsolutePaths, null); + } + + String describeFingerprint( + boolean digestAbsolutePaths, @Nullable ArtifactRoot originatingTargetRoot) { return String.format("conflictPolicy: %s\n", conflictPolicy) + String.format("prefix: %s\n", prefix) + String.format( + "originatingTargetRoot: %s\n", + originatingTargetRoot == null ? "null" : originatingTargetRoot.getExecPathString()) + + String.format( "symlinks: %s\n", describeNestedSetFingerprint( digestAbsolutePaths
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index be1276a..2a21e77 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -213,7 +213,7 @@ @Override public void fingerprint( ActionKeyContext actionKeyContext, Fingerprint fp, boolean digestAbsolutePaths) { - runfiles.fingerprint(actionKeyContext, fp, digestAbsolutePaths); + runfiles.fingerprint(actionKeyContext, fp, digestAbsolutePaths, originatingTargetRoot); } @Override
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 7d1c1b1..552fbd2 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
@@ -363,7 +363,11 @@ Fingerprint fp) { fp.addString(GUID); fp.addBoolean(remotableSourceManifestActions); - runfiles.fingerprint(actionKeyContext, fp, manifestWriter.emitsAbsolutePaths()); + runfiles.fingerprint( + actionKeyContext, + fp, + manifestWriter.emitsAbsolutePaths(), + preferTargetConfigurationRunfiles ? getPrimaryOutput().getRoot() : null); fp.addBoolean(repoMappingManifest != null); if (repoMappingManifest != null) { fp.addPath(repoMappingManifest.getExecPath()); @@ -376,7 +380,9 @@ "GUID: %s\nremotableSourceManifestActions: %s\nrunfiles: %s\n", GUID, remotableSourceManifestActions, - runfiles.describeFingerprint(manifestWriter.emitsAbsolutePaths())); + runfiles.describeFingerprint( + manifestWriter.emitsAbsolutePaths(), + preferTargetConfigurationRunfiles ? getPrimaryOutput().getRoot() : null)); } /** Supported manifest writing strategies. */
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 daa47df..7ff56e4 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
@@ -243,7 +243,11 @@ // safe to add more fields in the future. fp.addBoolean(runfiles != null); if (runfiles != null) { - runfiles.fingerprint(actionKeyContext, fp, /* digestAbsolutePaths= */ true); + runfiles.fingerprint( + actionKeyContext, + fp, + /* digestAbsolutePaths= */ true, + preferTargetConfigurationRunfiles ? outputManifest.getRoot() : null); } fp.addBoolean(repoMappingManifest != null); if (repoMappingManifest != null) {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java index 6f5842d..926b857 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -407,4 +408,45 @@ warningPrefixConflictReceiver(), /* repoMappingManifest= */ null, targetConfigRoot)) .containsEntry(PathFragment.create("TESTING/artifact"), anotherConfigArtifact); } + + @Test + public void testFingerprintPreferOriginatingTargetConfiguration() throws Exception { + ArtifactRoot targetConfigRoot = + ArtifactRoot.asDerivedRoot( + scratch.resolve("/execroot"), ArtifactRoot.RootType.OUTPUT, "bin"); + ArtifactRoot otherConfigRoot = + ArtifactRoot.asDerivedRoot( + scratch.resolve("/execroot"), ArtifactRoot.RootType.OUTPUT, "bin-other"); + + Artifact targetConfigArtifact = ActionsTestUtil.createArtifact(targetConfigRoot, "artifact"); + Artifact otherConfigArtifact = ActionsTestUtil.createArtifact(otherConfigRoot, "artifact"); + + Runfiles runfiles = + new Runfiles.Builder("TESTING") + .addArtifact(targetConfigArtifact) + .addArtifact(otherConfigArtifact) + .build(); + + ActionKeyContext actionKeyContext = new ActionKeyContext(); + Fingerprint fp1 = new Fingerprint(); + runfiles.fingerprint(actionKeyContext, fp1, /* digestAbsolutePaths= */ false, targetConfigRoot); + + Fingerprint fp2 = new Fingerprint(); + runfiles.fingerprint(actionKeyContext, fp2, /* digestAbsolutePaths= */ false, otherConfigRoot); + + Fingerprint fpNull = new Fingerprint(); + runfiles.fingerprint(actionKeyContext, fpNull, /* digestAbsolutePaths= */ false, null); + + Fingerprint fpDefault = new Fingerprint(); + runfiles.fingerprint(actionKeyContext, fpDefault, /* digestAbsolutePaths= */ false); + + String fp1Hex = fp1.hexDigestAndReset(); + String fp2Hex = fp2.hexDigestAndReset(); + String fpNullHex = fpNull.hexDigestAndReset(); + String fpDefaultHex = fpDefault.hexDigestAndReset(); + + assertThat(fp1Hex).isNotEqualTo(fp2Hex); + assertThat(fp1Hex).isNotEqualTo(fpNullHex); + assertThat(fpNullHex).isEqualTo(fpDefaultHex); + } }
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 573dcce..ce2b208 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
@@ -460,6 +460,36 @@ } } + @Test + public void testComputeKeyPreferTargetConfigurationRunfiles() throws Exception { + Artifact manifest1 = getBinArtifactWithNoOwner("manifest1"); + Artifact manifest2 = getBinArtifactWithNoOwner("manifest2"); + + Runfiles runfiles = new Runfiles.Builder("TESTING").addArtifact(buildFile).build(); + + SourceManifestAction action1 = + new SourceManifestAction( + ManifestType.SOURCE_SYMLINKS, + NULL_ACTION_OWNER, + manifest1, + runfiles, + /* repoMappingManifest= */ null, + /* remotableSourceManifestActions= */ false, + /* preferTargetConfigurationRunfiles= */ false); + + SourceManifestAction action2 = + new SourceManifestAction( + ManifestType.SOURCE_SYMLINKS, + NULL_ACTION_OWNER, + manifest2, + runfiles, + /* repoMappingManifest= */ null, + /* remotableSourceManifestActions= */ false, + /* preferTargetConfigurationRunfiles= */ true); + + assertThat(computeKey(action2)).isNotEqualTo(computeKey(action1)); + } + private String computeKey(SourceManifestAction action) { Fingerprint fp = new Fingerprint(); action.computeKey(actionKeyContext, /* inputMetadataProvider= */ null, fp);