Skip to content

Commit 7f6e649

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
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. Work towards #22658 (reply in thread) Work towards #6526 Closes #25081. PiperOrigin-RevId: 721499678 Change-Id: I4551f268093c7b851791a41deb57292b2c23afb7
1 parent 8782cc1 commit 7f6e649

22 files changed

+345
-184
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ public void addToFingerprint(
5656
CoreOptions.OutputPathsMode outputPathsMode,
5757
Fingerprint fingerprint)
5858
throws CommandLineExpansionException, InterruptedException {
59-
for (String s : arguments()) {
59+
for (String s :
60+
arguments(/* artifactExpander= */ null, PathMapper.forActionKey(outputPathsMode))) {
6061
fingerprint.addString(s);
6162
}
6263
}

‎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/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 outputPathsMode) {
87+
return outputPathsMode == 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/PathMappers.java‎

Lines changed: 0 additions & 50 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
*

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

Lines changed: 4 additions & 5 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) {
@@ -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

‎src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java‎

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -111,24 +111,40 @@ public CcToolchainVariables getCcToolchainVariables(Dict<?, ?> buildVariables)
111111

112112
CcToolchainVariables.Builder ccToolchainVariables = CcToolchainVariables.builder();
113113
for (var entry : buildVariables.entrySet()) {
114-
if (entry.getValue() instanceof String) {
115-
ccToolchainVariables.addStringVariable((String) entry.getKey(), (String) entry.getValue());
116-
} else if (entry.getValue() instanceof Boolean) {
117-
ccToolchainVariables.addBooleanValue((String) entry.getKey(), (Boolean) entry.getValue());
118-
} else if (entry.getValue() instanceof Iterable<?>) {
119-
if (entry.getKey().equals("libraries_to_link")) {
120-
SequenceBuilder sb = new SequenceBuilder();
121-
for (var value : (Iterable<?>) entry.getValue()) {
122-
sb.addValue((VariableValue) value);
114+
String key = (String) entry.getKey();
115+
Object value = entry.getValue();
116+
switch (value) {
117+
case String s -> ccToolchainVariables.addStringVariable(key, s);
118+
case Artifact a -> ccToolchainVariables.addArtifactVariable(key, a);
119+
case Boolean b -> ccToolchainVariables.addBooleanValue(key, b);
120+
case Iterable<?> values -> {
121+
if (key.equals("libraries_to_link")) {
122+
SequenceBuilder sb = new SequenceBuilder();
123+
for (var v : (Iterable<VariableValue>) values) {
124+
sb.addValue(v);
125+
}
126+
ccToolchainVariables.addCustomBuiltVariable(key, sb);
127+
} else {
128+
ccToolchainVariables.addStringSequenceVariable(key, (Iterable<String>) values);
123129
}
124-
ccToolchainVariables.addCustomBuiltVariable((String) entry.getKey(), sb);
125-
} else {
126-
ccToolchainVariables.addStringSequenceVariable(
127-
(String) entry.getKey(), (Iterable<String>) entry.getValue());
128130
}
129-
} else if (entry.getValue() instanceof Depset) {
130-
ccToolchainVariables.addStringSequenceVariable(
131-
(String) entry.getKey(), ((Depset) entry.getValue()).getSet(String.class));
131+
case Depset depset -> {
132+
Class<?> type = depset.getElementClass();
133+
// Type doesn't matter for empty depsets.
134+
if (type == String.class || type == null) {
135+
ccToolchainVariables.addStringSequenceVariable(key, depset.getSet(String.class));
136+
} else if (type == Artifact.class) {
137+
ccToolchainVariables.addArtifactSequenceVariable(key, depset.getSet(Artifact.class));
138+
} else if (type == PathFragment.class) {
139+
ccToolchainVariables.addPathFragmentSequenceVariable(
140+
key, depset.getSet(PathFragment.class));
141+
} else {
142+
throw new IllegalStateException("Unexpected depset element type: %s".formatted(type));
143+
}
144+
}
145+
default ->
146+
throw new IllegalStateException(
147+
"Unexpected value: %s (%s)".formatted(value, value.getClass()));
132148
}
133149
}
134150
return ccToolchainVariables.build();

‎src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -412,18 +412,18 @@ private boolean canBeExpanded(
412412
}
413413
if (expandIfTrue != null
414414
&& (!variables.isAvailable(expandIfTrue, expander)
415-
|| !variables.getVariable(expandIfTrue).isTruthy())) {
415+
|| !variables.getVariable(expandIfTrue, pathMapper).isTruthy())) {
416416
return false;
417417
}
418418
if (expandIfFalse != null
419419
&& (!variables.isAvailable(expandIfFalse, expander)
420-
|| variables.getVariable(expandIfFalse).isTruthy())) {
420+
|| variables.getVariable(expandIfFalse, pathMapper).isTruthy())) {
421421
return false;
422422
}
423423
if (expandIfEqual != null
424424
&& (!variables.isAvailable(expandIfEqual.variable, expander)
425425
|| !variables
426-
.getVariable(expandIfEqual.variable)
426+
.getVariable(expandIfEqual.variable, pathMapper)
427427
.getStringValue(expandIfEqual.variable, pathMapper)
428428
.equals(expandIfEqual.value))) {
429429
return false;

0 commit comments

Comments
 (0)