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(