Skip to content

Commit

Permalink
Rollforward of bazelbuild@7f6e649: Add support for path mapping to `C…
Browse files Browse the repository at this point in the history
…ppArchive`

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 bazelbuild#25154.

PiperOrigin-RevId: 721850758
Change-Id: I34a66006ba4c8fd73d62ce8cea249577f62a1b02
  • Loading branch information
fmeum authored and bazel-io committed Jan 31, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 4c00ef2 commit f841c22
Showing 27 changed files with 364 additions and 193 deletions.
Original file line number Diff line number Diff line change
@@ -53,10 +53,12 @@ public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper
public void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException {
for (String s : arguments()) {
for (String s :
arguments(
/* artifactExpander= */ null, PathMapper.forActionKey(effectiveOutputPathsMode))) {
fingerprint.addString(s);
}
}
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
@@ -313,6 +313,7 @@ java_library(
"ArtifactRoot.java",
"Artifacts.java",
"PathMapper.java",
"PathMapperConstants.java",
],
deps = [
":action_lookup_data",
@@ -322,6 +323,7 @@ java_library(
":commandline_item",
":package_roots",
"//src/main/java/com/google/devtools/build/docgen/annot",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset:fingerprint_cache",
Original file line number Diff line number Diff line change
@@ -99,7 +99,7 @@ public abstract Iterable<String> arguments(
public abstract void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException;

Original file line number Diff line number Diff line change
@@ -172,15 +172,15 @@ public ImmutableList<String> allArguments(PathMapper pathMapper)
public void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException {
ImmutableList<CommandLineAndParamFileInfo> commandLines = unpack();
for (CommandLineAndParamFileInfo pair : commandLines) {
CommandLine commandLine = pair.commandLine;
ParamFileInfo paramFileInfo = pair.paramFileInfo;
commandLine.addToFingerprint(
actionKeyContext, artifactExpander, outputPathsMode, fingerprint);
actionKeyContext, artifactExpander, effectiveOutputPathsMode, fingerprint);
if (paramFileInfo != null) {
addParamFileInfoToFingerprint(paramFileInfo, fingerprint);
}
28 changes: 13 additions & 15 deletions src/main/java/com/google/devtools/build/lib/actions/PathMapper.java
Original file line number Diff line number Diff line change
@@ -14,11 +14,10 @@

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

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn;
import com.google.devtools.build.lib.actions.CommandLineItem.MapFn;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CheckReturnValue;
@@ -45,7 +44,7 @@ public interface PathMapper {
* {@link #storeIn(StarlarkSemantics)}.
*/
static PathMapper loadFrom(StarlarkSemantics semantics) {
return semantics.get(SEMANTICS_KEY);
return semantics.get(PathMapperConstants.SEMANTICS_KEY);
}

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

/** Returns the instance to use during action key computation. */
static PathMapper forActionKey(CoreOptions.OutputPathsMode effectiveOutputPathsMode) {
return effectiveOutputPathsMode == CoreOptions.OutputPathsMode.OFF
? NOOP
: PathMapperConstants.FOR_FINGERPRINTING;
}

/**
* Returns the exec path with the path mapping applied.
*
@@ -142,7 +149,7 @@ default FileRootApi mapRoot(Artifact artifact) {
if (root.isSourceRoot()) {
// Source roots' paths are never mapped, but we still need to wrap them in a
// MappedArtifactRoot to ensure correct Starlark comparison behavior.
return mappedSourceRoots.get(root);
return PathMapperConstants.mappedSourceRoots.get(root);
}
// It would *not* be correct to just apply #map to the exec path of the root: The root part of
// 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) {
}
};

StarlarkSemantics.Key<PathMapper> SEMANTICS_KEY =
new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP);

// Not meant for use outside this interface.
LoadingCache<ArtifactRoot, MappedArtifactRoot> mappedSourceRoots =
Caffeine.newBuilder()
.weakKeys()
.build(sourceRoot -> new MappedArtifactRoot(sourceRoot.getExecPath()));

/** A {@link FileRootApi} returned by {@link PathMapper#mapRoot(Artifact)}. */
@StarlarkBuiltin(
name = "mapped_root",
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Copyright 2025 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

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

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.devtools.build.lib.vfs.PathFragment;
import net.starlark.java.eval.StarlarkSemantics;

/** Holder class for symbols used by the PathMapper interface that shouldn't be public. */
final class PathMapperConstants {

public static final StarlarkSemantics.Key<PathMapper> SEMANTICS_KEY =
new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP);
public static final LoadingCache<ArtifactRoot, PathMapper.MappedArtifactRoot> mappedSourceRoots =
Caffeine.newBuilder()
.weakKeys()
.build(sourceRoot -> new PathMapper.MappedArtifactRoot(sourceRoot.getExecPath()));

private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out");
private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out");

/**
* A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally
* enabled for an action.
*
* <p>When computing an action key, the following approaches to taking path mapping into account
* do <b>not</b> work:
*
* <ul>
* <li>Using the actual path mapper is prohibitive since constructing it requires checking for
* collisions among the action input's paths when computing the action key, which flattens
* the input depsets of all actions that opt into path mapping and also increases CPU usage.
* <li>Unconditionally using {@link
* com.google.devtools.build.lib.analysis.actions.StrippingPathMapper} can result in stale
* action keys when an action is opted out of path mapping at execution time due to input
* path collisions after stripping. See path_mapping_test for an example.
* <li>Using {@link PathMapper#NOOP} does not distinguish between map_each results built from
* strings and those built from {@link
* com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}.
* While the latter will be mapped at execution time, the former won't, resulting in the
* same digest for actions that behave differently at execution time. This is covered by
* tests in StarlarkRuleImplementationFunctionsTest.
* </ul>
*
* <p>Instead, we use a special path mapping instance that preserves the equality relations
* between the original config segments, but prepends a fixed string to distinguish hard-coded
* path strings from mapped paths. This relies on actions using path mapping to be "root
* agnostic": they must not contain logic that depends on any particular (output) root path.
*/
static final PathMapper FOR_FINGERPRINTING =
execPath -> {
if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) {
// This is not an output path.
return execPath;
}
String execPathString = execPath.getPathString();
int startOfConfigSegment = execPathString.indexOf('/') + 1;
if (startOfConfigSegment == 0) {
return execPath;
}
return PathFragment.createAlreadyNormalized(
execPathString.substring(0, startOfConfigSegment)
+ "pm-"
+ execPathString.substring(startOfConfigSegment));
};

private PathMapperConstants() {}
}
Original file line number Diff line number Diff line change
@@ -1360,7 +1360,7 @@ Object substituteTreeFileArtifactArgvFragment(TreeFileArtifactArgvFragment argvF
public void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException {
List<Object> arguments = rawArgsAsList();
Original file line number Diff line number Diff line change
@@ -40,49 +40,6 @@
* PathMapper}).
*/
public final class PathMappers {
private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out");
private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out");

/**
* A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally
* enabled for an action.
*
* <p>When computing an action key, the following approaches to taking path mapping into account
* do <b>not</b> work:
*
* <ul>
* <li>Using the actual path mapper is prohibitive since constructing it requires checking for
* collisions among the action input's paths when computing the action key, which flattens
* the input depsets of all actions that opt into path mapping and also increases CPU usage.
* <li>Unconditionally using {@link StrippingPathMapper} can result in stale action keys when an
* action is opted out of path mapping at execution time due to input path collisions after
* stripping. See path_mapping_test for an example.
* <li>Using {@link PathMapper#NOOP} does not distinguish between map_each results built from
* strings and those built from {@link
* com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}.
* While the latter will be mapped at execution time, the former won't, resulting in the
* same digest for actions that behave differently at execution time. This is covered by
* tests in StarlarkRuleImplementationFunctionsTest.
* </ul>
*
* <p>Instead, we use a special path mapping instance that preserves the equality relations
* between the original config segments, but prepends a fixed string to distinguish hard-coded
* path strings from mapped paths. This relies on actions using path mapping to be "root
* agnostic": they must not contain logic that depends on any particular (output) root path.
*/
private static final PathMapper FOR_FINGERPRINTING =
execPath -> {
if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) {
// This is not an output path.
return execPath;
}
String execPathString = execPath.getPathString();
int startOfConfigSegment = execPathString.indexOf('/') + 1;
return PathFragment.createAlreadyNormalized(
execPathString.substring(0, startOfConfigSegment)
+ "pm-"
+ execPathString.substring(startOfConfigSegment));
};

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

/** Returns the instance to use during action key computation. */
public static PathMapper forActionKey(OutputPathsMode outputPathsMode) {
return outputPathsMode == OutputPathsMode.OFF
? PathMapper.NOOP
: PathMappers.FOR_FINGERPRINTING;
}

/**
* Actions that support path mapping should call this method when creating their {@link Spawn}.
*
@@ -190,7 +140,12 @@ public static OutputPathsMode getOutputPathsMode(
return configuration.getOptions().get(CoreOptions.class).outputPathsMode;
}

private static OutputPathsMode getEffectiveOutputPathsMode(
/**
* Returns the effective {@link OutputPathsMode} for an action based on the action's mnemonic and
* execution info. This may return a mode other than {@link OutputPathsMode#OFF} even though path
* mapping will be disabled during execution due to path collisions.
*/
public static OutputPathsMode getEffectiveOutputPathsMode(
OutputPathsMode outputPathsMode, String mnemonic, Map<String, String> executionInfo) {
if (executionInfo.containsKey(ExecutionRequirements.LOCAL)
|| (executionInfo.containsKey(ExecutionRequirements.NO_SANDBOX)
Original file line number Diff line number Diff line change
@@ -373,7 +373,11 @@ protected void computeKey(
Fingerprint fp)
throws CommandLineExpansionException, InterruptedException {
fp.addString(GUID);
commandLines.addToFingerprint(actionKeyContext, artifactExpander, outputPathsMode, fp);
commandLines.addToFingerprint(
actionKeyContext,
artifactExpander,
PathMappers.getEffectiveOutputPathsMode(outputPathsMode, getMnemonic(), getExecutionInfo()),
fp);
fp.addString(mnemonic);
env.addTo(fp);
fp.addStringMap(getExecutionInfo());
Original file line number Diff line number Diff line change
@@ -41,7 +41,6 @@
import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError;
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.analysis.actions.PathMappers;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -575,7 +574,7 @@ private int addToFingerprint(
maybeExpandDirectories(
artifactExpander,
arguments.subList(argi, argi + count),
PathMappers.forActionKey(outputPathsMode));
PathMapper.forActionKey(outputPathsMode));
argi += count;
if (mapEach != null) {
// TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase)
@@ -590,7 +589,7 @@ private int addToFingerprint(
fingerprint::addString,
location,
artifactExpander,
PathMappers.forActionKey(outputPathsMode),
PathMapper.forActionKey(outputPathsMode),
starlarkSemantics);
} else {
for (Object value : maybeExpandedValues) {
@@ -1034,7 +1033,7 @@ public ArgChunk expand(@Nullable ArtifactExpander artifactExpander, PathMapper p
public void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
CoreOptions.OutputPathsMode effectiveOutputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException {
List<Object> arguments = rawArgsAsList();
@@ -1057,7 +1056,7 @@ public void addToFingerprint(
actionKeyContext,
fingerprint,
artifactExpander,
outputPathsMode);
effectiveOutputPathsMode);
} else if (arg == SingleFormattedArg.MARKER) {
argi = SingleFormattedArg.addToFingerprint(arguments, argi, fingerprint);
} else {
@@ -1205,7 +1204,7 @@ public void expandToCommandLine(Object object, Consumer<String> args)
args,
location,
artifactExpander,
PathMappers.forActionKey(outputPathsMode),
PathMapper.forActionKey(outputPathsMode),
starlarkSemantics);
}

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

return VectorArg.expandDirectories(
artifactExpander, ImmutableList.of(object), PathMappers.forActionKey(outputPathsMode));
artifactExpander, ImmutableList.of(object), PathMapper.forActionKey(outputPathsMode));
}

@Override
Loading

0 comments on commit f841c22

Please sign in to comment.