From 3bcf76c2c6d02308274fe993f61fb3e2422dc4a0 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 29 May 2023 13:49:58 +0200 Subject: [PATCH] Automatically map paths in `Args#map_each` When path mapping is enabled, `File` objects accessed in a user-supplied callback passed to `Args#map_each` automatically have their paths mapped. Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on `map_each` do not need to be modified to use path mapping. --- .../devtools/build/lib/actions/Artifact.java | 100 ++++++++++++++++-- .../google/devtools/build/lib/actions/BUILD | 3 + .../build/lib/actions/PathMapper.java | 92 ++++++++++++++-- .../starlark/StarlarkCustomCommandLine.java | 24 +++-- .../build/lib/starlarkbuildapi/FileApi.java | 10 +- .../build/lib/actions/ArtifactTest.java | 56 ++++++++++ .../actions/StrippingPathMapperTest.java | 44 ++++---- .../lib/exec/SpawnInputExpanderTest.java | 38 ++++--- .../build/lib/sandbox/SandboxHelpersTest.java | 14 ++- .../lib/starlark/StarlarkRuleContextTest.java | 45 ++++++++ 10 files changed, 364 insertions(+), 62 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index c5e79a90ee5962..2f779d5fc3cf6c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -28,6 +28,7 @@ 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.ArtifactRoot.RootType; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; @@ -42,6 +43,7 @@ import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.starlarkbuildapi.FileApi; +import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi; import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.HashCodes; @@ -62,9 +64,11 @@ import java.util.Set; 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; +import net.starlark.java.eval.StarlarkSemantics; /** * An Artifact represents a file used by the build system, whether it's a source file or a derived @@ -582,17 +586,23 @@ public SpecialArtifact getParent() { /** * Returns the directory name of this artifact, similar to dirname(1). * - *

The directory name is always a relative path to the execution directory. + *

The directory name is always a relative path to the execution directory. */ - @Override public final String getDirname() { + return getDirname(execPath); + } + + @Override + public final String getDirnameForStarlark(StarlarkSemantics semantics) { + return getDirname(PathMapper.loadFrom(semantics).map(execPath)); + } + + private static String getDirname(PathFragment execPath) { PathFragment parent = execPath.getParentDirectory(); return (parent == null) ? "/" : parent.getSafePathString(); } - /** - * Returns the base file name of this artifact, similar to basename(1). - */ + /** Returns the base file name of this artifact, similar to basename(1). */ @Override public final String getFilename() { return execPath.getBaseName(); @@ -649,16 +659,37 @@ public final Label getOwner() { * package-path entries (for source Artifacts), or one of the bin, genfiles or includes dirs (for * derived Artifacts). It will always be an ancestor of getPath(). */ - @Override public final ArtifactRoot getRoot() { return root; } + @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); + } + @Override public final PathFragment getExecPath() { return execPath; } + @Override + public String getExecPathStringForStarlark(StarlarkSemantics semantics) { + return PathMapper.loadFrom(semantics).getMappedExecPathString(this); + } + /** * Returns the relative path to this artifact relative to its root. It makes no guarantees as to * the semantic meaning or the completeness of the returned path value. In other words, no @@ -1641,4 +1672,61 @@ 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 { + 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)) { + return false; + } + MappedArtifactRoot other = (MappedArtifactRoot) obj; + 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(""); + } + + @Override + public boolean isImmutable() { + return true; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 2037280420f728..db269e9c9b0531 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -307,6 +307,7 @@ java_library( "ArtifactResolver.java", "ArtifactRoot.java", "Artifacts.java", + "PathMapper.java", ], deps = [ ":action_lookup_data", @@ -316,6 +317,7 @@ java_library( ":fileset_output_symlink", ":package_roots", ":path_strippable", + "//src/main/java/com/google/devtools/build/docgen/annot", "//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/concurrent", @@ -334,6 +336,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe:execution_phase_skykey", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", "//src/main/protobuf:failure_details_java_proto", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java index 0e7dcc546dd531..51b8fe63380eba 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java @@ -17,14 +17,20 @@ import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn; import com.google.devtools.build.lib.actions.CommandLineItem.MapFn; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.errorprone.annotations.CheckReturnValue; import java.util.List; +import java.util.Objects; import javax.annotation.Nullable; +import net.starlark.java.eval.StarlarkSemantics; /** * Support for mapping config parts of exec paths in an action's command line as well as when * staging its inputs and outputs for execution, with the aim of making the resulting {@link Spawn} * more cacheable. * + *

Implementations must be pure functions of the set of inputs (including paths and potentially + * content) of a given action. + * *

Actions that want to support path mapping should use {@link * com.google.devtools.build.lib.analysis.actions.PathMappers}. * @@ -32,23 +38,77 @@ * com.google.devtools.build.lib.analysis.actions.StrippingPathMapper}, which removes the config * part (e.g. "k8-fastbuild") from exec paths to allow for cross-configuration cache hits. */ -public interface PathMapper { +public abstract class PathMapper { /** * Returns the exec path with the path mapping applied. * + *

An exec path typically consists of a root part (such as "bazel-out/k8-opt/bin" + * or "") and a root-relative part (such as "path/to/pkg/file"). + * + *

Overrides must satisfy the following properties: + * + *

+ * *

Path mappers may return paths with different roots for two paths that have the same root * (e.g., they may map an artifact at {@code bazel-out/k8-fastbuild/bin/pkg/foo} to {@code * bazel-out//bin/pkg/foo}). Paths of artifacts that should share the same * parent directory, such as runfiles or tree artifact files, should thus be derived from the * mapped path of their parent. */ - PathFragment map(PathFragment execPath); + public abstract PathFragment map(PathFragment execPath); /** Returns the exec path of the input with the path mapping applied. */ - default String getMappedExecPathString(ActionInput artifact) { + public String getMappedExecPathString(ActionInput artifact) { return map(artifact.getExecPath()).getPathString(); } + /** A {@link PathMapper} that doesn't change paths. */ + public static final PathMapper NOOP = + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + return execPath; + } + }; + + private static final StarlarkSemantics.Key SEMANTICS_KEY = + new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP); + + /** + * Retrieve the {@link PathMapper} instance stored in the given {@link StarlarkSemantics} via + * {@link #storeIn(StarlarkSemantics)}. + */ + public static PathMapper loadFrom(StarlarkSemantics semantics) { + return semantics.get(SEMANTICS_KEY); + } + + /** + * Creates a new {@link StarlarkSemantics} instance which causes all Starlark threads using it to + * automatically apply this {@link PathMapper} to all struct fields of {@link + * com.google.devtools.build.lib.starlarkbuildapi.FileApi}. + * + *

This is meant to be used when evaluating user-defined callbacks to Starlark variants of + * custom command lines that are evaluated during the execution phase. + * + *

Since any unmapped path appearing in a command line will prevent cross-configuration cache + * hits, this mapping is applied automatically instead of requiring users to explicitly map all + * paths themselves. As an added benefit, this allows actions to opt into path mapping without + * actual changes to their command line code. + */ + @CheckReturnValue + public StarlarkSemantics storeIn(StarlarkSemantics semantics) { + // Since PathMapper#equals returns true for all instances of the same class, every non-noop + // instance is different from the default value for the key and thus persisted. Furthermore, + // since this causes the resulting semantics to compare equal if they PathMapper class + // agrees, there is only a single cache entry for it in Starlark's CallUtils cache for Starlark + // methods. + return semantics.toBuilder().set(SEMANTICS_KEY, this).build(); + } + /** * We don't yet have a Starlark API for mapping paths in command lines. Simple Starlark calls like * {@code args.add(arg_name, file_path} are automatically handled. But calls that involve custom @@ -57,7 +117,7 @@ default String getMappedExecPathString(ActionInput artifact) { *

This method allows implementations to hard-code support for specific command line entries * for specific Starlark actions. */ - default List mapCustomStarlarkArgs(List args) { + public List mapCustomStarlarkArgs(List args) { return args; } @@ -74,7 +134,7 @@ default List mapCustomStarlarkArgs(List args) { * *

By default, this method returns {@link MapFn#DEFAULT}. */ - default ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { + public ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { return MapFn.DEFAULT; } @@ -84,7 +144,7 @@ default ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { *

Can be used by actions to skip additional work that isn't needed if path mapping is not * enabled. */ - default boolean isNoop() { + public boolean isNoop() { return this == NOOP; } @@ -95,10 +155,24 @@ default boolean isNoop() { * *

The default implementation returns the {@link Class} of the path mapper. */ - default Object cacheKey() { + public Object cacheKey() { return this.getClass(); } - /** A {@link PathMapper} that doesn't change paths. */ - PathMapper NOOP = execPath -> execPath; + @Override + public final boolean equals(Object obj) { + if (obj == null || obj.getClass() != getClass()) { + return false; + } + // We store PathMapper instances in StarlarkSemantics to allow mapping of File struct fields + // such as "path" in map_each callbacks of Args. Since this code is only executed during the + // execution phase, we do not want two different non-noop PathMapper instances to result in + // distinct StarlarkSemantics instances for caching purposes. + return isNoop() == ((PathMapper) obj).isNoop(); + } + + @Override + public final int hashCode() { + return Objects.hashCode(isNoop()); + } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java index 075c075ecfda51..75b978f72ee8ae 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java @@ -214,6 +214,7 @@ private int eval( stringValues::add, location, artifactExpander, + pathMapper, starlarkSemantics); } else { int count = expandedValues.size(); @@ -394,7 +395,8 @@ private int addToFingerprint( mapEach, location, starlarkSemantics, - (features & EXPAND_DIRECTORIES) != 0 ? artifactExpander : null); + (features & EXPAND_DIRECTORIES) != 0 ? artifactExpander : null, + PathMapper.NOOP); try { actionKeyContext.addNestedSetToFingerprint(commandLineItemMapFn, fingerprint, values); } finally { @@ -433,6 +435,7 @@ private int addToFingerprint( fingerprint::addString, location, artifactExpander, + PathMapper.NOOP, starlarkSemantics); } else { for (Object value : maybeExpandedValues) { @@ -900,10 +903,11 @@ private static void applyMapEach( Consumer consumer, Location loc, @Nullable ArtifactExpander artifactExpander, + PathMapper pathMapper, StarlarkSemantics starlarkSemantics) throws CommandLineExpansionException, InterruptedException { try (Mutability mu = Mutability.create("map_each")) { - StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics); + StarlarkThread thread = new StarlarkThread(mu, pathMapper.storeIn(starlarkSemantics)); // TODO(b/77140311): Error if we issue print statements. thread.setPrintHandler((th, msg) -> {}); int count = originalValues.size(); @@ -964,6 +968,7 @@ private static class CommandLineItemMapEachAdaptor * cleared. */ private final boolean hasArtifactExpander; + private final PathMapper pathMapper; @Nullable private ArtifactExpander artifactExpander; @@ -971,12 +976,14 @@ private static class CommandLineItemMapEachAdaptor StarlarkCallable mapFn, Location location, StarlarkSemantics starlarkSemantics, - @Nullable ArtifactExpander artifactExpander) { + @Nullable ArtifactExpander artifactExpander, + PathMapper pathMapper) { this.mapFn = mapFn; this.location = location; this.starlarkSemantics = starlarkSemantics; this.hasArtifactExpander = artifactExpander != null; this.artifactExpander = artifactExpander; + this.pathMapper = pathMapper; } @Override @@ -984,7 +991,8 @@ public void expandToCommandLine(Object object, Consumer args) throws CommandLineExpansionException, InterruptedException { Preconditions.checkState(artifactExpander != null || !hasArtifactExpander); applyMapEach( - mapFn, maybeExpandDirectory(object), args, location, artifactExpander, starlarkSemantics); + mapFn, maybeExpandDirectory(object), args, location, artifactExpander, + pathMapper, starlarkSemantics); } private List maybeExpandDirectory(Object object) throws CommandLineExpansionException { @@ -1078,7 +1086,7 @@ static class FilesetSymlinkFile implements FileApi, CommandLineItem { } @Override - public String getDirname() { + public String getDirnameForStarlark(StarlarkSemantics semantics) { PathFragment parent = execPath.getParentDirectory(); return (parent == null) ? "/" : parent.getSafePathString(); } @@ -1099,7 +1107,7 @@ public Label getOwnerLabel() { } @Override - public FileRootApi getRoot() { + public FileRootApi getRootForStarlark(StarlarkSemantics semantics) { return fileset.getRoot(); } @@ -1122,7 +1130,7 @@ public String getRunfilesPathString() { } @Override - public String getExecPathString() { + public String getExecPathStringForStarlark(StarlarkSemantics semantics) { return execPath.getPathString(); } @@ -1134,7 +1142,7 @@ public String getTreeRelativePathString() throws EvalException { @Override public String expandToCommandLine() { - return getExecPathString(); + return execPath.getPathString(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/FileApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/FileApi.java index a3b444c93c900f..90b570050563d9 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/FileApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/FileApi.java @@ -20,6 +20,7 @@ import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.StarlarkSemantics; import net.starlark.java.eval.StarlarkValue; /** The interface for files in Starlark. */ @@ -45,10 +46,11 @@ public interface FileApi extends StarlarkValue { @StarlarkMethod( name = "dirname", structField = true, + useStarlarkSemantics = true, doc = "The name of the directory containing this file. It's taken from " + "path and is always relative to the execution directory.") - String getDirname(); + String getDirnameForStarlark(StarlarkSemantics semantics); @StarlarkMethod( name = "basename", @@ -75,8 +77,9 @@ public interface FileApi extends StarlarkValue { @StarlarkMethod( name = "root", structField = true, + useStarlarkSemantics = true, doc = "The root beneath which this file resides.") - FileRootApi getRoot(); + FileRootApi getRootForStarlark(StarlarkSemantics semantics); @StarlarkMethod( name = "is_source", @@ -102,6 +105,7 @@ public interface FileApi extends StarlarkValue { @StarlarkMethod( name = "path", structField = true, + useStarlarkSemantics = true, doc = "The execution path of this file, relative to the workspace's execution directory. It" + " consists of two parts, an optional first part called the root (see also" @@ -112,7 +116,7 @@ public interface FileApi extends StarlarkValue { + " architecture that was used while building said file. Use the" + " short_path for the path under which the file is mapped if it's in" + " the runfiles of a binary.") - String getExecPathString(); + String getExecPathStringForStarlark(StarlarkSemantics semantics); @StarlarkMethod( name = "tree_relative_path", diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java index e2d6701d11dc9f..c65064ede3afd2 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java @@ -55,6 +55,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; +import net.starlark.java.eval.StarlarkSemantics; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -600,6 +601,61 @@ public void archivedTreeArtifact_getExecPathWithinArchivedArtifactsTree_returnsC PathFragment.create("bazel-out/:archived_tree_artifacts/k8-fastbuild/bin/dir/subdir")); } + private static final PathMapper PATH_MAPPER = + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + if (execPath.startsWith(PathFragment.create("output"))) { + // output/k8-opt/bin/path/to/pkg/file --> output//path/to/pkg/file + return execPath + .subFragment(0, 1) + .getRelative(Integer.toUnsignedString(execPath.subFragment(3).hashCode())) + .getRelative(execPath.subFragment(3)); + } else { + return execPath; + } + } + }; + + @Test + public void mappedArtifact_sourceArtifact() throws IOException { + StarlarkSemantics semantics = PATH_MAPPER.storeIn(StarlarkSemantics.DEFAULT); + + Root sourceRoot = Root.fromPath(scratch.getFileSystem().getPath("/some/path")); + ArtifactRoot root = ArtifactRoot.asSourceRoot(sourceRoot); + Artifact artifact = + ActionsTestUtil.createArtifact(root, scratch.file("/some/path/path/to/pkg/file")); + + assertThat(artifact.getExecPathStringForStarlark(semantics)).isEqualTo("path/to/pkg/file"); + assertThat(artifact.getDirnameForStarlark(semantics)).isEqualTo("path/to/pkg"); + assertThat(artifact.getRootForStarlark(semantics)).isSameInstanceAs(root); + // Verify both equals and compareTo implementations give the same result. + assertThat(artifact.getRootForStarlark(semantics)).isEqualTo(root); + assertThat(root).isEqualTo(artifact.getRootForStarlark(semantics)); + } + + @Test + public void mappedArtifact_derivedArtifact() throws IOException { + StarlarkSemantics semantics = PATH_MAPPER.storeIn(StarlarkSemantics.DEFAULT); + + Path execRoot = scratch.getFileSystem().getPath("/some/path"); + ArtifactRoot root = + ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "output", "k8-opt", "bin"); + Artifact artifact = + ActionsTestUtil.createArtifact( + root, scratch.file("/some/path/output/k8-opt/bin/path/to/pkg/file")); + + assertThat(artifact.getExecPathStringForStarlark(semantics)) + .isEqualTo("output/1084027401/path/to/pkg/file"); + assertThat(artifact.getDirnameForStarlark(semantics)) + .isEqualTo("output/1084027401/path/to/pkg"); + assertThat(artifact.getRootForStarlark(semantics).getExecPathString()) + .isEqualTo("output/1084027401"); + // Verify both equals implementations give the same result. + assertThat(artifact.getRootForStarlark(semantics)).isNotEqualTo(root); + assertThat(root).isNotEqualTo(artifact.getRootForStarlark(semantics)); + } + private static SpecialArtifact createTreeArtifact(ArtifactRoot root, String relativePath) { return createTreeArtifact(root, relativePath, ActionsTestUtil.NULL_ACTION_LOOKUP_DATA); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java index 4bb42e57a6a1d2..0cce3471f5aa1e 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java @@ -16,6 +16,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; +import static java.lang.String.format; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Spawn; @@ -85,22 +86,22 @@ public void javaLibraryWithJavacopts() throws Exception { .collect(toImmutableList())) .containsExactly( "java/com/google/test/A.java", - outDir + "/cfg/bin/java/com/google/test/B.java", - outDir + "/cfg/bin/java/com/google/test/C.java", - outDir + "/cfg/bin/java/com/google/test/liba-hjar.jar", - outDir + "/cfg/bin/java/com/google/test/liba-hjar.jdeps", - "-XepOpt:foo:bar=" + outDir + "/cfg/bin/java/com/google/test/B.java", - "-XepOpt:baz=" - + outDir - + "/cfg/bin/java/com/google/test/C.java," - + outDir - + "/cfg/bin/java/com/google/test/B.java"); + format("%s/cfg/bin/java/com/google/test/B.java", outDir), + format("%s/cfg/bin/java/com/google/test/C.java", outDir), + format("%s/cfg/bin/java/com/google/test/liba-hjar.jar", outDir), + format("%s/cfg/bin/java/com/google/test/liba-hjar.jdeps", outDir), + format("-XepOpt:foo:bar=%s/cfg/bin/java/com/google/test/B.java", outDir), + format( + "-XepOpt:baz=%s/cfg/bin/java/com/google/test/C.java,%s/cfg/bin/java/com/google/test/B.java", + outDir, outDir)); } private void addStarlarkRule(Dict executionRequirements) throws IOException { scratch.file("defs/BUILD"); scratch.file( "defs/defs.bzl", + "def _map_each(file):", + " return '{}:{}:{}:{}'.format(file.short_path, file.path, file.root.path, file.dirname)", "def _my_rule_impl(ctx):", " args = ctx.actions.args()", " args.add(ctx.outputs.out)", @@ -108,6 +109,7 @@ private void addStarlarkRule(Dict executionRequirements) throws " depset(ctx.files.srcs),", " before_each = '-source',", " format_each = '<%s>',", + " map_each = _map_each,", " )", " ctx.actions.run(", " outputs = [ctx.outputs.out],", @@ -115,7 +117,7 @@ private void addStarlarkRule(Dict executionRequirements) throws " executable = ctx.executable._tool,", " arguments = [args],", " mnemonic = 'MyRuleAction',", - String.format(" execution_requirements = %s,", Starlark.repr(executionRequirements)), + format(" execution_requirements = %s,", Starlark.repr(executionRequirements)), " )", " return [DefaultInfo(files = depset([ctx.outputs.out]))]", "my_rule = rule(", @@ -170,12 +172,14 @@ public void starlarkRule_optedInViaExecutionRequirements() throws Exception { String outDir = analysisMock.getProductName() + "-out"; assertThat(spawn.getArguments().stream().collect(toImmutableList())) .containsExactly( - outDir + "/cfg/bin/tool/tool", - outDir + "/cfg/bin/pkg/out.bin", + format("%s/cfg/bin/tool/tool", outDir), + format("%s/cfg/bin/pkg/out.bin", outDir), "-source", - "<" + outDir + "/cfg/bin/pkg/gen_src.txt>", + format( + "", + outDir), "-source", - "") + "") .inOrder(); } @@ -196,12 +200,14 @@ public void starlarkRule_optedInViaModifyExecutionInfo() throws Exception { String outDir = analysisMock.getProductName() + "-out"; assertThat(spawn.getArguments().stream().collect(toImmutableList())) .containsExactly( - outDir + "/cfg/bin/tool/tool", - outDir + "/cfg/bin/pkg/out.bin", + format("%s/cfg/bin/tool/tool", outDir), + format("%s/cfg/bin/pkg/out.bin", outDir), "-source", - "<" + outDir + "/cfg/bin/pkg/gen_src.txt>", + format( + "", + outDir), "-source", - "") + "") .inOrder(); } } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java index e55beea7fd9eda..97bbc8a40d0c71 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java @@ -276,7 +276,12 @@ public void testRunfilesTwoFiles_pathMapped() throws Exception { supplier, mockCache, NO_ARTIFACT_EXPANDER, - execPath -> PathFragment.create(execPath.getPathString().replace("k8-opt/", "")), + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + return PathFragment.create(execPath.getPathString().replace("k8-opt/", "")); + } + }, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(2); assertThat(inputMappings) @@ -409,20 +414,23 @@ public void testRunfilesWithTreeArtifacts_pathMapped() throws Exception { fakeCache.put(file2, FileArtifactValue.createForTesting(file2)); PathMapper pathMapper = - execPath -> { - // Replace the config segment "k8-opt" in "bazel-bin/k8-opt/bin" with a hash of the full - // path to verify that the new paths are constructed by appending the child paths to the - // mapped parent path, not by mapping the child paths directly. - PathFragment runfilesPath = execPath.subFragment(3); - String runfilesPathHash = - DigestHashFunction.SHA256 - .getHashFunction() - .hashString(runfilesPath.getPathString(), UTF_8) - .toString(); - return execPath - .subFragment(0, 1) - .getRelative(runfilesPathHash.substring(0, 8)) - .getRelative(execPath.subFragment(2)); + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + // Replace the config segment "k8-opt" in "bazel-bin/k8-opt/bin" with a hash of the full + // path to verify that the new paths are constructed by appending the child paths to the + // mapped parent path, not by mapping the child paths directly. + PathFragment runfilesPath = execPath.subFragment(3); + String runfilesPathHash = + DigestHashFunction.SHA256 + .getHashFunction() + .hashString(runfilesPath.getPathString(), UTF_8) + .toString(); + return execPath + .subFragment(0, 1) + .getRelative(runfilesPathHash.substring(0, 8)) + .getRelative(execPath.subFragment(2)); + } }; expander.addRunfilesToInputs( diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java index 78fe5c244ee282..a84b78d078f7e1 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java @@ -425,7 +425,12 @@ public void populateInputsAndDirsToCreate_createsMappedDirectories() { ActionsTestUtil.createTreeArtifactWithGeneratingAction( outputRoot, "bin/config/other_dir/subdir"); PathMapper pathMapper = - execPath -> PathFragment.create(execPath.getPathString().replace("config/", "")); + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + return PathFragment.create(execPath.getPathString().replace("config/", "")); + } + }; Spawn spawn = new SpawnBuilder().withOutputs(outputFile, outputDir).setPathMapper(pathMapper).build(); var sandboxHelpers = new SandboxHelpers(); @@ -452,7 +457,12 @@ public void populateInputsAndDirsToCreate_createsMappedDirectories() { public void moveOutputs_mappedPathMovedToUnmappedPath() throws Exception { PathFragment unmappedOutputPath = PathFragment.create("bin/config/output"); PathMapper pathMapper = - execPath -> PathFragment.create(execPath.getPathString().replace("config/", "")); + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + return PathFragment.create(execPath.getPathString().replace("config/", "")); + } + }; Spawn spawn = new SpawnBuilder() .withOutputs(unmappedOutputPath.getPathString()) diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java index ee1ed9385cbf1f..1a625652e3a2bd 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java @@ -29,7 +29,9 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.analysis.ActionsProvider; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; @@ -40,6 +42,7 @@ import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.actions.StarlarkAction; import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget; +import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.analysis.starlark.StarlarkExecGroupCollection; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; @@ -54,6 +57,7 @@ import com.google.devtools.build.lib.rules.java.JavaInfo; import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider; import com.google.devtools.build.lib.starlark.util.BazelEvaluationTestCase; +import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.OS; @@ -2725,6 +2729,47 @@ public void testArgsMapEachFunctionAllowClosure() throws Exception { assertThat(ev.eval("action.content")).isEqualTo("lambda:a\nlambda:b\nlocal:c;local:d\n"); } + @Test + public void testArgsMapEachWithPathMapper() throws Exception { + scratch.file( + "test/rules.bzl", + getSimpleUnderTestDefinition("ctx.actions.write(out, '')"), + testingRuleDefinition); + scratch.file("test/BUILD", simpleBuildDefinition); + StarlarkRuleContext ruleContext = createRuleContext("//test:testing"); + setRuleContext(ruleContext); + + ev.update("file1", ev.eval("ruleContext.actions.declare_file('file1')")); + ev.update("file2", ev.eval("ruleContext.actions.declare_file('dir/file2')")); + Object result = + ev.eval( + "ruleContext.actions.args().add_all(" + + " [file1, file2]," + + " allow_closure=True," + + " map_each=lambda f: 'file:%s:%s:%s:%s:%s' % (" + // Verify that mapped roots are comparable. + + " f.path, f.dirname, f.root.path, type(f.root), f.root <= f.root)" + + ")"); + + PathMapper stripConfig = + new PathMapper() { + @Override + public PathFragment map(PathFragment execPath) { + return execPath.subFragment(0, 1).getRelative(execPath.subFragment(2)); + } + }; + + assertThat(result).isInstanceOf(Args.class); + CommandLine args = ((Args) result).build(); + String out = TestConstants.PRODUCT_NAME + "-out"; + assertThat(args.arguments(null, stripConfig)) + .containsExactly( + String.format("file:%1$s/bin/test/file1:%1$s/bin/test:%1$s/bin:mapped_root:True", out), + String.format( + "file:%1$s/bin/test/dir/file2:%1$s/bin/test/dir:%1$s/bin:mapped_root:True", out)) + .inOrder(); + } + @Test public void testTemplateExpansionActionInterface() throws Exception { scratch.file(