Skip to content

Commit

Permalink
Fix issues with path mapping in map_each functions
Browse files Browse the repository at this point in the history 
Fixes the following issues with path mapping applied to `map_each` functions:
* Action keys could be stale if an action implementation switches from hard-coded artifact strings to `File`s mapped at execution time. This commit introduces a special `PathMapper` instance for fingerprinting, see the long comment on `PathMappers.FOR_FINGERPRINTING`. It includes a number of tests to verify that neither too much nor too little is mapped during action key computation.
* The Starlark objects representing mapped artifact roots didn't have the correct semantics for equality and ordered comparisons. They are now unequal to unmapped roots.
* When not using path mapping, the `StarlarkSemantics` of a thread used to evaluate `map_each` callbacks is not modified.

These changes require splitting `PathMappers` out of `analysis_cluster` and also make `StrippingPathMapper` a subclass of `PathMapper` instead of using an anonymous one.

Closes #22227.

PiperOrigin-RevId: 657449600
Change-Id: If949d23acab163cb192ea3af1baeb7a3caca7339
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Jul 30, 2024
1 parent f0aa097 commit ca6e80e
Show file tree
Hide file tree
Showing 27 changed files with 820 additions and 323 deletions.
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

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

import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.util.Fingerprint;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -52,6 +53,7 @@ public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper
public void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException {
for (String s : arguments()) {
Expand Down
72 changes: 1 addition & 71 deletions src/main/java/com/google/devtools/build/lib/actions/Artifact.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Streams;
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
Expand Down Expand Up @@ -57,7 +56,6 @@
import java.util.List;
import java.util.function.UnaryOperator;
import javax.annotation.Nullable;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.Starlark;
Expand Down Expand Up @@ -519,19 +517,7 @@ public final ArtifactRoot getRoot() {

@Override
public final FileRootApi getRootForStarlark(StarlarkSemantics semantics) {
// It would *not* be correct to just apply PathMapper#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. the digest of the artifact.
PathFragment mappedExecPath = PathMapper.loadFrom(semantics).map(execPath);
if (mappedExecPath.equals(execPath)) {
return root;
}
// PathMapper#map never changes the root-relative part of the exec path, so we can remove that
// suffix to get the mapped root part.
int rootRelativeSegmentCount = execPath.segmentCount() - root.getExecPath().segmentCount();
PathFragment mappedRootExecPath =
mappedExecPath.subFragment(0, mappedExecPath.segmentCount() - rootRelativeSegmentCount);
return new MappedArtifactRoot(mappedRootExecPath);
return PathMapper.loadFrom(semantics).mapRoot(this);
}

@Override
Expand Down Expand Up @@ -1370,60 +1356,4 @@ public String toString() {
return MoreObjects.toStringHelper(this).add("artifact", artifact.toDebugString()).toString();
}
}

/** A {@link FileRootApi} obtained by applying a {@link PathMapper} to an {@link ArtifactRoot}. */
@StarlarkBuiltin(
name = "mapped_root",
category = DocCategory.BUILTIN,
doc = "A root for files that have been subject to path mapping")
private static final class MappedArtifactRoot
implements FileRootApi, Comparable<MappedArtifactRoot> {
private final PathFragment mappedRootExecPath;

MappedArtifactRoot(PathFragment mappedRootExecPath) {
this.mappedRootExecPath = mappedRootExecPath;
}

@Override
public String getExecPathString() {
return mappedRootExecPath.getPathString();
}

@Override
public int compareTo(MappedArtifactRoot otherRoot) {
return mappedRootExecPath.compareTo(otherRoot.mappedRootExecPath);
}

@Override
public boolean equals(Object obj) {
// Per the contract of PathMapper#map, mapped roots never have exec paths that are equal to
// exec paths of non-mapped roots, that is, of instances of ArtifactRoot. Thus, it is correct
// for both equals implementations to return false if the other object is not an instance of
// the respective class.
if (!(obj instanceof MappedArtifactRoot other)) {
return false;
}
return mappedRootExecPath.equals(other.mappedRootExecPath);
}

@Override
public int hashCode() {
return mappedRootExecPath.hashCode();
}

@Override
public String toString() {
return mappedRootExecPath + " [mapped]";
}

@Override
public void repr(Printer printer) {
printer.append("<mapped root>");
}

@Override
public boolean isImmutable() {
return true;
}
}
}
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ java_library(
":runfiles_tree",
":thread_state_receiver",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
Expand Down Expand Up @@ -344,6 +345,7 @@ java_library(
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/net/starlark/java/annot",
"//src/main/java/net/starlark/java/eval",
"//third_party:caffeine",
"//third_party:guava",
"//third_party:jsr305",
"//third_party/protobuf:protobuf_java",
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/CommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.util.Fingerprint;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -98,6 +99,7 @@ public abstract Iterable<String> arguments(
public abstract void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
Fingerprint fingerprint)
throws CommandLineExpansionException, InterruptedException;

Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -177,13 +178,15 @@ public ImmutableList<String> allArguments(PathMapper pathMapper)
public void addToFingerprint(
ActionKeyContext actionKeyContext,
@Nullable ArtifactExpander artifactExpander,
CoreOptions.OutputPathsMode outputPathsMode,
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, fingerprint);
commandLine.addToFingerprint(
actionKeyContext, artifactExpander, outputPathsMode, fingerprint);
if (paramFileInfo != null) {
addParamFileInfoToFingerprint(paramFileInfo, fingerprint);
}
Expand Down
110 changes: 109 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/PathMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,17 @@

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.starlarkbuildapi.FileRootApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CheckReturnValue;
import javax.annotation.Nullable;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.StarlarkSemantics;

/**
Expand Down Expand Up @@ -57,6 +63,11 @@ static PathMapper loadFrom(StarlarkSemantics semantics) {
*/
@CheckReturnValue
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) {
return semantics;
}
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
Expand Down Expand Up @@ -116,6 +127,31 @@ default ExceptionlessMapFn<Object> getMapFn(@Nullable String previousFlag) {
return MapFn.DEFAULT;
}

/**
* Returns a {@link FileRootApi} representing the new root of the given artifact after mapping.
*
* <p>All objects returned by this method must be {@link Comparable} among each other.
*/
default FileRootApi mapRoot(Artifact artifact) {
ArtifactRoot root = artifact.getRoot();
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);
}
// 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.
// the digest of the artifact.
PathFragment execPath = artifact.getExecPath();
PathFragment mappedExecPath = map(execPath);
// map never changes the root-relative part of the exec path, so we can remove that suffix to
// get the mapped root part.
int rootRelativeSegmentCount = execPath.segmentCount() - root.getExecPath().segmentCount();
PathFragment mappedRootExecPath =
mappedExecPath.subFragment(0, mappedExecPath.segmentCount() - rootRelativeSegmentCount);
return new MappedArtifactRoot(mappedRootExecPath);
}

