Skip to content

Commit 08f0709

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Rollforward of 7f6e649: Add support for path mapping to CppArchive
This mostly requires wiring up the existing machinery for structured variables and (most) link build variables. Utility methods on `PathMappers` are moved to `PathMapper` so that they can be dependend on by `AbstractCommandLine` without creating a cycle. NEW: - Allow toolchain variables to be None and skip them in that case, matching the previous behavior. - Use the effective output paths mode for fingerprinting to preserve action cache entries for non-path mapped actions even when path mapping is generally enabled. Closes #25154. PiperOrigin-RevId: 721850758 Change-Id: I34a66006ba4c8fd73d62ce8cea249577f62a1b02
1 parent fe7b4ab commit 08f0709

27 files changed

+364
-193
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,12 @@ public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper
5353
public void addToFingerprint(
5454
ActionKeyContext actionKeyContext,
5555
@Nullable ArtifactExpander artifactExpander,
56-
CoreOptions.OutputPathsMode outputPathsMode,
56+
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
5757
Fingerprint fingerprint)
5858
throws CommandLineExpansionException, InterruptedException {
59-
for (String s : arguments()) {
59+
for (String s :
60+
arguments(
61+
/* artifactExpander= */ null, PathMapper.forActionKey(effectiveOutputPathsMode))) {
6062
fingerprint.addString(s);
6163
}
6264
}

‎src/main/java/com/google/devtools/build/lib/actions/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,7 @@ java_library(
311311
"ArtifactRoot.java",
312312
"Artifacts.java",
313313
"PathMapper.java",
314+
"PathMapperConstants.java",
314315
],
315316
deps = [
316317
":action_lookup_data",
@@ -320,6 +321,7 @@ java_library(
320321
":commandline_item",
321322
":package_roots",
322323
"//src/main/java/com/google/devtools/build/docgen/annot",
324+
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
323325
"//src/main/java/com/google/devtools/build/lib/cmdline",
324326
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
325327
"//src/main/java/com/google/devtools/build/lib/collect/nestedset:fingerprint_cache",

‎src/main/java/com/google/devtools/build/lib/actions/CommandLine.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public abstract Iterable<String> arguments(
9999
public abstract void addToFingerprint(
100100
ActionKeyContext actionKeyContext,
101101
@Nullable ArtifactExpander artifactExpander,
102-
CoreOptions.OutputPathsMode outputPathsMode,
102+
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
103103
Fingerprint fingerprint)
104104
throws CommandLineExpansionException, InterruptedException;
105105

‎src/main/java/com/google/devtools/build/lib/actions/CommandLines.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,15 +172,15 @@ public ImmutableList<String> allArguments(PathMapper pathMapper)
172172
public void addToFingerprint(
173173
ActionKeyContext actionKeyContext,
174174
@Nullable ArtifactExpander artifactExpander,
175-
CoreOptions.OutputPathsMode outputPathsMode,
175+
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
176176
Fingerprint fingerprint)
177177
throws CommandLineExpansionException, InterruptedException {
178178
ImmutableList<CommandLineAndParamFileInfo> commandLines = unpack();
179179
for (CommandLineAndParamFileInfo pair : commandLines) {
180180
CommandLine commandLine = pair.commandLine;
181181
ParamFileInfo paramFileInfo = pair.paramFileInfo;
182182
commandLine.addToFingerprint(
183-
actionKeyContext, artifactExpander, outputPathsMode, fingerprint);
183+
actionKeyContext, artifactExpander, effectiveOutputPathsMode, fingerprint);
184184
if (paramFileInfo != null) {
185185
addParamFileInfoToFingerprint(paramFileInfo, fingerprint);
186186
}

‎src/main/java/com/google/devtools/build/lib/actions/PathMapper.java‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,10 @@
1414

1515
package com.google.devtools.build.lib.actions;
1616

17-
import com.github.benmanes.caffeine.cache.Caffeine;
18-
import com.github.benmanes.caffeine.cache.LoadingCache;
1917
import com.google.devtools.build.docgen.annot.DocCategory;
2018
import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn;
2119
import com.google.devtools.build.lib.actions.CommandLineItem.MapFn;
20+
import com.google.devtools.build.lib.analysis.config.CoreOptions;
2221
import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi;
2322
import com.google.devtools.build.lib.vfs.PathFragment;
2423
import com.google.errorprone.annotations.CheckReturnValue;
@@ -45,7 +44,7 @@ public interface PathMapper {
4544
* {@link #storeIn(StarlarkSemantics)}.
4645
*/
4746
static PathMapper loadFrom(StarlarkSemantics semantics) {
48-
return semantics.get(SEMANTICS_KEY);
47+
return semantics.get(PathMapperConstants.SEMANTICS_KEY);
4948
}
5049

5150
/**
@@ -65,10 +64,11 @@ static PathMapper loadFrom(StarlarkSemantics semantics) {
6564
default StarlarkSemantics storeIn(StarlarkSemantics semantics) {
6665
// This in particular covers the case where the semantics do not have a path mapper yet and this
6766
// is NOOP.
68-
if (semantics.get(SEMANTICS_KEY) == this) {
67+
if (semantics.get(PathMapperConstants.SEMANTICS_KEY) == this) {
6968
return semantics;
7069
}
71-
return new StarlarkSemantics(semantics.toBuilder().set(SEMANTICS_KEY, this).build()) {
70+
return new StarlarkSemantics(
71+
semantics.toBuilder().set(PathMapperConstants.SEMANTICS_KEY, this).build()) {
7272
// The path mapper doesn't affect which fields or methods are available on any given Starlark
7373
// object; it just affects the behavior of certain methods on Artifact. We thus preserve the
7474
// original semantics as a cache key. Otherwise, even if PathMapper#equals returned true for
@@ -82,6 +82,13 @@ public StarlarkSemantics getStarlarkClassDescriptorCacheKey() {
8282
};
8383
}
8484

85+
/** Returns the instance to use during action key computation. */
86+
static PathMapper forActionKey(CoreOptions.OutputPathsMode effectiveOutputPathsMode) {
87+
return effectiveOutputPathsMode == CoreOptions.OutputPathsMode.OFF
88+
? NOOP
89+
: PathMapperConstants.FOR_FINGERPRINTING;
90+
}
91+
8592
/**
8693
* Returns the exec path with the path mapping applied.
8794
*
@@ -142,7 +149,7 @@ default FileRootApi mapRoot(Artifact artifact) {
142149
if (root.isSourceRoot()) {
143150
// Source roots' paths are never mapped, but we still need to wrap them in a
144151
// MappedArtifactRoot to ensure correct Starlark comparison behavior.
145-
return mappedSourceRoots.get(root);
152+
return PathMapperConstants.mappedSourceRoots.get(root);
146153
}
147154
// It would *not* be correct to just apply #map to the exec path of the root: The root part of
148155
// the mapped exec path of this artifact may depend on its complete exec path as well as on e.g.
@@ -192,15 +199,6 @@ public FileRootApi mapRoot(Artifact artifact) {
192199
}
193200
};
194201

195-
StarlarkSemantics.Key<PathMapper> SEMANTICS_KEY =
196-
new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP);
197-
198-
// Not meant for use outside this interface.
199-
LoadingCache<ArtifactRoot, MappedArtifactRoot> mappedSourceRoots =
200-
Caffeine.newBuilder()
201-
.weakKeys()
202-
.build(sourceRoot -> new MappedArtifactRoot(sourceRoot.getExecPath()));
203-
204202
/** A {@link FileRootApi} returned by {@link PathMapper#mapRoot(Artifact)}. */
205203
@StarlarkBuiltin(
206204
name = "mapped_root",

‎src/main/java/com/google/devtools/build/lib/actions/PathMapperConstants.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright 2025 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.devtools.build.lib.actions;
16+
17+
import com.github.benmanes.caffeine.cache.Caffeine;
18+
import com.github.benmanes.caffeine.cache.LoadingCache;
19+
import com.google.devtools.build.lib.vfs.PathFragment;
20+
import net.starlark.java.eval.StarlarkSemantics;
21+
22+
/** Holder class for symbols used by the PathMapper interface that shouldn't be public. */
23+
final class PathMapperConstants {
24+
25+
public static final StarlarkSemantics.Key<PathMapper> SEMANTICS_KEY =
26+
new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP);
27+
public static final LoadingCache<ArtifactRoot, PathMapper.MappedArtifactRoot> mappedSourceRoots =
28+
Caffeine.newBuilder()
29+
.weakKeys()
30+
.build(sourceRoot -> new PathMapper.MappedArtifactRoot(sourceRoot.getExecPath()));
31+
32+
private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out");
33+
private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out");
34+
35+
/**
36+
* A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally
37+
* enabled for an action.
38+
*
39+
* <p>When computing an action key, the following approaches to taking path mapping into account
40+
* do <b>not</b> work:
41+
*
42+
* <ul>
43+
* <li>Using the actual path mapper is prohibitive since constructing it requires checking for
44+
* collisions among the action input's paths when computing the action key, which flattens
45+
* the input depsets of all actions that opt into path mapping and also increases CPU usage.
46+
* <li>Unconditionally using {@link
47+
* com.google.devtools.build.lib.analysis.actions.StrippingPathMapper} can result in stale
48+
* action keys when an action is opted out of path mapping at execution time due to input
49+
* path collisions after stripping. See path_mapping_test for an example.
50+
* <li>Using {@link PathMapper#NOOP} does not distinguish between map_each results built from
51+
* strings and those built from {@link
52+
* com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}.
53+
* While the latter will be mapped at execution time, the former won't, resulting in the
54+
* same digest for actions that behave differently at execution time. This is covered by
55+
* tests in StarlarkRuleImplementationFunctionsTest.
56+
* </ul>
57+
*
58+
* <p>Instead, we use a special path mapping instance that preserves the equality relations
59+
* between the original config segments, but prepends a fixed string to distinguish hard-coded
60+
* path strings from mapped paths. This relies on actions using path mapping to be "root
61+
* agnostic": they must not contain logic that depends on any particular (output) root path.
62+
*/
63+
static final PathMapper FOR_FINGERPRINTING =
64+
execPath -> {
65+
if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) {
66+
// This is not an output path.
67+
return execPath;
68+
}
69+
String execPathString = execPath.getPathString();
70+
int startOfConfigSegment = execPathString.indexOf('/') + 1;
71+
if (startOfConfigSegment == 0) {
72+
return execPath;
73+
}
74+
return PathFragment.createAlreadyNormalized(
75+
execPathString.substring(0, startOfConfigSegment)
76+
+ "pm-"
77+
+ execPathString.substring(startOfConfigSegment));
78+
};
79+
80+
private PathMapperConstants() {}
81+
}

‎src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1360,7 +1360,7 @@ Object substituteTreeFileArtifactArgvFragment(TreeFileArtifactArgvFragment argvF
13601360
public void addToFingerprint(
13611361
ActionKeyContext actionKeyContext,
13621362
@Nullable ArtifactExpander artifactExpander,
1363-
CoreOptions.OutputPathsMode outputPathsMode,
1363+
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
13641364
Fingerprint fingerprint)
13651365
throws CommandLineExpansionException, InterruptedException {
13661366
List<Object> arguments = rawArgsAsList();

‎src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java‎

Lines changed: 6 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -40,49 +40,6 @@
4040
* PathMapper}).
4141
*/
4242
public final class PathMappers {
43-
private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out");
44-
private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out");
45-
46-
/**
47-
* A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally
48-
* enabled for an action.
49-
*
50-
* <p>When computing an action key, the following approaches to taking path mapping into account
51-
* do <b>not</b> work:
52-
*
53-
* <ul>
54-
* <li>Using the actual path mapper is prohibitive since constructing it requires checking for
55-
* collisions among the action input's paths when computing the action key, which flattens
56-
* the input depsets of all actions that opt into path mapping and also increases CPU usage.
57-
* <li>Unconditionally using {@link StrippingPathMapper} can result in stale action keys when an
58-
* action is opted out of path mapping at execution time due to input path collisions after
59-
* stripping. See path_mapping_test for an example.
60-
* <li>Using {@link PathMapper#NOOP} does not distinguish between map_each results built from
61-
* strings and those built from {@link
62-
* com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}.
63-
* While the latter will be mapped at execution time, the former won't, resulting in the
64-
* same digest for actions that behave differently at execution time. This is covered by
65-
* tests in StarlarkRuleImplementationFunctionsTest.
66-
* </ul>
67-
*
68-
* <p>Instead, we use a special path mapping instance that preserves the equality relations
69-
* between the original config segments, but prepends a fixed string to distinguish hard-coded
70-
* path strings from mapped paths. This relies on actions using path mapping to be "root
71-
* agnostic": they must not contain logic that depends on any particular (output) root path.
72-
*/
73-
private static final PathMapper FOR_FINGERPRINTING =
74-
execPath -> {
75-
if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) {
76-
// This is not an output path.
77-
return execPath;
78-
}
79-
String execPathString = execPath.getPathString();
80-
int startOfConfigSegment = execPathString.indexOf('/') + 1;
81-
return PathFragment.createAlreadyNormalized(
82-
execPathString.substring(0, startOfConfigSegment)
83-
+ "pm-"
84-
+ execPathString.substring(startOfConfigSegment));
85-
};
8643

8744
// TODO: Remove actions from this list by adding ExecutionRequirements.SUPPORTS_PATH_MAPPING to
8845
// their execution info instead.
@@ -136,13 +93,6 @@ public static void addToFingerprint(
13693
}
13794
}
13895

139-
/** Returns the instance to use during action key computation. */
140-
public static PathMapper forActionKey(OutputPathsMode outputPathsMode) {
141-
return outputPathsMode == OutputPathsMode.OFF
142-
? PathMapper.NOOP
143-
: PathMappers.FOR_FINGERPRINTING;
144-
}
145-
14696
/**
14797
* Actions that support path mapping should call this method when creating their {@link Spawn}.
14898
*
@@ -190,7 +140,12 @@ public static OutputPathsMode getOutputPathsMode(
190140
return configuration.getOptions().get(CoreOptions.class).outputPathsMode;
191141
}
192142

193-
private static OutputPathsMode getEffectiveOutputPathsMode(
143+
/**
144+
* Returns the effective {@link OutputPathsMode} for an action based on the action's mnemonic and
145+
* execution info. This may return a mode other than {@link OutputPathsMode#OFF} even though path
146+
* mapping will be disabled during execution due to path collisions.
147+
*/
148+
public static OutputPathsMode getEffectiveOutputPathsMode(
194149
OutputPathsMode outputPathsMode, String mnemonic, Map<String, String> executionInfo) {
195150
if (executionInfo.containsKey(ExecutionRequirements.LOCAL)
196151
|| (executionInfo.containsKey(ExecutionRequirements.NO_SANDBOX)

‎src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,11 @@ protected void computeKey(
373373
Fingerprint fp)
374374
throws CommandLineExpansionException, InterruptedException {
375375
fp.addString(GUID);
376-
commandLines.addToFingerprint(actionKeyContext, artifactExpander, outputPathsMode, fp);
376+
commandLines.addToFingerprint(
377+
actionKeyContext,
378+
artifactExpander,
379+
PathMappers.getEffectiveOutputPathsMode(outputPathsMode, getMnemonic(), getExecutionInfo()),
380+
fp);
377381
fp.addString(mnemonic);
378382
env.addTo(fp);
379383
fp.addStringMap(getExecutionInfo());

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError;
4242
import com.google.devtools.build.lib.actions.PathMapper;
4343
import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
44-
import com.google.devtools.build.lib.analysis.actions.PathMappers;
4544
import com.google.devtools.build.lib.analysis.config.CoreOptions;
4645
import com.google.devtools.build.lib.cmdline.Label;
4746
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -575,7 +574,7 @@ private int addToFingerprint(
575574
maybeExpandDirectories(
576575
artifactExpander,
577576
arguments.subList(argi, argi + count),
578-
PathMappers.forActionKey(outputPathsMode));
577+
PathMapper.forActionKey(outputPathsMode));
579578
argi += count;
580579
if (mapEach != null) {
581580
// TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase)
@@ -590,7 +589,7 @@ private int addToFingerprint(
590589
fingerprint::addString,
591590
location,
592591
artifactExpander,
593-
PathMappers.forActionKey(outputPathsMode),
592+
PathMapper.forActionKey(outputPathsMode),
594593
starlarkSemantics);
595594
} else {
596595
for (Object value : maybeExpandedValues) {
@@ -1034,7 +1033,7 @@ public ArgChunk expand(@Nullable ArtifactExpander artifactExpander, PathMapper p
10341033
public void addToFingerprint(
10351034
ActionKeyContext actionKeyContext,
10361035
@Nullable ArtifactExpander artifactExpander,
1037-
CoreOptions.OutputPathsMode outputPathsMode,
1036+
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
10381037
Fingerprint fingerprint)
10391038
throws CommandLineExpansionException, InterruptedException {
10401039
List<Object> arguments = rawArgsAsList();
@@ -1057,7 +1056,7 @@ public void addToFingerprint(
10571056
actionKeyContext,
10581057
fingerprint,
10591058
artifactExpander,
1060-
outputPathsMode);
1059+
effectiveOutputPathsMode);
10611060
} else if (arg == SingleFormattedArg.MARKER) {
10621061
argi = SingleFormattedArg.addToFingerprint(arguments, argi, fingerprint);
10631062
} else {
@@ -1205,7 +1204,7 @@ public void expandToCommandLine(Object object, Consumer<String> args)
12051204
args,
12061205
location,
12071206
artifactExpander,
1208-
PathMappers.forActionKey(outputPathsMode),
1207+
PathMapper.forActionKey(outputPathsMode),
12091208
starlarkSemantics);
12101209
}
12111210

@@ -1215,7 +1214,7 @@ private List<Object> maybeExpandDirectory(Object object) throws CommandLineExpan
12151214
}
12161215

12171216
return VectorArg.expandDirectories(
1218-
artifactExpander, ImmutableList.of(object), PathMappers.forActionKey(outputPathsMode));
1217+
artifactExpander, ImmutableList.of(object), PathMapper.forActionKey(outputPathsMode));
12191218
}
12201219

12211220
@Override

0 commit comments

Comments
 (0)