Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "[8.1.0] Add support for path mapping to CppArchive" #25153

Merged
merged 1 commit into from
Jan 31, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Revert "[8.1.0] Add support for path mapping to CppArchive (#25146)"
This reverts commit 65621ac.
meteorcloudy authored Jan 31, 2025

Verified

This commit was signed with the committer’s verified signature.
jkcdarunday Jan Keith Darunday
commit cdfab051604584032535c68031b4c16c9872c170
Original file line number Diff line number Diff line change
@@ -56,8 +56,7 @@ public void addToFingerprint(
CoreOptions.OutputPathsMode outputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException {
for (String s :
arguments(/* artifactExpander= */ null, PathMapper.forActionKey(outputPathsMode))) {
for (String s : arguments()) {
fingerprint.addString(s);
}
}
2 changes: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
@@ -313,7 +313,6 @@ java_library(
"ArtifactRoot.java",
"Artifacts.java",
"PathMapper.java",
"PathMapperConstants.java",
],
deps = [
":action_lookup_data",
@@ -323,7 +322,6 @@ 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
@@ -14,10 +14,11 @@

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;
@@ -44,7 +45,7 @@ public interface PathMapper {
* {@link #storeIn(StarlarkSemantics)}.
*/
static PathMapper loadFrom(StarlarkSemantics semantics) {
return semantics.get(PathMapperConstants.SEMANTICS_KEY);
return semantics.get(SEMANTICS_KEY);
}

/**
@@ -64,11 +65,10 @@ 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(PathMapperConstants.SEMANTICS_KEY) == this) {
if (semantics.get(SEMANTICS_KEY) == this) {
return semantics;
}
return new StarlarkSemantics(
semantics.toBuilder().set(PathMapperConstants.SEMANTICS_KEY, this).build()) {
return new StarlarkSemantics(semantics.toBuilder().set(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,13 +82,6 @@ public StarlarkSemantics getStarlarkClassDescriptorCacheKey() {
};
}

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

/**
* Returns the exec path with the path mapping applied.
*
@@ -149,7 +142,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 PathMapperConstants.mappedSourceRoots.get(root);
return 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.
@@ -199,6 +192,15 @@ 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",

This file was deleted.

Original file line number Diff line number Diff line change
@@ -40,6 +40,49 @@
* 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.
@@ -93,6 +136,13 @@ 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}.
*
Original file line number Diff line number Diff line change
@@ -41,6 +41,7 @@
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;
@@ -574,7 +575,7 @@ private int addToFingerprint(
maybeExpandDirectories(
artifactExpander,
arguments.subList(argi, argi + count),
PathMapper.forActionKey(outputPathsMode));
PathMappers.forActionKey(outputPathsMode));
argi += count;
if (mapEach != null) {
// TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase)
@@ -589,7 +590,7 @@ private int addToFingerprint(
fingerprint::addString,
location,
artifactExpander,
PathMapper.forActionKey(outputPathsMode),
PathMappers.forActionKey(outputPathsMode),
starlarkSemantics);
} else {
for (Object value : maybeExpandedValues) {
@@ -1204,7 +1205,7 @@ public void expandToCommandLine(Object object, Consumer<String> args)
args,
location,
artifactExpander,
PathMapper.forActionKey(outputPathsMode),
PathMappers.forActionKey(outputPathsMode),
starlarkSemantics);
}

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

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

@Override
Original file line number Diff line number Diff line change
@@ -105,40 +105,24 @@ public CcToolchainVariables getCcToolchainVariables(Dict<?, ?> buildVariables)

CcToolchainVariables.Builder ccToolchainVariables = CcToolchainVariables.builder();
for (var entry : buildVariables.entrySet()) {
String key = (String) entry.getKey();
Object value = entry.getValue();
switch (value) {
case String s -> ccToolchainVariables.addStringVariable(key, s);
case Artifact a -> ccToolchainVariables.addArtifactVariable(key, a);
case Boolean b -> ccToolchainVariables.addBooleanValue(key, b);
case Iterable<?> values -> {
if (key.equals("libraries_to_link")) {
SequenceBuilder sb = new SequenceBuilder();
for (var v : (Iterable<VariableValue>) values) {
sb.addValue(v);
}
ccToolchainVariables.addCustomBuiltVariable(key, sb);
} else {
ccToolchainVariables.addStringSequenceVariable(key, (Iterable<String>) values);
if (entry.getValue() instanceof String) {
ccToolchainVariables.addStringVariable((String) entry.getKey(), (String) entry.getValue());
} else if (entry.getValue() instanceof Boolean) {
ccToolchainVariables.addBooleanValue((String) entry.getKey(), (Boolean) entry.getValue());
} else if (entry.getValue() instanceof Iterable<?>) {
if (entry.getKey().equals("libraries_to_link")) {
SequenceBuilder sb = new SequenceBuilder();
for (var value : (Iterable<?>) entry.getValue()) {
sb.addValue((VariableValue) value);
}
ccToolchainVariables.addCustomBuiltVariable((String) entry.getKey(), sb);
} else {
ccToolchainVariables.addStringSequenceVariable(
(String) entry.getKey(), (Iterable<String>) entry.getValue());
}
case Depset depset -> {
Class<?> type = depset.getElementClass();
// Type doesn't matter for empty depsets.
if (type == String.class || type == null) {
ccToolchainVariables.addStringSequenceVariable(key, depset.getSet(String.class));
} else if (type == Artifact.class) {
ccToolchainVariables.addArtifactSequenceVariable(key, depset.getSet(Artifact.class));
} else if (type == PathFragment.class) {
ccToolchainVariables.addPathFragmentSequenceVariable(
key, depset.getSet(PathFragment.class));
} else {
throw new IllegalStateException("Unexpected depset element type: %s".formatted(type));
}
}
default ->
throw new IllegalStateException(
"Unexpected value: %s (%s)".formatted(value, value.getClass()));
} else if (entry.getValue() instanceof Depset) {
ccToolchainVariables.addStringSequenceVariable(
(String) entry.getKey(), ((Depset) entry.getValue()).getSet(String.class));
}
}
return ccToolchainVariables.build();
Original file line number Diff line number Diff line change
@@ -412,18 +412,18 @@ private boolean canBeExpanded(
}
if (expandIfTrue != null
&& (!variables.isAvailable(expandIfTrue, expander)
|| !variables.getVariable(expandIfTrue, pathMapper).isTruthy())) {
|| !variables.getVariable(expandIfTrue).isTruthy())) {
return false;
}
if (expandIfFalse != null
&& (!variables.isAvailable(expandIfFalse, expander)
|| variables.getVariable(expandIfFalse, pathMapper).isTruthy())) {
|| variables.getVariable(expandIfFalse).isTruthy())) {
return false;
}
if (expandIfEqual != null
&& (!variables.isAvailable(expandIfEqual.variable, expander)
|| !variables
.getVariable(expandIfEqual.variable, pathMapper)
.getVariable(expandIfEqual.variable)
.getStringValue(expandIfEqual.variable, pathMapper)
.equals(expandIfEqual.value))) {
return false;
Loading