/**
* Returns {@code true} if the mapper is known to map all paths identically.
*
Expand All @@ -138,8 +174,80 @@ default Object cacheKey() {
}

/** A {@link PathMapper} that doesn't change paths. */
PathMapper NOOP = execPath -> execPath;
PathMapper NOOP =
new PathMapper() {
@Override
public PathFragment map(PathFragment execPath) {
return execPath;
}

@Override
public FileRootApi mapRoot(Artifact artifact) {
return artifact.getRoot();
}
};

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",
category = DocCategory.BUILTIN,
doc = "A root for files that have been subject to path mapping")
final class MappedArtifactRoot implements FileRootApi, Comparable<MappedArtifactRoot> {
private final PathFragment mappedRootExecPath;

public MappedArtifactRoot(PathFragment mappedRootExecPath) {
this.mappedRootExecPath = mappedRootExecPath;
}

@Override
public String getExecPathString() {
return mappedRootExecPath.getPathString();
}

@Override
public int compareTo(MappedArtifactRoot otherRoot) {
return mappedRootExecPath.compareTo(otherRoot.mappedRootExecPath);
}

@Override
public boolean equals(Object obj) {
// Per the contract of PathMapper#map, mapped roots never have exec paths that are equal to
// exec paths of non-mapped roots, that is, of instances of ArtifactRoot. Thus, it is correct
// for both equals implementations to return false if the other object is not an instance of
// the respective class.
if (!(obj instanceof MappedArtifactRoot other)) {
return false;
}
return mappedRootExecPath.equals(other.mappedRootExecPath);
}

@Override
public int hashCode() {
return mappedRootExecPath.hashCode();
}

@Override
public String toString() {
return mappedRootExecPath + " [mapped]";
}

@Override
public void repr(Printer printer) {
printer.append("<mapped root>");
}

@Override
public boolean isImmutable() {
return true;
}
}
}
33 changes: 30 additions & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,9 @@ java_library(
"WindowsPowershellCommandConstructor.java",
"actions/ActionConstructionContext.java",
"actions/FileWriteAction.java",
"actions/PathMappers.java",
"actions/ShellCommand.java",
"actions/SpawnAction.java",
"actions/StarlarkAction.java",
"actions/StrippingPathMapper.java",
"actions/SymlinkTreeAction.java",
"actions/SymlinkTreeActionContext.java",
"config/DependencyEvaluationException.java",
Expand Down Expand Up @@ -267,6 +265,7 @@ java_library(
":actions/deterministic_writer",
":actions/lazy_write_nested_set_of_tuple_action",
":actions/parameter_file_write_action",
":actions/path_mappers",
":actions/substitution",
":actions/symlink_action",
":actions/template_expansion_action",
Expand Down Expand Up @@ -367,7 +366,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
"//src/main/java/com/google/devtools/build/lib/actions:arg_chunk",
"//src/main/java/com/google/devtools/build/lib/actions:artifact_expander",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
Expand Down Expand Up @@ -1420,6 +1418,7 @@ java_library(
name = "actions/custom_command_line",
srcs = ["actions/CustomCommandLine.java"],
deps = [
":config/core_options",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifact_expander",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
Expand Down Expand Up @@ -1512,6 +1511,7 @@ java_library(
deps = [
":actions/abstract_file_write_action",
":actions/deterministic_writer",
":config/core_options",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifact_expander",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
Expand All @@ -1526,6 +1526,31 @@ java_library(
],
)

java_library(
name = "actions/path_mappers",
srcs = [
"actions/PathMappers.java",
"actions/StrippingPathMapper.java",
],
deps = [
":config/build_configuration",
":config/core_options",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
"//src/main/java/com/google/devtools/build/lib/actions:arg_chunk",
"//src/main/java/com/google/devtools/build/lib/actions:artifact_expander",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//third_party:guava",
"//third_party:jsr305",
],
)

java_library(
name = "actions/proto_deterministic_writer",
srcs = ["actions/ProtoDeterministicWriter.java"],
Expand Down Expand Up @@ -2526,6 +2551,8 @@ java_library(
name = "starlark/starlark_custom_command_line",
srcs = ["starlark/StarlarkCustomCommandLine.java"],
deps = [
":actions/path_mappers",
":config/core_options",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:arg_chunk",
"//src/main/java/com/google/devtools/build/lib/actions:artifact_expander",
Expand Down
Loading

0 comments on commit ca6e80e

Please sign in to comment.