From d07e5dcdf51cb551b9ba0379f63e6c0e16dc34f0 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 29 Jul 2024 22:26:49 -0700 Subject: [PATCH] [7.4.0] Fix issues with path mapping in `map_each` functions 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 --- .../lib/actions/AbstractCommandLine.java | 0 .../devtools/build/lib/actions/Artifact.java | 73 +-- .../google/devtools/build/lib/actions/BUILD | 3 + .../build/lib/actions/CommandLine.java | 2 + .../build/lib/actions/CommandLines.java | 5 +- .../build/lib/actions/PathMapper.java | 110 ++++- .../google/devtools/build/lib/analysis/BUILD | 32 +- .../analysis/actions/CustomCommandLine.java | 2 + .../actions/ParameterFileWriteAction.java | 7 +- .../lib/analysis/actions/PathMappers.java | 84 +++- .../lib/analysis/actions/SpawnAction.java | 17 +- .../analysis/actions/StrippingPathMapper.java | 226 +++++----- .../starlark/StarlarkCustomCommandLine.java | 51 ++- .../google/devtools/build/lib/rules/cpp/BUILD | 1 + .../build/lib/rules/cpp/CppCompileAction.java | 12 +- .../devtools/build/lib/rules/java/BUILD | 1 + .../lib/rules/java/JavaCompileAction.java | 21 +- .../build/lib/actions/ArtifactTest.java | 116 +++-- .../google/devtools/build/lib/actions/BUILD | 2 + .../lib/actions/CustomCommandLineTest.java | 7 +- .../google/devtools/build/lib/analysis/BUILD | 1 + ...thMapperTest.java => PathMappersTest.java} | 16 +- .../StarlarkCustomCommandLineTest.java | 14 +- .../google/devtools/build/lib/starlark/BUILD | 1 + ...arlarkRuleImplementationFunctionsTest.java | 224 ++++++++-- src/test/shell/bazel/path_mapping_test.sh | 417 ++++++++++++++++++ 26 files changed, 1141 insertions(+), 304 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java rename src/test/java/com/google/devtools/build/lib/analysis/actions/{StrippingPathMapperTest.java => PathMappersTest.java} (93%) diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java new file mode 100644 index 00000000000000..e69de29bb2d1d6 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 12f87cc1da543b..722ba8f135c404 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,7 +28,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.ArtifactRoot.RootType; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; @@ -65,7 +64,6 @@ 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; @@ -666,19 +664,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 @@ -1673,61 +1659,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 { - 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 1c6250ace7be5a..4419d2f3effb81 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -156,6 +156,7 @@ java_library( ":runfiles_supplier", ":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", @@ -315,6 +316,7 @@ java_library( ":fileset_output_symlink", ":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", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", @@ -338,6 +340,7 @@ java_library( "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", "//src/main/protobuf:failure_details_java_proto", + "//third_party:caffeine", "//third_party:guava", "//third_party:jsr305", "//third_party/protobuf:protobuf_java", diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java index 5921fab7e38a73..026d8c2d8851e6 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java @@ -17,6 +17,7 @@ import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.collect.IterablesChain; import com.google.devtools.build.lib.util.Fingerprint; @@ -61,6 +62,7 @@ public Iterable arguments(ArtifactExpander artifactExpander, PathMapper public void addToFingerprint( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, + CoreOptions.OutputPathsMode outputPathsMode, Fingerprint fingerprint) throws CommandLineExpansionException, InterruptedException { for (String s : arguments()) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java index d2be926068effe..7964f01d96497f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; 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.collect.IterablesChain; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Path; @@ -182,13 +183,15 @@ public ImmutableList allArguments(PathMapper pathMapper) public void addToFingerprint( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, + CoreOptions.OutputPathsMode outputPathsMode, Fingerprint fingerprint) throws CommandLineExpansionException, InterruptedException { ImmutableList 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); } 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 3a662ccdb56852..23ee0fbbfa992c 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 @@ -14,12 +14,18 @@ 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 java.util.List; import javax.annotation.Nullable; +import net.starlark.java.annot.StarlarkBuiltin; +import net.starlark.java.eval.Printer; import net.starlark.java.eval.StarlarkSemantics; /** @@ -58,6 +64,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 @@ -117,6 +128,31 @@ default ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { return MapFn.DEFAULT; } + /** + * Returns a {@link FileRootApi} representing the new root of the given artifact after mapping. + * + *

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. * @@ -139,8 +175,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 SEMANTICS_KEY = new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP); + + // Not meant for use outside this interface. + LoadingCache 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 { + 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(""); + } + + @Override + public boolean isImmutable() { + return true; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 767469bab938a3..46c46cd38fe202 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -213,11 +213,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", "buildinfo/BuildInfoFactory.java", @@ -272,6 +270,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", @@ -438,7 +437,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", - "//src/main/java/com/google/devtools/build/skyframe:skyframe", + "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/com/google/devtools/common/options", "//src/main/java/net/starlark/java/annot", @@ -1382,6 +1381,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:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", @@ -1470,6 +1470,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:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", @@ -1483,6 +1484,29 @@ 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: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"], @@ -2497,6 +2521,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:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index df9bc4390da5f5..364a48c2567419 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; +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; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -1364,6 +1365,7 @@ Object substituteTreeFileArtifactArgvFragment(TreeFileArtifactArgvFragment argvF public void addToFingerprint( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, + CoreOptions.OutputPathsMode outputPathsMode, Fingerprint fingerprint) throws CommandLineExpansionException, InterruptedException { List arguments = rawArgsAsList(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java index 0df56ee4a3b5a1..d946fa14acd4c4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.UserExecException; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -175,7 +176,8 @@ protected void computeKey( throws CommandLineExpansionException, InterruptedException { fp.addString(GUID); fp.addString(type.toString()); - commandLine.addToFingerprint(actionKeyContext, artifactExpander, fp); + commandLine.addToFingerprint( + actionKeyContext, artifactExpander, CoreOptions.OutputPathsMode.OFF, fp); } @Override @@ -191,7 +193,8 @@ public String describeKey() { // incomprehensible. Instead, just give a digest, which makes it easy to // tell if two contents are equal or not. var fp = new Fingerprint(); - commandLine.addToFingerprint(new ActionKeyContext(), null, fp); + commandLine.addToFingerprint( + new ActionKeyContext(), null, CoreOptions.OutputPathsMode.OFF, fp); message.append(BaseEncoding.base16().lowerCase().encode(fp.digestAndReset())); message.append( "\n" diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java index aa093422b0b04d..2e06fb68edeac4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java @@ -18,7 +18,8 @@ import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionKeyContext; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineLimits; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.PathMapper; @@ -26,6 +27,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Map; @@ -36,6 +38,50 @@ * 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. + * + *

When computing an action key, the following approaches to taking path mapping into account + * do not work: + * + *

    + *
  • 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. + *
  • 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. + *
  • 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. + *
+ * + *

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. private static final ImmutableSet SUPPORTED_MNEMONICS = @@ -61,19 +107,20 @@ public final class PathMappers { * Actions that support path mapping should call this method from {@link * Action#getKey(ActionKeyContext, ArtifactExpander)}. * - *

Compared to {@link #create(AbstractAction, OutputPathsMode)}, this method does not flatten - * nested sets and thus can't result in memory regressions. + *

Compared to {@link #create}, this method does not flatten nested sets and thus can't result + * in memory regressions. * - * @param mnemonic the mnemonic of the action - * @param executionInfo the execution info of the action * @param outputPathsMode the value of {@link CoreOptions#outputPathsMode} * @param fingerprint the fingerprint to add to */ public static void addToFingerprint( String mnemonic, Map executionInfo, + NestedSet additionalArtifactsForPathMapping, + ActionKeyContext actionKeyContext, OutputPathsMode outputPathsMode, - Fingerprint fingerprint) { + Fingerprint fingerprint) + throws CommandLineExpansionException, InterruptedException { // Creating a new PathMapper instance can be expensive, but isn't needed here: Whether and // how path mapping applies to the action only depends on the output paths mode and the action // inputs, which are already part of the action key. @@ -81,9 +128,19 @@ public static void addToFingerprint( getEffectiveOutputPathsMode(outputPathsMode, mnemonic, executionInfo); if (effectiveOutputPathsMode == OutputPathsMode.STRIP) { fingerprint.addString(StrippingPathMapper.GUID); + // These artifacts are not part of the actual command line or inputs, but influence the + // behavior of path mapping. + actionKeyContext.addNestedSetToFingerprint(fingerprint, additionalArtifactsForPathMapping); } } + /** 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}. * @@ -97,27 +154,28 @@ public static void addToFingerprint( *

Note: This method flattens nested sets and should thus not be called from methods that are * executed in the analysis phase. * - *

Actions calling this method should also call {@link #addToFingerprint(String, Map, - * OutputPathsMode, Fingerprint)} from {@link Action#getKey(ActionKeyContext, ArtifactExpander)} - * to ensure correct incremental builds. + *

Actions calling this method should also call {@link #addToFingerprint} from {@link + * Action#getKey(ActionKeyContext, ArtifactExpander)} to ensure correct incremental builds. * * @param action the {@link AbstractAction} for which a {@link Spawn} is to be created * @param outputPathsMode the value of {@link CoreOptions#outputPathsMode} + * @param isStarlarkAction whether the action is a Starlark action * @return a {@link PathMapper} that maps paths of the action's inputs and outputs. May be {@link * PathMapper#NOOP} if path mapping is not applicable to the action. */ - public static PathMapper create(AbstractAction action, OutputPathsMode outputPathsMode) { + public static PathMapper create( + AbstractAction action, OutputPathsMode outputPathsMode, boolean isStarlarkAction) { if (getEffectiveOutputPathsMode( outputPathsMode, action.getMnemonic(), action.getExecutionInfo()) != OutputPathsMode.STRIP) { return PathMapper.NOOP; } - return StrippingPathMapper.tryCreate(action).orElse(PathMapper.NOOP); + return StrippingPathMapper.tryCreate(action, isStarlarkAction).orElse(PathMapper.NOOP); } /** - * Helper method to simplify calling {@link #create(SpawnAction, OutputPathsMode)} for actions - * that store the configuration directly. + * Helper method to simplify calling {@link #create} for actions that store the configuration + * directly. * * @param configuration the configuration * @return the value of diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index c0afd349ff13f1..4d10bff0dc65fd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -69,6 +69,7 @@ import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.exec.SpawnStrategyResolver; import com.google.devtools.build.lib.server.FailureDetails; @@ -185,7 +186,8 @@ public CommandLines getCommandLines() { @Override public List getArguments() throws CommandLineExpansionException, InterruptedException { - return commandLines.allArguments(PathMappers.create(this, outputPathsMode)); + return commandLines.allArguments( + PathMappers.create(this, outputPathsMode, this instanceof StarlarkAction)); } @Override @@ -340,7 +342,8 @@ protected Spawn getSpawn( Map> filesetMappings, boolean reportOutputs) throws CommandLineExpansionException, InterruptedException { - PathMapper pathMapper = PathMappers.create(this, outputPathsMode); + PathMapper pathMapper = + PathMappers.create(this, outputPathsMode, this instanceof StarlarkAction); ExpandedCommandLines expandedCommandLines = commandLines.expand( artifactExpander, getPrimaryOutput().getExecPath(), pathMapper, getCommandLineLimits()); @@ -372,7 +375,7 @@ protected void computeKey( Fingerprint fp) throws CommandLineExpansionException, InterruptedException { fp.addString(GUID); - commandLines.addToFingerprint(actionKeyContext, artifactExpander, fp); + commandLines.addToFingerprint(actionKeyContext, artifactExpander, outputPathsMode, fp); fp.addString(mnemonic); // We don't need the toolManifests here, because they are a subset of the inputManifests by // definition and the output of an action shouldn't change whether something is considered a @@ -380,7 +383,13 @@ protected void computeKey( fp.addPaths(runfilesSupplier.getRunfilesDirs()); env.addTo(fp); fp.addStringMap(getExecutionInfo()); - PathMappers.addToFingerprint(getMnemonic(), getExecutionInfo(), outputPathsMode, fp); + PathMappers.addToFingerprint( + getMnemonic(), + getExecutionInfo(), + NestedSetBuilder.emptySet(Order.STABLE_ORDER), + actionKeyContext, + outputPathsMode, + fp); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java index 563bd8c8ce6fab..558a401634f440 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java @@ -17,16 +17,17 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.AbstractAction; -import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; +import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.CommandLineItem; import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn; import com.google.devtools.build.lib.actions.CommandLineItem.MapFn; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.Spawn; -import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode; +import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.HashMap; import java.util.List; @@ -63,12 +64,11 @@ * *

    *
  1. "Qualifying" actions strip config paths from their command lines. An action qualifies if - * its implementation logic uses {@link PathMappers#create(Action, OutputPathsMode)} as - * described in its javadocs and has its mnemonic listed in {@link - * PathMappers#SUPPORTED_MNEMONICS}. Such an action must pass the {@link PathMapper} to all - * structured command line constructions. If any unstructured command line arguments refer to - * artifact paths, custom handling needs to be added to {@code mapCustomStarlarkArgv} or - * {@code getMapFn} below. + * its implementation logic uses {@link PathMappers#create} as described in its javadocs and + * has its mnemonic listed in {@link PathMappers#SUPPORTED_MNEMONICS}. Such an action must + * pass the {@link PathMapper} to all structured command line constructions. If any + * unstructured command line arguments refer to artifact paths, custom handling needs to be + * added to {@code mapCustomStarlarkArgv} or {@code getMapFn} below. *
  2. A supporting executor strips paths from qualifying actions' inputs and outputs before * staging for execution by taking {@link Spawn#getPathMapper()} into account. *
@@ -76,20 +76,56 @@ *

So an action is responsible for declaring that it strips paths and adjusting its command line * accordingly. The executor is responsible for remapping action inputs and outputs to match. */ -public final class StrippingPathMapper { - static final String GUID = "8eb2ad5a-85d4-435b-858f-5c192e91997d"; +public final class StrippingPathMapper implements PathMapper { + public static final String GUID = "8eb2ad5a-85d4-435b-858f-5c192e91997d"; private static final String FIXED_CONFIG_SEGMENT = "cfg"; + private final PathFragment outputRoot; + private final String mnemonic; + private final boolean isStarlarkAction; + private final boolean isJavaAction; + private final ExceptionlessMapFn structuredArgStripper; + private final StringStripper argStripper; + private final ArtifactRoot outputArtifactRoot; + private final MappedArtifactRoot strippedOutputArtifactRoot; + + private StrippingPathMapper(Artifact primaryOutput, String mnemonic, boolean isStarlarkAction) { + // This is expected to always be "(bazel|blaze)-out". + this.outputRoot = primaryOutput.getExecPath().subFragment(0, 1); + this.mnemonic = mnemonic; + this.isStarlarkAction = isStarlarkAction; + this.argStripper = new StringStripper(outputRoot.getPathString()); + this.structuredArgStripper = + (object, args) -> { + if (object instanceof String str) { + args.accept(this.argStripper.strip(str)); + } else { + args.accept(CommandLineItem.expandToCommandLine(object)); + } + }; + // This kind of special handling should not be extended. It is a hack that works around a + // limitation of the native implementation of location expansion: The output is just a list of + // strings, not a structured command line that would allow transparent path mapping. + // Instead, reimplement location expansion in Starlark and have it return an Args object. + this.isJavaAction = + mnemonic.equals("Javac") + || mnemonic.equals("JavacTurbine") + || mnemonic.equals("Turbine") + || mnemonic.equals("JavaResourceJar"); + this.outputArtifactRoot = primaryOutput.getRoot(); + this.strippedOutputArtifactRoot = new MappedArtifactRoot(map(outputArtifactRoot.getExecPath())); + } + /** * Creates a new {@link PathMapper} that strips config prefixes if the particular action instance * supports it. * * @param action the action to potentially strip paths from + * @param isStarlarkAction whether the action is a Starlark action * @return a {@link StrippingPathMapper} if the action supports it, else {@link Optional#empty()}. */ - static Optional tryCreate(AbstractAction action) { - // This is expected to always be "bazel-out", but we don't want to hardcode it here. + static Optional tryCreate(AbstractAction action, boolean isStarlarkAction) { PathFragment outputRoot = action.getPrimaryOutput().getExecPath().subFragment(0, 1); // Additional artifacts to map are not part of the action's inputs, but may still lead to // path collisions after stripping. It is thus important to include them in this check. @@ -98,93 +134,85 @@ static Optional tryCreate(AbstractAction action) { action.getInputs().toList(), action.getAdditionalArtifactsForPathMapping().toList()), outputRoot)) { return Optional.of( - create(action.getMnemonic(), action instanceof StarlarkAction, outputRoot)); + new StrippingPathMapper( + action.getPrimaryOutput(), action.getMnemonic(), isStarlarkAction)); } return Optional.empty(); } - private static PathMapper create( - String mnemonic, boolean isStarlarkAction, PathFragment outputRoot) { - final StringStripper argStripper = new StringStripper(outputRoot.getPathString()); - final ExceptionlessMapFn structuredArgStripper = - (object, args) -> { - if (object instanceof String) { - args.accept(argStripper.strip((String) object)); - } else { - args.accept(CommandLineItem.expandToCommandLine(object)); - } - }; - // This kind of special handling should not be extended. It is a hack that works around a - // limitation of the native implementation of location expansion: The output is just a list of - // strings, not a structured command line that would allow transparent path mapping. - // Instead, reimplement location expansion in Starlark and have it return an Args object. - final boolean isJavaAction = - mnemonic.equals("Javac") - || mnemonic.equals("JavacTurbine") - || mnemonic.equals("Turbine") - || mnemonic.equals("JavaResourceJar"); - return new PathMapper() { - @Override - public String getMappedExecPathString(ActionInput artifact) { - if (artifact instanceof DerivedArtifact || artifact instanceof ParamFileActionInput) { - return strip(artifact.getExecPath()).getPathString(); - } else { - return artifact.getExecPathString(); - } - } + @Override + public String getMappedExecPathString(ActionInput artifact) { + if (isSupportedInputType(artifact) && isOutputPath(artifact, outputRoot)) { + return strip(artifact.getExecPath()).getPathString(); + } else { + return artifact.getExecPathString(); + } + } - @Override - public PathFragment map(PathFragment execPath) { - return isOutputPath(execPath, outputRoot) ? strip(execPath) : execPath; - } + @Override + public PathFragment map(PathFragment execPath) { + return isOutputPath(execPath, outputRoot) ? strip(execPath) : execPath; + } - @Override - public List mapCustomStarlarkArgs(List args) { - if (!isStarlarkAction) { - return args; - } - // Add your favorite Starlark mnemonic that needs custom arg processing here. - if (!mnemonic.contains("Android") - && !mnemonic.equals("MergeManifests") - && !mnemonic.equals("StarlarkRClassGenerator") - && !mnemonic.equals("StarlarkAARGenerator") - && !mnemonic.equals("JetifySrcs") - && !mnemonic.equals("Desugar")) { - return args; - } - // Add your favorite arg to custom-process here. When Bazel finds one of these in the - // argument list (an argument name), it strips output path prefixes from the following - // argument (the argument value). - ImmutableList starlarkArgsToStrip = - ImmutableList.of( - "--mainData", - "--primaryData", - "--directData", - "--data", - "--resources", - "--mergeeManifests", - "--library", - "-i", - "--input"); - for (int i = 1; i < args.size(); i++) { - if (starlarkArgsToStrip.contains(args.get(i - 1))) { - args.set(i, argStripper.strip(args.get(i))); - } - } - return args; + @Override + public List mapCustomStarlarkArgs(List args) { + if (!isStarlarkAction) { + return args; + } + // Add your favorite Starlark mnemonic that needs custom arg processing here. + if (!mnemonic.contains("Android") + && !mnemonic.equals("MergeManifests") + && !mnemonic.equals("StarlarkRClassGenerator") + && !mnemonic.equals("StarlarkAARGenerator") + && !mnemonic.equals("JetifySrcs") + && !mnemonic.equals("Desugar")) { + return args; + } + // Add your favorite arg to custom-process here. When Bazel finds one of these in the + // argument list (an argument name), it strips output path prefixes from the following + // argument (the argument value). + ImmutableList starlarkArgsToStrip = + ImmutableList.of( + "--mainData", + "--primaryData", + "--directData", + "--data", + "--resources", + "--mergeeManifests", + "--library", + "-i", + "--input"); + for (int i = 1; i < args.size(); i++) { + if (starlarkArgsToStrip.contains(args.get(i - 1))) { + args.set(i, argStripper.strip(args.get(i))); } + } + return args; + } - @Override - public ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { - if (isJavaAction) { - if (Objects.equals(previousFlag, "--javacopts") - || Objects.equals(previousFlag, "--resources")) { - return structuredArgStripper; - } - } - return MapFn.DEFAULT; + @Override + public ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { + if (isJavaAction) { + if (Objects.equals(previousFlag, "--javacopts") + || Objects.equals(previousFlag, "--resources")) { + return structuredArgStripper; } - }; + } + return MapFn.DEFAULT; + } + + @Override + public FileRootApi mapRoot(Artifact artifact) { + if (Objects.equals(artifact.getRoot(), outputArtifactRoot)) { + // The mapped root's path does not depend on the artifact, so we can share an instance. + return strippedOutputArtifactRoot; + } + // Fall back for source roots as well as middleman artifacts, which should be very rare. + return PathMapper.super.mapRoot(artifact); + } + + private boolean isSupportedInputType(ActionInput artifact) { + return artifact instanceof DerivedArtifact || artifact instanceof ParamFileActionInput; } /** Utility class to strip output path configuration prefixes from arbitrary strings. */ @@ -250,22 +278,18 @@ private static boolean isOutputPath(PathFragment pathFragment, PathFragment outp */ private static boolean isPathStrippable( Iterable actionInputs, PathFragment outputRoot) { - // For qualifying action types, check that no inputs or outputs would clash if paths were - // removed, e.g. "bazel-out/k8-fastbuild/foo" and "bazel-out/host/foo". - // - // A more clever algorithm could remap these with custom prefixes - "bazel-out/1/foo" and - // "bazel-out/2/foo" - if experience shows that would help. + // For qualifying action types, check that no inputs or outputs would clash if config segments + // were removed, e.g. "bazel-out/k8-fastbuild/bin/foo" and + // "bazel-out/k8-fastbuild-ST-1234/bin/foo". // - // Another approach could keep host paths intact (since the "host" path prefix doesn't vary - // with configurations). While this would help more action instances qualify, it also blocks - // caching the same action in host and target configurations. This could be mitigated by - // stripping the host prefix *only* when the entire action is in the host configuration. + // A more clever algorithm could remap these with custom prefixes - "bazel-out/1/bin/foo" and + // "bazel-out/2/bin/foo" - if experience shows that would help. HashMap rootRelativePaths = new HashMap<>(); for (ActionInput input : actionInputs) { if (!isOutputPath(input, outputRoot)) { continue; } - // For "bazel-out/k8-fastbuild/foo/bar", get "foo/bar". + // For "bazel-out/k8-fastbuild/bin/foo/bar", get "bin/foo/bar". if (!rootRelativePaths .computeIfAbsent(input.getExecPath().subFragment(2), k -> input) .equals(input)) { @@ -286,6 +310,4 @@ private static PathFragment strip(PathFragment execPath) { .getRelative(FIXED_CONFIG_SEGMENT) .getRelative(execPath.subFragment(2)); } - - private StrippingPathMapper() {} } 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 628bba3b6d950a..60299c532bc296 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 @@ -33,6 +33,8 @@ import com.google.devtools.build.lib.actions.FilesetOutputSymlink; 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; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -390,7 +392,8 @@ private int addToFingerprint( int argi, ActionKeyContext actionKeyContext, Fingerprint fingerprint, - @Nullable ArtifactExpander artifactExpander) + @Nullable ArtifactExpander artifactExpander, + CoreOptions.OutputPathsMode outputPathsMode) throws CommandLineExpansionException, InterruptedException { final Location location = ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; @@ -412,9 +415,8 @@ private int addToFingerprint( // below; // * the paths and possibly the digests of all input artifacts as well as the path mapping // mode, which are fingerprinted by SpawnAction. - // It is thus safe to use PathMapper.NOOP below for anything that relies on the default - // stringification behavior (which excludes custom mapEach functions, but those do not - // support path mapping yet). + // It is thus safe to ignore pathMapper below for anything that relies on the default + // stringification behavior (which excludes custom mapEach functions). if ((features & IS_NESTED_SET) != 0) { NestedSet values = (NestedSet) arguments.get(argi++); if (mapEach != null) { @@ -426,11 +428,12 @@ private int addToFingerprint( location, starlarkSemantics, (features & EXPAND_DIRECTORIES) != 0 ? artifactExpander : null, - PathMapper.NOOP); + outputPathsMode); try { actionKeyContext.addNestedSetToFingerprint(commandLineItemMapFn, fingerprint, values); } finally { - // The cache holds an entry for a NestedSet for every (map_fn, hasArtifactExpanderBit). + // The cache holds an entry for a NestedSet for every (map_fn, hasArtifactExpanderBit, + // pathMapperCacheKey). // Clearing the artifactExpander itself saves us from storing the contents of it in the // cache keys (it is no longer needed after we evaluate the value). // NestedSet cache is cleared after every build, which means that the artifactExpander @@ -447,7 +450,9 @@ private int addToFingerprint( int count = (Integer) arguments.get(argi++); List maybeExpandedValues = maybeExpandDirectories( - artifactExpander, arguments.subList(argi, argi + count), PathMapper.NOOP); + artifactExpander, + arguments.subList(argi, argi + count), + PathMappers.forActionKey(outputPathsMode)); argi += count; if (mapEach != null) { // TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase) @@ -462,7 +467,7 @@ private int addToFingerprint( fingerprint::addString, location, artifactExpander, - PathMapper.NOOP, + PathMappers.forActionKey(outputPathsMode), starlarkSemantics); } else { for (Object value : maybeExpandedValues) { @@ -937,6 +942,7 @@ public Iterable arguments( public void addToFingerprint( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, + CoreOptions.OutputPathsMode outputPathsMode, Fingerprint fingerprint) throws CommandLineExpansionException, InterruptedException { List arguments = rawArgsAsList(); @@ -953,7 +959,13 @@ public void addToFingerprint( if (arg instanceof VectorArg) { argi = ((VectorArg) arg) - .addToFingerprint(arguments, argi, actionKeyContext, fingerprint, artifactExpander); + .addToFingerprint( + arguments, + argi, + actionKeyContext, + fingerprint, + artifactExpander, + outputPathsMode); } else if (arg instanceof ScalarArg) { argi = ((ScalarArg) arg).addToFingerprint(arguments, argi, fingerprint); } else { @@ -1024,7 +1036,7 @@ private static void applyMapEach( } for (int i = 0; i < count; ++i) { args.set(0, originalValues.get(i)); - Object ret = Starlark.call(thread, mapFn, args, /*kwargs=*/ ImmutableMap.of()); + Object ret = Starlark.call(thread, mapFn, args, /* kwargs= */ ImmutableMap.of()); if (ret instanceof String) { consumer.accept((String) ret); } else if (ret instanceof Sequence) { @@ -1057,6 +1069,7 @@ private static class CommandLineItemMapEachAdaptor private final StarlarkCallable mapFn; private final Location location; private final StarlarkSemantics starlarkSemantics; + /** * Indicates whether artifactExpander was provided on construction. This is used to distinguish * the case where it's not provided from the case where it was provided but subsequently @@ -1064,7 +1077,7 @@ private static class CommandLineItemMapEachAdaptor */ private final boolean hasArtifactExpander; - private final PathMapper pathMapper; + private final CoreOptions.OutputPathsMode outputPathsMode; @Nullable private ArtifactExpander artifactExpander; @@ -1073,13 +1086,13 @@ private static class CommandLineItemMapEachAdaptor Location location, StarlarkSemantics starlarkSemantics, @Nullable ArtifactExpander artifactExpander, - PathMapper pathMapper) { + CoreOptions.OutputPathsMode outputPathsMode) { this.mapFn = mapFn; this.location = location; this.starlarkSemantics = starlarkSemantics; this.hasArtifactExpander = artifactExpander != null; this.artifactExpander = artifactExpander; - this.pathMapper = pathMapper; + this.outputPathsMode = outputPathsMode; } @Override @@ -1092,7 +1105,7 @@ public void expandToCommandLine(Object object, Consumer args) args, location, artifactExpander, - pathMapper, + PathMappers.forActionKey(outputPathsMode), starlarkSemantics); } @@ -1102,7 +1115,7 @@ private List maybeExpandDirectory(Object object) throws CommandLineExpan } return VectorArg.expandDirectories( - artifactExpander, ImmutableList.of(object), PathMapper.NOOP); + artifactExpander, ImmutableList.of(object), PathMappers.forActionKey(outputPathsMode)); } @Override @@ -1117,7 +1130,9 @@ public boolean equals(Object obj) { // We only compare presence of artifactExpander vs absence of it since the nestedset // fingerprint cache is emptied after every build, therefore if the artifact expander is // provided, it will be the same. - return mapFn == other.mapFn && hasArtifactExpander == other.hasArtifactExpander; + return mapFn == other.mapFn + && hasArtifactExpander == other.hasArtifactExpander + && outputPathsMode == other.outputPathsMode; } @Override @@ -1125,7 +1140,9 @@ public int hashCode() { // Force use of identityHashCode, in case the callable uses a custom hash function. (As of // this writing, only providers seem to have a custom hashCode, and those shouldn't be used // as map_each functions, but doesn't hurt to be safe...). - return 31 * System.identityHashCode(mapFn) + Boolean.hashCode(hasArtifactExpander); + return outputPathsMode.hashCode() + + 31 + * (Boolean.hashCode(hasArtifactExpander) + 31 * (System.identityHashCode(mapFn) + 1)); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD index 8ec70b0f13e657..c68ed5959bc88c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -61,6 +61,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line", "//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer", "//src/main/java/com/google/devtools/build/lib/analysis:actions/parameter_file_write_action", + "//src/main/java/com/google/devtools/build/lib/analysis:actions/path_mappers", "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_collection", diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 146ec1395b4fd6..51156c12777192 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -1342,7 +1342,13 @@ static void computeKey( fp.addInt(0); actionKeyContext.addNestedSetToFingerprint(fp, inputsForInvalidation); - PathMappers.addToFingerprint(mnemonic, executionInfo, outputPathsMode, fp); + PathMappers.addToFingerprint( + mnemonic, + executionInfo, + NestedSetBuilder.emptySet(Order.STABLE_ORDER), + actionKeyContext, + outputPathsMode, + fp); } private byte[] getCommandLineKey() throws CommandLineExpansionException { @@ -1368,7 +1374,9 @@ static byte[] computeCommandLineKey(List compilerOptions) { @Override public ActionResult execute(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { - PathMapper pathMapper = PathMappers.create(this, PathMappers.getOutputPathsMode(configuration)); + PathMapper pathMapper = + PathMappers.create( + this, PathMappers.getOutputPathsMode(configuration), /* isStarlarkAction= */ false); if (featureConfiguration.isEnabled(CppRuleClasses.COMPILER_PARAM_FILE)) { try { diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD index 6ad42581f2cff1..fb2654b3484cf5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD @@ -120,6 +120,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line", "//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer", "//src/main/java/com/google/devtools/build/lib/analysis:actions/lazy_write_paths_file_action", + "//src/main/java/com/google/devtools/build/lib/analysis:actions/path_mappers", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:build_info", "//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_collection", diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 9084c27b17bc8c..9d2784c2d2eaee 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -62,6 +62,7 @@ import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.analysis.actions.PathMappers; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -228,8 +229,9 @@ protected void computeKey( throws CommandLineExpansionException, InterruptedException { fp.addUUID(GUID); fp.addInt(classpathMode.ordinal()); - executableLine.addToFingerprint(actionKeyContext, artifactExpander, fp); - flagLine.addToFingerprint(actionKeyContext, artifactExpander, fp); + CoreOptions.OutputPathsMode outputPathsMode = PathMappers.getOutputPathsMode(configuration); + executableLine.addToFingerprint(actionKeyContext, artifactExpander, outputPathsMode, fp); + flagLine.addToFingerprint(actionKeyContext, artifactExpander, outputPathsMode, fp); // As the classpath is no longer part of commandLines implicitly, we need to explicitly add // the transitive inputs to the key here. actionKeyContext.addNestedSetToFingerprint(fp, transitiveInputs); @@ -240,7 +242,12 @@ protected void computeKey( getEnvironment().addTo(fp); fp.addStringMap(executionInfo); PathMappers.addToFingerprint( - getMnemonic(), getExecutionInfo(), PathMappers.getOutputPathsMode(configuration), fp); + getMnemonic(), + getExecutionInfo(), + getAdditionalArtifactsForPathMapping(), + actionKeyContext, + outputPathsMode, + fp); } /** @@ -299,7 +306,9 @@ private JavaSpawn getReducedSpawn( boolean fallback) throws CommandLineExpansionException, InterruptedException { CustomCommandLine.Builder classpathLine = CustomCommandLine.builder(); - PathMapper pathMapper = PathMappers.create(this, PathMappers.getOutputPathsMode(configuration)); + PathMapper pathMapper = + PathMappers.create( + this, PathMappers.getOutputPathsMode(configuration), /* isStarlarkAction= */ false); if (fallback) { classpathLine.addExecPaths("--classpath", transitiveInputs); @@ -342,7 +351,9 @@ private JavaSpawn getReducedSpawn( private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) throws CommandLineExpansionException, InterruptedException { - PathMapper pathMapper = PathMappers.create(this, PathMappers.getOutputPathsMode(configuration)); + PathMapper pathMapper = + PathMappers.create( + this, PathMappers.getOutputPathsMode(configuration), /* isStarlarkAction= */ false); CommandLines.ExpandedCommandLines expandedCommandLines = getCommandLines() .expand( 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 c65064ede3afd2..c0259eb933f638 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 @@ -18,9 +18,11 @@ import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.mock; +import com.google.common.base.Equivalence; import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.Lists; import com.google.common.testing.EqualsTester; +import com.google.common.testing.EquivalenceTester; import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactSerializationContext; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; @@ -41,6 +43,7 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecs; import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationDepsUtils; import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester; +import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.FileTypeSet; @@ -55,6 +58,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; +import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkSemantics; import org.junit.Before; import org.junit.Test; @@ -618,42 +622,94 @@ public PathFragment map(PathFragment execPath) { }; @Test - public void mappedArtifact_sourceArtifact() throws IOException { + public void mappedArtifact() { 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); + ArtifactRoot sourceArtifactRoot = ArtifactRoot.asSourceRoot(sourceRoot); + Artifact sourceArtifact1 = + ActionsTestUtil.createArtifactWithExecPath( + sourceArtifactRoot, PathFragment.create("path/to/pkg/file1")); + Artifact sourceArtifact2 = + ActionsTestUtil.createArtifactWithExecPath( + sourceArtifactRoot, PathFragment.create("path/to/pkg/file2")); Path execRoot = scratch.getFileSystem().getPath("/some/path"); - ArtifactRoot root = + ArtifactRoot outputArtifactRoot = 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)); + Artifact outputArtifact1 = + ActionsTestUtil.createArtifactWithExecPath( + outputArtifactRoot, PathFragment.create("output/k8-opt/bin/path/to/pkg/file1")); + Artifact outputArtifact2 = + ActionsTestUtil.createArtifactWithExecPath( + outputArtifactRoot, PathFragment.create("output/k8-opt/bin/path/to/pkg/file2")); + + assertThat(sourceArtifact1.getExecPathStringForStarlark(semantics)) + .isEqualTo("path/to/pkg/file1"); + assertThat(sourceArtifact1.getDirnameForStarlark(semantics)).isEqualTo("path/to/pkg"); + + FileRootApi mappedSourceRoot1 = sourceArtifact1.getRootForStarlark(semantics); + assertThat(mappedSourceRoot1.getExecPathString()).isEqualTo(""); + + assertThat(sourceArtifact2.getExecPathStringForStarlark(semantics)) + .isEqualTo("path/to/pkg/file2"); + assertThat(sourceArtifact2.getDirnameForStarlark(semantics)).isEqualTo("path/to/pkg"); + + FileRootApi mappedSourceRoot2 = sourceArtifact1.getRootForStarlark(semantics); + assertThat(mappedSourceRoot2.getExecPathString()).isEqualTo(""); + + assertThat(outputArtifact1.getExecPathStringForStarlark(semantics)) + .isEqualTo("output/3540078408/path/to/pkg/file1"); + assertThat(outputArtifact1.getDirnameForStarlark(semantics)) + .isEqualTo("output/3540078408/path/to/pkg"); + + FileRootApi mappedOutputRoot1 = outputArtifact1.getRootForStarlark(semantics); + assertThat(mappedOutputRoot1.getExecPathString()).isEqualTo("output/3540078408"); + + assertThat(outputArtifact2.getExecPathStringForStarlark(semantics)) + .isEqualTo("output/3540078409/path/to/pkg/file2"); + assertThat(outputArtifact2.getDirnameForStarlark(semantics)) + .isEqualTo("output/3540078409/path/to/pkg"); + + FileRootApi mappedOutputRoot2 = outputArtifact2.getRootForStarlark(semantics); + assertThat(mappedOutputRoot2.getExecPathString()).isEqualTo("output/3540078409"); + + // Starlark equality uses Object#equals. + // Mapped roots are always distinct from non-mapped roots, even if their paths are equal. + new EqualsTester() + .addEqualityGroup(mappedSourceRoot1, mappedSourceRoot2) + .addEqualityGroup(mappedOutputRoot1) + .addEqualityGroup(mappedOutputRoot2) + .addEqualityGroup(sourceRoot) + .addEqualityGroup(outputArtifactRoot) + .testEquals(); + + var starlarkCompare = + new Equivalence>() { + @Override + protected boolean doEquivalent(Comparable a, Comparable b) { + // Compare a and b in both directions as the implementations of compareTo may be + // different. + return Starlark.ORDERING.compare(a, b) == 0 && Starlark.ORDERING.compare(b, a) == 0; + } + + @Override + protected int doHash(Comparable comparable) { + return 0; + } + }; + + ClassCastException e = + assertThrows( + ClassCastException.class, + () -> Starlark.ORDERING.compare(mappedOutputRoot1, outputArtifactRoot)); + assertThat(e).hasMessageThat().isEqualTo("unsupported comparison: mapped_root <=> root"); + + EquivalenceTester.of(starlarkCompare) + .addEquivalenceGroup((Comparable) mappedSourceRoot1, (Comparable) mappedSourceRoot2) + .addEquivalenceGroup((Comparable) mappedOutputRoot1) + .addEquivalenceGroup((Comparable) mappedOutputRoot2) + .test(); } private static SpecialArtifact createTreeArtifact(ArtifactRoot root, String relativePath) { diff --git a/src/test/java/com/google/devtools/build/lib/actions/BUILD b/src/test/java/com/google/devtools/build/lib/actions/BUILD index 132463361f5998..b422e55a5711e7 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/test/java/com/google/devtools/build/lib/actions/BUILD @@ -50,6 +50,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line", "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//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/clock", @@ -65,6 +66,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe/serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils:depsutils", + "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//src/main/java/com/google/devtools/build/lib/testing/common:directory_listing_helper", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:filetype", diff --git a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java index e2566cd09e7649..fd21556472dae0 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg.SimpleVectorArg; +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; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -683,7 +684,11 @@ public void addToFingerPrint_computesUniqueKeyForDifferentCommandLines() throws Map digests = new HashMap<>(); for (CustomCommandLine commandLine : commandLines) { Fingerprint fingerprint = new Fingerprint(); - commandLine.addToFingerprint(actionKeyContext, /* artifactExpander= */ null, fingerprint); + commandLine.addToFingerprint( + actionKeyContext, + /* artifactExpander= */ null, + CoreOptions.OutputPathsMode.OFF, + fingerprint); String digest = fingerprint.hexDigestAndReset(); CustomCommandLine previous = digests.putIfAbsent(digest, commandLine); if (previous != null) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index f06e35ccb5e7c8..b98c10f4ef66d5 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -65,6 +65,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:actions/launcher_file_write_action", "//src/main/java/com/google/devtools/build/lib/analysis:actions/lazy_write_paths_file_action", "//src/main/java/com/google/devtools/build/lib/analysis:actions/parameter_file_write_action", + "//src/main/java/com/google/devtools/build/lib/analysis:actions/path_mappers", "//src/main/java/com/google/devtools/build/lib/analysis:actions/spawn_action_template", "//src/main/java/com/google/devtools/build/lib/analysis:actions/substitution", "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", 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/PathMappersTest.java similarity index 93% rename from src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java rename to src/test/java/com/google/devtools/build/lib/analysis/actions/PathMappersTest.java index ef20556420156f..7343c35c79a0aa 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/PathMappersTest.java @@ -22,9 +22,11 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; import com.google.devtools.build.lib.rules.java.JavaInfo; +import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import net.starlark.java.eval.Dict; import net.starlark.java.eval.Starlark; @@ -33,9 +35,9 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Tests for {@link StrippingPathMapper}. */ +/** Tests for {@link PathMappers}. */ @RunWith(JUnit4.class) -public class StrippingPathMapperTest extends BuildViewTestCase { +public class PathMappersTest extends BuildViewTestCase { @Before public void setUp() throws Exception { @@ -268,4 +270,14 @@ def my_rule_impl(ctx): "%s/cfg/bin/foo/script".formatted(outDir), "%s/cfg/bin/my_rule".formatted(outDir)) .inOrder(); } + + @Test + public void forActionKey() { + var pathMapper = PathMappers.forActionKey(CoreOptions.OutputPathsMode.STRIP); + assertThat(pathMapper.isNoop()).isFalse(); + assertThat(pathMapper.map(PathFragment.create("pkg/file"))) + .isEqualTo(PathFragment.create("pkg/file")); + assertThat(pathMapper.map(PathFragment.create("bazel-out/k8-fastbuild-ST-12345/bin/pkg/file"))) + .isEqualTo(PathFragment.create("bazel-out/pm-k8-fastbuild-ST-12345/bin/pkg/file")); + } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java index f515027753c836..40c0453f50d175 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.actions.HasDigest; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine.VectorArg; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.testutil.Scratch; @@ -75,7 +76,8 @@ public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_returnsDigest Fingerprint fingerprint = new Fingerprint(); // TODO(b/167696101): Fail arguments computation when we are missing the directory from inputs. - commandLine.addToFingerprint(actionKeyContext, EMPTY_EXPANDER, fingerprint); + commandLine.addToFingerprint( + actionKeyContext, EMPTY_EXPANDER, CoreOptions.OutputPathsMode.OFF, fingerprint); assertThat(fingerprint.digestAndReset()).isNotEmpty(); } @@ -95,7 +97,8 @@ public void vectorArgAddToFingerprint_expandFileset_includesInDigest() throws Ex /*treeExpansions=*/ ImmutableMap.of(), ImmutableMap.of(fileset, ImmutableList.of(symlink1, symlink2))); - commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); + commandLine.addToFingerprint( + actionKeyContext, artifactExpander, CoreOptions.OutputPathsMode.OFF, fingerprint); assertThat(fingerprint.digestAndReset()).isNotEmpty(); } @@ -113,7 +116,8 @@ public void vectorArgAddToFingerprint_expandTreeArtifact_includesInDigest() thro ImmutableMap.of(tree, ImmutableList.of(child)), /*filesetExpansions*/ ImmutableMap.of()); - commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); + commandLine.addToFingerprint( + actionKeyContext, artifactExpander, CoreOptions.OutputPathsMode.OFF, fingerprint); assertThat(fingerprint.digestAndReset()).isNotEmpty(); } @@ -129,7 +133,9 @@ public void vectorArgAddToFingerprint_expandFilesetMissingExpansion_fails() { assertThrows( CommandLineExpansionException.class, - () -> commandLine.addToFingerprint(actionKeyContext, EMPTY_EXPANDER, fingerprint)); + () -> + commandLine.addToFingerprint( + actionKeyContext, EMPTY_EXPANDER, CoreOptions.OutputPathsMode.OFF, fingerprint)); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/starlark/BUILD index ffa40d40d9c13a..7f29de29449179 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD @@ -42,6 +42,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/analysis:actions_provider", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//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:config/starlark_defined_config_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition", diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java index f45631b32cbe97..800896ccdcf034 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.actions.Substitution; import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction; +import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; @@ -140,38 +141,46 @@ public final void createBuildFile() throws Exception { scratch.file( "foo/BUILD", - "genrule(name = 'foo',", - " cmd = 'dummy_cmd',", - " srcs = ['a.txt', 'b.img'],", - " tools = ['t.exe'],", - " outs = ['c.txt'])", - "genrule(name = 'bar',", - " cmd = 'dummy_cmd',", - " srcs = [':jl', ':gl'],", - " outs = ['d.txt'])", - "genrule(name = 'baz',", - " cmd = 'dummy_cmd',", - " outs = ['e.txt'])", - "java_library(name = 'jl',", - " srcs = ['a.java'])", - "genrule(name = 'gl',", - " cmd = 'touch $(OUTS)',", - " srcs = ['a.go'],", - " outs = [ 'gl.a', 'gl.gcgox', ],", - " output_to_bindir = 1,", - ")", - // The target below is used by testResolveCommand and testResolveTools - "sh_binary(name = 'mytool',", - " srcs = ['mytool.sh'],", - " data = ['file1.dat', 'file2.dat'],", - ")", - // The target below is used by testResolveCommand and testResolveTools - "genrule(name = 'resolve_me',", - " cmd = 'aa',", - " tools = [':mytool', 't.exe'],", - " srcs = ['file3.dat', 'file4.dat'],", - " outs = ['r1.txt', 'r2.txt'],", - ")"); + """ + genrule(name = 'foo', + cmd = 'dummy_cmd', + srcs = ['a.txt', 'b.img'], + tools = ['t.exe'], + outs = ['c.txt']) + genrule(name = 'bar', + cmd = 'dummy_cmd', + srcs = [':jl', ':gl'], + outs = ['d.txt']) + genrule(name = 'baz', + cmd = 'dummy_cmd', + outs = ['e.txt']) + java_library(name = 'jl', + srcs = ['a.java']) + genrule(name = 'gl', + cmd = 'touch $(OUTS)', + srcs = ['a.go'], + outs = [ 'gl.a', 'gl.gcgox', ], + output_to_bindir = 1, + ) + # The target below is used by testResolveCommand and testResolveTools + sh_binary(name = 'mytool', + srcs = ['mytool.sh'], + data = ['file1.dat', 'file2.dat'], + ) + # The target below is used by testResolveCommand and testResolveTools + genrule(name = 'resolve_me', + cmd = 'aa', + tools = [':mytool', 't.exe'], + srcs = ['file3.dat', 'file4.dat'], + outs = ['r1.txt', 'r2.txt'], + ) + genrule(name = 'mixed_cfgs', + cmd = 'some_cmd', + srcs = ['a.txt', ':baz'], + tools = ['r1.txt'], + outs = ['out.txt'], + ) + """); } private void setRuleContext(StarlarkRuleContext ctx) throws Exception { @@ -3083,7 +3092,10 @@ public void testStarlarkCustomCommandLineKeyComputation() throws Exception { CommandLineExpansionException.class, () -> commandLine.addToFingerprint( - actionKeyContext, /* artifactExpander= */ null, new Fingerprint())); + actionKeyContext, + /* artifactExpander= */ null, + OutputPathsMode.OFF, + new Fingerprint())); } @Test @@ -3400,7 +3412,8 @@ public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringInAdd() .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); assertThat(getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER)) .isNotEqualTo(getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER)); - assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); } @Test @@ -3427,7 +3440,8 @@ public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringInAddFor .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); assertThat(getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER)) .isNotEqualTo(getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER)); - assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); } @Test @@ -3454,7 +3468,8 @@ public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringInAddAll .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); assertThat(getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER)) .isNotEqualTo(getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER)); - assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); } @Test @@ -3481,7 +3496,120 @@ public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringInAddAll .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); assertThat(getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER)) .isNotEqualTo(getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER)); - assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); + } + + @Test + public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringInAddAllDepsetMapEach() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + """ + def _map_each(x): + if type(x.obj) == "File": + return x.obj.path + return str(x.obj) + file = ruleContext.actions.declare_file('file') + args = ruleContext.actions.args() + args.add_all(depset([struct(obj = file.path)]), map_each=_map_each) + """); + CommandLine commandLine2 = + getCommandLine( + """ + def _map_each(x): + if type(x.obj) == "File": + return x.obj.path + return str(x.obj) + file = ruleContext.actions.declare_file('file') + args = ruleContext.actions.args() + args.add_all(depset([struct(obj = file)]), map_each=_map_each) + """); + + assertThat(getArguments(commandLine1, PathMapper.NOOP)) + .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); + assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2)); + + assertThat(getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER)) + .isNotEqualTo(getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER)); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); + } + + @Test + public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringInAddAllListMapEach() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + """ + def _map_each(x): + if type(x.obj) == "File": + return x.obj.path + return str(x.obj) + file = ruleContext.actions.declare_file('file') + args = ruleContext.actions.args() + args.add_all(depset([struct(obj = file.path)]), map_each=_map_each) + """); + CommandLine commandLine2 = + getCommandLine( + """ + def _map_each(x): + if type(x.obj) == "File": + return x.obj.path + return str(x.obj) + file = ruleContext.actions.declare_file('file') + args = ruleContext.actions.args() + args.add_all(depset([struct(obj = file)]), map_each=_map_each) + """); + + assertThat(getArguments(commandLine1, PathMapper.NOOP)) + .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); + assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2)); + + assertThat(getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER)) + .isNotEqualTo(getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER)); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); + } + + @Test + public void starlarkCustomCommandLineKeyComputation_artifactVsPathStringMapEachWithUniquify() + throws Exception { + setRuleContext(createRuleContext("//foo:mixed_cfgs")); + + CommandLine commandLine1 = + getCommandLine( + """ +def _map_each(x): + return x.field.root.path +args = ruleContext.actions.args() +d = depset([struct(field = f) for f in ruleContext.files.srcs + ruleContext.files.tools]) +args.add_all(d, map_each = _map_each, uniquify = True) +"""); + CommandLine commandLine2 = + getCommandLine( + """ +def _map_each(x): + return x.field +args = ruleContext.actions.args() +d = depset([struct(field = f.root.path) for f in ruleContext.files.srcs + ruleContext.files.tools]) +args.add_all(d, map_each = _map_each, uniquify = True) +"""); + + assertThat(getArguments(commandLine1, PathMapper.NOOP)) + .isEqualTo(getArguments(commandLine2, PathMapper.NOOP)); + assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2)); + + List arguments1 = getArguments(commandLine1, NON_TRIVIAL_PATH_MAPPER); + List arguments2 = getArguments(commandLine2, NON_TRIVIAL_PATH_MAPPER); + assertThat(arguments1).isNotEqualTo(arguments2); + assertThat(arguments1.size()).isNotEqualTo(arguments2.size()); + assertThat(getDigest(commandLine1, OutputPathsMode.STRIP)) + .isNotEqualTo(getDigest(commandLine2, OutputPathsMode.STRIP)); } private static ArtifactExpander createArtifactExpander(String dirRelativePath, String... files) { @@ -3509,13 +3637,24 @@ private static ArtifactExpander createArtifactExpander( private String getDigest(CommandLine commandLine) throws CommandLineExpansionException, InterruptedException { - return getDigest(commandLine, /*artifactExpander=*/ null); + return getDigest(commandLine, /* artifactExpander= */ null, OutputPathsMode.OFF); } private String getDigest(CommandLine commandLine, ArtifactExpander artifactExpander) throws CommandLineExpansionException, InterruptedException { + return getDigest(commandLine, artifactExpander, OutputPathsMode.OFF); + } + + private String getDigest(CommandLine commandLine, OutputPathsMode outputPathsMode) + throws CommandLineExpansionException, InterruptedException { + return getDigest(commandLine, /* artifactExpander= */ null, outputPathsMode); + } + + private String getDigest( + CommandLine commandLine, ArtifactExpander artifactExpander, OutputPathsMode outputPathsMode) + throws CommandLineExpansionException, InterruptedException { Fingerprint fingerprint = new Fingerprint(); - commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, outputPathsMode, fingerprint); return fingerprint.hexDigestAndReset(); } @@ -3530,12 +3669,7 @@ private CommandLine getCommandLine(RepositoryMapping mainRepoMapping, String... } private static final PathMapper NON_TRIVIAL_PATH_MAPPER = - new PathMapper() { - @Override - public PathFragment map(PathFragment path) { - return path.subFragment(0, 1).getChild("cfg").getRelative(path.subFragment(2)); - } - }; + path -> path.subFragment(0, 1).getChild("cfg").getRelative(path.subFragment(2)); private List getArguments(CommandLine commandLine, PathMapper pathMapper) throws CommandLineExpansionException, InterruptedException { diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 682c04e1176f00..fd30217d2b58c5 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -46,6 +46,15 @@ source "$(rlocation "io_bazel/src/test/shell/bazel/remote_helpers.sh")" \ source "$(rlocation "io_bazel/src/test/shell/bazel/remote/remote_utils.sh")" \ || { echo "remote_utils.sh not found!" >&2; exit 1; } +case "$(uname -s | tr [:upper:] [:lower:])" in +msys*|mingw*|cygwin*) + function is_windows() { true; } + ;; +*) + function is_windows() { false; } + ;; +esac + function set_up() { start_worker @@ -594,4 +603,412 @@ EOF expect_log ' 3 remote cache hit' } +function test_path_stripping_action_key_not_stale_for_path_collision() { + mkdir rules + cat > rules/defs.bzl <<'EOF' +LocationInfo = provider(fields = ["location"]) + +def _location_setting_impl(ctx): + return LocationInfo(location = ctx.build_setting_value) + +location_setting = rule( + implementation = _location_setting_impl, + build_setting = config.string(), +) + +def _location_transition_impl(settings, attr): + return {"//rules:location": attr.location} + +_location_transition = transition( + implementation = _location_transition_impl, + inputs = [], + outputs = ["//rules:location"], +) + +def _bazelcon_greeting_impl(ctx): + file = ctx.actions.declare_file("greeting.txt") + content = "Hello, BazelCon {}!\n".format(ctx.attr.location) + ctx.actions.write(file, content) + return [ + DefaultInfo(files = depset([file])), + LocationInfo(location = ctx.attr.location), + ] + +bazelcon_greeting = rule( + _bazelcon_greeting_impl, + cfg = _location_transition, + attrs = { + "location": attr.string(), + }, +) + +def _file_path(target): + return target[DefaultInfo].files.to_list()[0].path + +def _all_greetings_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name) + + targets = ctx.attr.greetings + if ctx.attr.sort: + targets = sorted(targets, key = lambda target: target[LocationInfo].location) + + args = ctx.actions.args() + args.add(out) + args.add_all(targets, map_each = _file_path) + + ctx.actions.run_shell( + inputs = depset(ctx.files.greetings), + outputs = [out], + arguments = [args], + command = "cat ${@:2} > $1", + execution_requirements = {"supports-path-mapping": ""}, + ) + + return [DefaultInfo(files = depset([out]))] + +all_greetings = rule( + _all_greetings_impl, + attrs = { + "greetings": attr.label_list(allow_files = True), + "sort": attr.bool(), + }, +) +EOF + cat > rules/BUILD << 'EOF' +load("//rules:defs.bzl", "location_setting") + +location_setting( + name = "location", + build_setting_default = "", +) +EOF + + mkdir -p pkg/greetings + cat > pkg/greetings/BUILD <<'EOF' +load("//rules:defs.bzl", "bazelcon_greeting") +bazelcon_greeting( + name = "munich", + location = "Munich", + visibility = ["//visibility:public"], +) +bazelcon_greeting( + name = "new_york", + location = "New York", + visibility = ["//visibility:public"], +) +bazelcon_greeting( + name = "mountain_view", + location = "Mountain View", + visibility = ["//visibility:public"], +) +EOF + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "all_greetings") +all_greetings( + name = "all_greetings", + greetings = [ + "//pkg/greetings:new_york", + "//pkg/greetings:munich", + "//pkg/greetings:mountain_view", + ], +) +EOF + + bazel build pkg:all_greetings -s \ + --experimental_output_paths=strip \ + --remote_executor=grpc://localhost:${worker_port} \ + &> $TEST_log || fail "run failed unexpectedly" + assert_equals "Hello, BazelCon New York! +Hello, BazelCon Munich! +Hello, BazelCon Mountain View!" "$(cat "$(bazel cquery --output=files //pkg:all_greetings)")" + + # Change the action command line in a way that only affects the unstripped + # map_each output. + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "all_greetings") +all_greetings( + name = "all_greetings", + greetings = [ + "//pkg/greetings:new_york", + "//pkg/greetings:munich", + "//pkg/greetings:mountain_view", + ], + sort = True, +) +EOF + + bazel build pkg:all_greetings \ + --experimental_output_paths=strip \ + --remote_executor=grpc://localhost:${worker_port} \ + &> $TEST_log || fail "run failed unexpectedly" + assert_equals "Hello, BazelCon Mountain View! +Hello, BazelCon Munich! +Hello, BazelCon New York!" "$(cat "$(bazel cquery --output=files //pkg:all_greetings)")" +} + +function test_path_stripping_deduplicated_action() { + if is_windows; then + echo "Skipping test_path_stripping_deduplicated_action on Windows as it requires sandboxing" + return + fi + + mkdir rules + touch rules/BUILD + cat > rules/defs.bzl <<'EOF' +def _slow_rule_impl(ctx): + out_file = ctx.actions.declare_file(ctx.attr.name + "_file") + out_dir = ctx.actions.declare_directory(ctx.attr.name + "_dir") + out_symlink = ctx.actions.declare_symlink(ctx.attr.name + "_symlink") + outs = [out_file, out_dir, out_symlink] + args = ctx.actions.args().add_all(outs, expand_directories = False) + ctx.actions.run_shell( + outputs = outs, + command = """ + # Sleep to ensure that two actions are scheduled in parallel. + sleep 3 + + echo "Hello, stdout!" + >&2 echo "Hello, stderr!" + + echo 'echo "Hello, file!"' > $1 + chmod +x $1 + echo 'Hello, dir!' > $2/file + ln -s $(basename $2)/file $3 + """, + arguments = [args], + execution_requirements = {"supports-path-mapping": ""}, + ) + return [ + DefaultInfo(files = depset(outs)), + ] + +slow_rule = rule(_slow_rule_impl) +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "slow_rule") + +slow_rule(name = "my_rule") + +COMMAND = """ +function validate() { + # Sorted by file name. + local -r dir=$$1 + local -r file=$$2 + local -r symlink=$$3 + + [[ $$($$file) == "Hello, file!" ]] || exit 1 + + [[ -d $$dir ]] || exit 1 + [[ $$(cat $$dir/file) == "Hello, dir!" ]] || exit 1 + + [[ -L $$symlink ]] || exit 1 + [[ $$(cat $$symlink) == "Hello, dir!" ]] || exit 1 +} +validate $(execpaths :my_rule) +touch $@ +""" + +genrule( + name = "gen_exec", + outs = ["out_exec"], + cmd = COMMAND, + tools = [":my_rule"], +) + +genrule( + name = "gen_target", + outs = ["out_target"], + cmd = COMMAND, + srcs = [":my_rule"], +) +EOF + + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + //pkg:all &> $TEST_log || fail "build failed unexpectedly" + # The first slow_write action plus two genrules. + expect_log '3 \(linux\|darwin\|processwrapper\)-sandbox' + expect_log '1 deduplicated' + + # Even though the spawn is only executed once, its stdout/stderr should be + # printed as if it wasn't deduplicated. + expect_log_once 'INFO: From Action pkg/my_rule_file:' + expect_log_once 'INFO: From Action pkg/my_rule_file \[for tool\]:' + expect_log_n 'Hello, stderr!' 2 + expect_log_n 'Hello, stdout!' 2 + + bazel clean || fail "clean failed unexpectedly" + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + //pkg:all &> $TEST_log || fail "build failed unexpectedly" + # The cache is checked before deduplication. + expect_log '4 remote cache hit' +} + +function test_path_stripping_deduplicated_action_output_not_created() { + if is_windows; then + echo "Skipping test_path_stripping_deduplicated_action_output_not_created on Windows as it requires sandboxing" + return + fi + + mkdir rules + touch rules/BUILD + cat > rules/defs.bzl <<'EOF' +def _slow_rule_impl(ctx): + out_file = ctx.actions.declare_file(ctx.attr.name + "_file") + outs = [out_file] + args = ctx.actions.args().add_all(outs) + ctx.actions.run_shell( + outputs = outs, + command = """ + # Sleep to ensure that two actions are scheduled in parallel. + sleep 3 + + echo "Hello, stdout!" + >&2 echo "Hello, stderr!" + + # Do not create the output file + """, + arguments = [args], + execution_requirements = {"supports-path-mapping": ""}, + ) + return [ + DefaultInfo(files = depset(outs)), + ] + +slow_rule = rule(_slow_rule_impl) +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "slow_rule") + +slow_rule(name = "my_rule") + +COMMAND = """ +[[ $$($(execpath :my_rule)) == "Hello, file!" ]] || exit 1 +touch $@ +""" + +genrule( + name = "gen_exec", + outs = ["out_exec"], + cmd = COMMAND, + tools = [":my_rule"], +) + +genrule( + name = "gen_target", + outs = ["out_target"], + cmd = COMMAND, + srcs = [":my_rule"], +) +EOF + + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + --keep_going \ + //pkg:all &> $TEST_log && fail "build succeeded unexpectedly" + # The second action runs normally after discovering that the first one failed + # to create the output file. + expect_log '2 \(linux\|darwin\|processwrapper\)-sandbox' + expect_not_log '[0-9] deduplicated' + + expect_log 'Action pkg/my_rule_file failed:' + expect_log 'Action pkg/my_rule_file \[for tool\] failed:' + # Remote cache warning. + expect_log 'Expected output pkg/my_rule_file was not created locally.' + + expect_log_once 'INFO: From Action pkg/my_rule_file:' + expect_log_once 'INFO: From Action pkg/my_rule_file \[for tool\]:' + expect_log_n 'Hello, stderr!' 2 + expect_log_n 'Hello, stdout!' 2 +} + +function test_path_stripping_deduplicated_action_non_zero_exit_code() { + if is_windows; then + echo "Skipping test_path_stripping_deduplicated_action_non_zero_exit_code on Windows as it requires sandboxing" + return + fi + + mkdir rules + touch rules/BUILD + cat > rules/defs.bzl <<'EOF' +def _slow_rule_impl(ctx): + out_file = ctx.actions.declare_file(ctx.attr.name + "_file") + outs = [out_file] + args = ctx.actions.args().add_all(outs) + ctx.actions.run_shell( + outputs = outs, + command = """ + # Sleep to ensure that two actions are scheduled in parallel. + sleep 3 + + echo "Hello, stdout!" + >&2 echo "Hello, stderr!" + + # Create the output file, but with a non-zero exit code. + echo 'echo "Hello, file!"' > $1 + exit 1 + """, + arguments = [args], + execution_requirements = {"supports-path-mapping": ""}, + ) + return [ + DefaultInfo(files = depset(outs)), + ] + +slow_rule = rule(_slow_rule_impl) +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load("//rules:defs.bzl", "slow_rule") + +slow_rule(name = "my_rule") + +COMMAND = """ +[[ $$($(execpath :my_rule)) == "Hello, file!" ]] || exit 1 +touch $@ +""" + +genrule( + name = "gen_exec", + outs = ["out_exec"], + cmd = COMMAND, + tools = [":my_rule"], +) + +genrule( + name = "gen_target", + outs = ["out_target"], + cmd = COMMAND, + srcs = [":my_rule"], +) +EOF + + bazel build \ + --experimental_output_paths=strip \ + --remote_cache=grpc://localhost:${worker_port} \ + --keep_going \ + //pkg:all &> $TEST_log && fail "build succeeded unexpectedly" + # Failing actions are not deduplicated. + expect_not_log '[0-9] deduplicated' + + expect_log 'Action pkg/my_rule_file failed:' + expect_log 'Action pkg/my_rule_file \[for tool\] failed:' + + # The first execution emits stdout/stderr, the second doesn't. + # stdout/stderr are emitted as part of the failing action error, not as an + # info. + expect_not_log 'INFO: From Action pkg/my_rule_file' + expect_log_n 'Hello, stderr!' 2 + expect_log_n 'Hello, stdout!' 2 +} + run_suite "path mapping tests"