Prefer artifacts in the same configuration as the depending target. When artifacts from the same target in multiple configurations are encountered when constructing runfiles, prefer the version that matches the configuration of the depending target. Specifically, when a target `T` depends on a target `A` in multiple configurations, either directly or transitively, prefer the runfiles from the version of `A` in the configuration that matches the configuration of `T`. This can happen, for example, when a target appears in both the tools and data attributes. This new behavior is enabled with `--incompatible_prefer_depending_configuration_runfiles`. PiperOrigin-RevId: 966777482 Change-Id: I64927d0265dcb20269e7da687fc2c2d2cae28d05
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 a62116b..f6cbe18 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
@@ -27,6 +27,7 @@ import com.google.common.collect.Streams; 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.CommandLineItem; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.collect.nestedset.Depset; @@ -327,7 +328,18 @@ */ public SortedMap<PathFragment, Artifact> getRunfilesInputs( @Nullable Artifact repoMappingManifest) { - return getRunfilesInputs(RunfilesConflictReceiver.NO_OP, repoMappingManifest); + return getRunfilesInputs(RunfilesConflictReceiver.NO_OP, repoMappingManifest, null); + } + + public SortedMap<PathFragment, Artifact> getRunfilesInputs( + @Nullable Artifact repoMappingManifest, @Nullable ArtifactRoot originatingTargetRoot) { + return getRunfilesInputs( + RunfilesConflictReceiver.NO_OP, repoMappingManifest, originatingTargetRoot); + } + + SortedMap<PathFragment, Artifact> getRunfilesInputs( + RunfilesConflictReceiver receiver, @Nullable Artifact repoMappingManifest) { + return getRunfilesInputs(receiver, repoMappingManifest, null); } /** @@ -337,17 +349,22 @@ * @param repoMappingManifest repository mapping manifest to add as a root symlink. This manifest * has to be added automatically for every executable and is thus not part of the Runfiles * advertised by a configured target. + * @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. The configuration of the depending target is preferred. * @return Map<PathFragment, Artifact> path fragment to artifact, of normal source tree entries * and elements that live outside the source tree. Null values represent empty input files. */ SortedMap<PathFragment, Artifact> getRunfilesInputs( - RunfilesConflictReceiver receiver, @Nullable Artifact repoMappingManifest) { + RunfilesConflictReceiver receiver, + @Nullable Artifact repoMappingManifest, + @Nullable ArtifactRoot originatingTargetRoot) { Map<PathFragment, Artifact> manifest = new LinkedHashMap<>(); for (SymlinkEntry entry : symlinks.toList()) { - checkAndPut(manifest, receiver, entry.getPath(), entry.getArtifact()); + checkAndPut(manifest, receiver, entry.getPath(), entry.getArtifact(), originatingTargetRoot); } for (Artifact artifact : artifacts.toList()) { - checkAndPut(manifest, receiver, artifact.getRunfilesPath(), artifact); + checkAndPut(manifest, receiver, artifact.getRunfilesPath(), artifact, originatingTargetRoot); } manifest = filterListForObscuringSymlinks(receiver, manifest); @@ -382,11 +399,17 @@ for (SymlinkEntry entry : rootSymlinks.toList()) { sawWorkspaceName |= entry.getPath().startsWith(workspaceName); - checkAndPut(finalManifest, receiver, entry.getPath(), entry.getArtifact()); + checkAndPut( + finalManifest, receiver, entry.getPath(), entry.getArtifact(), originatingTargetRoot); } if (repoMappingManifest != null) { - checkAndPut(finalManifest, receiver, REPO_MAPPING_PATH_FRAGMENT, repoMappingManifest); + checkAndPut( + finalManifest, + receiver, + REPO_MAPPING_PATH_FRAGMENT, + repoMappingManifest, + originatingTargetRoot); } if (!sawWorkspaceName) { @@ -481,11 +504,22 @@ Map<PathFragment, Artifact> map, RunfilesConflictReceiver receiver, PathFragment path, - @Nullable Artifact artifact) { + @Nullable Artifact artifact, + @Nullable ArtifactRoot originatingTargetRoot) { if (artifact != null && artifact.isRunfilesTree()) { receiver.nestedRunfilesTree(artifact); } else { - map.put(path, artifact); + Artifact existing = map.put(path, artifact); + if (existing != null && originatingTargetRoot != null && artifact != null) { + // Found multiple artifacts at the same path. This means the same artifact in multiple + // configurations, so prefer the one that is in the same configuration as the overall target + // if possible. Using the artifact root is an indirect way to match configurations. + boolean existingMatches = existing.getRoot().equals(originatingTargetRoot); + boolean newMatches = artifact.getRoot().equals(originatingTargetRoot); + if (!newMatches && existingMatches) { + map.put(path, existing); // keep the existing artifact + } + } } }
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 16ff667..be1276a 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
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.ActionEnvironment; 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.CommandLine; import com.google.devtools.build.lib.actions.CommandLine.FlatCommandLine; import com.google.devtools.build.lib.actions.RunfilesTree; @@ -95,6 +96,7 @@ private final PathFragment execPath; private final Runfiles runfiles; @Nullable private final Artifact repoMappingManifest; + @Nullable private final ArtifactRoot originatingTargetRoot; /** * The cached runfiles mapping. Possible values: @@ -119,12 +121,14 @@ PathFragment execPath, Runfiles runfiles, @Nullable Artifact repoMappingManifest, + @Nullable ArtifactRoot originatingTargetRoot, boolean buildRunfileLinks, boolean cacheMapping, RunfileSymlinksMode runfileSymlinksMode) { this.execPath = execPath; this.runfiles = runfiles; this.repoMappingManifest = repoMappingManifest; + this.originatingTargetRoot = originatingTargetRoot; this.buildRunfileLinks = buildRunfileLinks; this.runfileSymlinksMode = runfileSymlinksMode; this.cachedMapping = cacheMapping ? NOT_YET_COMPUTED : null; @@ -136,6 +140,7 @@ execPath, runfiles, /* repoMappingManifest= */ null, + /* originatingTargetRoot= */ null, /* buildRunfileLinks= */ false, /* cacheMapping= */ false, RunfileSymlinksMode.CREATE); @@ -149,7 +154,7 @@ @Override public SortedMap<PathFragment, Artifact> getMapping() { if (cachedMapping == null) { - return runfiles.getRunfilesInputs(repoMappingManifest); + return runfiles.getRunfilesInputs(repoMappingManifest, originatingTargetRoot); } SortedMap<PathFragment, Artifact> result = cachedMapping.get(); @@ -163,7 +168,7 @@ return result; } - result = runfiles.getRunfilesInputs(repoMappingManifest); + result = runfiles.getRunfilesInputs(repoMappingManifest, originatingTargetRoot); cachedMapping = new WeakReference<>(result); return result; } @@ -324,11 +329,18 @@ runfilesManifest = null; } + boolean preferTargetConfigurationRunfiles = + ruleContext + .getConfiguration() + .getOptions() + .get(CoreOptions.class) + .getPreferDependingConfigurationRunfiles(); RunfilesTreeImpl runfilesTree = new RunfilesTreeImpl( runfilesTreeArtifact.getExecPath(), runfiles, repoMappingManifest, + preferTargetConfigurationRunfiles ? runfilesTreeArtifact.getRoot() : null, buildRunfileLinks, cacheRunfilesMappings(ruleContext), runfileSymlinksMode); @@ -515,7 +527,12 @@ inputManifest, runfiles, repoMappingManifest, - context.getConfiguration().remotableSourceManifestActions())); + context.getConfiguration().remotableSourceManifestActions(), + context + .getConfiguration() + .getOptions() + .get(CoreOptions.class) + .getPreferDependingConfigurationRunfiles())); if (!createSymlinks) { // Just return the manifest if that's all the build calls for. @@ -557,11 +574,18 @@ RuleContext ruleContext, Artifact runfilesTreeArtifact, Runfiles runfiles) { // We always want symlinks to be created because that's the point of a symlink tree. boolean buildRunfilesLinks = true; + boolean preferTargetConfigurationRunfiles = + ruleContext + .getConfiguration() + .getOptions() + .get(CoreOptions.class) + .getPreferDependingConfigurationRunfiles(); RunfilesTreeImpl runfilesTree = new RunfilesTreeImpl( runfilesTreeArtifact.getExecPath(), runfiles, null, + preferTargetConfigurationRunfiles ? runfilesTreeArtifact.getRoot() : null, buildRunfilesLinks, false, ruleContext.getConfiguration().getRunfileSymlinksMode());
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 886c7fd..7d1c1b1 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
@@ -129,6 +129,7 @@ private final Runfiles runfiles; private final boolean remotableSourceManifestActions; + private final boolean preferTargetConfigurationRunfiles; private NestedSet<Artifact> symlinkArtifacts = null; @@ -164,12 +165,31 @@ Runfiles runfiles, @Nullable Artifact repoMappingManifest, boolean remotableSourceManifestActions) { + this( + manifestWriter, + owner, + primaryOutput, + runfiles, + repoMappingManifest, + remotableSourceManifestActions, + /* preferTargetConfigurationRunfiles= */ false); + } + + public SourceManifestAction( + ManifestWriter manifestWriter, + ActionOwner owner, + Artifact primaryOutput, + Runfiles runfiles, + @Nullable Artifact repoMappingManifest, + boolean remotableSourceManifestActions, + boolean preferTargetConfigurationRunfiles) { // The real set of inputs is computed in #getInputs(). super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput); this.manifestWriter = manifestWriter; this.runfiles = runfiles; this.repoMappingManifest = repoMappingManifest; this.remotableSourceManifestActions = remotableSourceManifestActions; + this.preferTargetConfigurationRunfiles = preferTargetConfigurationRunfiles; } /** @@ -209,7 +229,11 @@ @VisibleForTesting public void writeTo(OutputStream out, @Nullable EventHandler eventHandler) throws IOException { writeFile( - out, runfiles.getRunfilesInputs(repoMappingManifest), /* inputMetadataProvider= */ null); + out, + runfiles.getRunfilesInputs( + repoMappingManifest, + preferTargetConfigurationRunfiles ? getPrimaryOutput().getRoot() : null), + /* inputMetadataProvider= */ null); } /** @@ -257,7 +281,10 @@ }; Map<PathFragment, Artifact> runfilesInputs = - runfiles.getRunfilesInputs(receiver, repoMappingManifest); + runfiles.getRunfilesInputs( + receiver, + repoMappingManifest, + preferTargetConfigurationRunfiles ? getPrimaryOutput().getRoot() : null); eventHandler.replayOn(ctx.getEventHandler()); if (seenNestedRunfilesTree[0]) { FailureDetail failureDetail =
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 4f3527b..daa47df 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
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; @@ -58,6 +59,7 @@ // Exactly one of these two fields is non-null. @Nullable private final Runfiles runfiles; @Nullable private final String workspaceNameForFileset; + private final boolean preferTargetConfigurationRunfiles; /** * Creates SymlinkTreeAction instance. @@ -85,7 +87,8 @@ repoMappingManifest, config.getActionEnvironment(), config.getRunfileSymlinksMode(), - config.getWorkspaceName()); + config.getWorkspaceName(), + config.getOptions().get(CoreOptions.class).getPreferDependingConfigurationRunfiles()); } /** @@ -111,6 +114,28 @@ ActionEnvironment env, RunfileSymlinksMode runfileSymlinksMode, String workspaceName) { + this( + owner, + inputManifest, + runfiles, + outputManifest, + repoMappingManifest, + env, + runfileSymlinksMode, + workspaceName, + /* preferTargetConfigurationRunfiles= */ false); + } + + public SymlinkTreeAction( + ActionOwner owner, + Artifact inputManifest, + @Nullable Runfiles runfiles, + Artifact outputManifest, + @Nullable Artifact repoMappingManifest, + ActionEnvironment env, + RunfileSymlinksMode runfileSymlinksMode, + String workspaceName, + boolean preferTargetConfigurationRunfiles) { super( owner, computeInputs(runfileSymlinksMode, runfiles, inputManifest, repoMappingManifest), @@ -121,6 +146,7 @@ this.runfileSymlinksMode = runfileSymlinksMode; this.inputManifest = inputManifest; this.repoMappingManifest = repoMappingManifest; + this.preferTargetConfigurationRunfiles = preferTargetConfigurationRunfiles; if (inputManifest.isFileset()) { checkArgument(runfiles == null, "Runfiles present for fileset %s", inputManifest); this.runfiles = null; @@ -205,6 +231,7 @@ fp.addString(GUID); fp.addNullableString(workspaceNameForFileset); fp.addInt(runfileSymlinksMode.ordinal()); + fp.addBoolean(preferTargetConfigurationRunfiles); env.addTo(fp); // We need to ensure that the fingerprints for two different instances of this action are // different. Consider the hypothetical scenario where we add a second runfiles object to this @@ -251,4 +278,8 @@ public boolean mayInsensitivelyPropagateInputs() { return true; } + + public boolean isPreferTargetConfigurationRunfiles() { + return preferTargetConfigurationRunfiles; + } }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 960cc4c..8305789 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -557,6 +557,23 @@ public abstract boolean getAlwaysIncludeFilesToBuildInData(); @Option( + name = "incompatible_prefer_depending_configuration_runfiles", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + """ + If true, when artifacts from the same target in multiple configurations are encountered + when constructing runfiles, prefer the version that matches the configuration of the + depending target. Specifically, when a target T depends on a target A in multiple + configurations, either directly or transitively, prefer the runfiles from the + version of A in the configuration that matches the configuration of T. This can happen, + for example, when a target appears in both the tools and data attributes. + """) + public abstract boolean getPreferDependingConfigurationRunfiles(); + + @Option( name = "incompatible_compact_repo_mapping_manifest", defaultValue = "true", documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java index 52b2d20..d79e087 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
@@ -111,7 +111,13 @@ private static Map<PathFragment, Artifact> getRunfilesMap(SymlinkTreeAction action) { // This call outputs warnings about overlapping symlinks. However, since this has already been // called by the SourceManifestAction, we silence the warnings here. - return action.getRunfiles().getRunfilesInputs(action.getRepoMappingManifest()); + return action + .getRunfiles() + .getRunfilesInputs( + action.getRepoMappingManifest(), + action.isPreferTargetConfigurationRunfiles() + ? action.getOutputManifest().getRoot() + : null); } private SymlinkTreeHelper createSymlinkTreeHelper(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java index 16f36ce..4f0149f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java
@@ -287,7 +287,12 @@ output, runfiles, /* repoMappingManifest= */ null, - ruleContext.getConfiguration().remotableSourceManifestActions())); + ruleContext.getConfiguration().remotableSourceManifestActions(), + ruleContext + .getConfiguration() + .getOptions() + .get(CoreOptions.class) + .getPreferDependingConfigurationRunfiles())); } @StarlarkMethod(
diff --git a/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl b/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl index 93a320c..a1a50c2 100644 --- a/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl +++ b/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl
@@ -226,6 +226,7 @@ "//command_line_option:incompatible_filegroup_runfiles_for_data", "//command_line_option:incompatible_bep_cpu_from_platform", "//command_line_option:incompatible_limit_platforms_in_output_dir_to", + "//command_line_option:incompatible_prefer_depending_configuration_runfiles", ], inputs = ["//command_line_option:features"], outputs = [
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 3b2e4c0..6f5842d 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
@@ -355,4 +355,56 @@ .containsExactly( PathFragment.create("my-artifact-empty"), PathFragment.create("my-symlink-empty")); } + + @Test + public void testGetRunfilesInputsPreferOriginatingTargetConfiguration() 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"); + ArtifactRoot anotherConfigRoot = + ArtifactRoot.asDerivedRoot( + scratch.resolve("/execroot"), ArtifactRoot.RootType.OUTPUT, "bin-another"); + + Artifact targetConfigArtifact = ActionsTestUtil.createArtifact(targetConfigRoot, "artifact"); + Artifact otherConfigArtifact = ActionsTestUtil.createArtifact(otherConfigRoot, "artifact"); + Artifact anotherConfigArtifact = ActionsTestUtil.createArtifact(anotherConfigRoot, "artifact"); + + // Case 1: The preferred artifact is added last in the builder + Runfiles runfilesLast = + new Runfiles.Builder("TESTING") + .addArtifact(otherConfigArtifact) + .addArtifact(targetConfigArtifact) + .build(); + assertThat( + runfilesLast.getRunfilesInputs( + warningPrefixConflictReceiver(), /* repoMappingManifest= */ null, targetConfigRoot)) + .containsEntry(PathFragment.create("TESTING/artifact"), targetConfigArtifact); + + // Case 2: The preferred artifact is added first in the builder + Runfiles runfilesFirst = + new Runfiles.Builder("TESTING") + .addArtifact(targetConfigArtifact) + .addArtifact(otherConfigArtifact) + .build(); + assertThat( + runfilesFirst.getRunfilesInputs( + warningPrefixConflictReceiver(), /* repoMappingManifest= */ null, targetConfigRoot)) + .containsEntry(PathFragment.create("TESTING/artifact"), targetConfigArtifact); + + // Case 3: Neither artifact has the same artifact root as the originating target, and the last + // one should be kept + Runfiles runfilesNeither = + new Runfiles.Builder("TESTING") + .addArtifact(otherConfigArtifact) + .addArtifact(anotherConfigArtifact) + .build(); + assertThat( + runfilesNeither.getRunfilesInputs( + warningPrefixConflictReceiver(), /* repoMappingManifest= */ null, targetConfigRoot)) + .containsEntry(PathFragment.create("TESTING/artifact"), anotherConfigArtifact); + } }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java index ecba324..994215d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java
@@ -41,7 +41,8 @@ private enum RunfilesActionAttributes { RUNFILES, FIXED_ENVIRONMENT, - VARIABLE_ENVIRONMENT + VARIABLE_ENVIRONMENT, + PREFER_TARGET_CONFIGURATION_RUNFILES } @Test @@ -75,7 +76,9 @@ attributesToFlip.contains(RunfilesActionAttributes.FIXED_ENVIRONMENT), attributesToFlip.contains(RunfilesActionAttributes.VARIABLE_ENVIRONMENT)), runfileSymlinksMode, - "workspace")); + "workspace", + attributesToFlip.contains( + RunfilesActionAttributes.PREFER_TARGET_CONFIGURATION_RUNFILES))); tester = tester.combinations(