Skip to content

Commit

Permalink
Rollforward of 7f6e649: Add support for path mapping to CppArchive
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fmeum authored and bazel-io committed Jan 31, 2025
1 parent 4c00ef2 commit f841c22
Show file tree
Hide file tree
Showing 27 changed files with 364 additions and 193 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
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
Expand Up @@ -313,6 +313,7 @@ java_library(
"ArtifactRoot.java",
"Artifacts.java",
"PathMapper.java",
"PathMapperConstants.java",
],
deps = [
":action_lookup_data",
Expand All @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
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
Expand Up @@ -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;
Expand All @@ -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);
}

/**
Expand All @@ -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
Expand All @@ -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.
*
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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",
Expand Down
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
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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}.
*
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -590,7 +589,7 @@ private int addToFingerprint(
fingerprint::addString,
location,
artifactExpander,
PathMappers.forActionKey(outputPathsMode),
PathMapper.forActionKey(outputPathsMode),
starlarkSemantics);
} else {
for (Object value : maybeExpandedValues) {
Expand Down Expand Up @@ -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();
Expand All @@ -1057,7 +1056,7 @@ public void addToFingerprint(
actionKeyContext,
fingerprint,
artifactExpander,
outputPathsMode);
effectiveOutputPathsMode);
} else if (arg == SingleFormattedArg.MARKER) {
argi = SingleFormattedArg.addToFingerprint(arguments, argi, fingerprint);
} else {
Expand Down Expand Up @@ -1205,7 +1204,7 @@ public void expandToCommandLine(Object object, Consumer<String> args)
args,
location,
artifactExpander,
PathMappers.forActionKey(outputPathsMode),
PathMapper.forActionKey(outputPathsMode),
starlarkSemantics);
}

Expand All @@ -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
Expand Down
Loading

0 comments on commit f841c22

Please sign in to comment.