diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java index 3a24ecfdb0b3ce..2c5bc990972059 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java @@ -300,4 +300,10 @@ public enum WorkerProtocolFormat { /** Requires the execution service do NOT share caches across different workspace. */ public static final String DIFFERENTIATE_WORKSPACE_CACHE = "internal-differentiate-workspace-cache"; + + /** + * Indicates that the action is compatible with path mapping, e.g., removing the configuration + * segment from the paths of all inputs and outputs. + */ + public static final String SUPPORTS_PATH_MAPPING = "supports-path-mapping"; } 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 1a98a05803b2e5..23c8b30a4e3331 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,6 +18,9 @@ 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.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; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; @@ -26,6 +29,7 @@ import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; import javax.annotation.Nullable; +import java.util.Map; /** * Utility methods that are the canonical way for actions to support path mapping (see {@link @@ -64,16 +68,20 @@ public final class PathMappers { * 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, OutputPathsMode outputPathsMode, Fingerprint fingerprint) { + String mnemonic, + Map executionInfo, + OutputPathsMode outputPathsMode, + Fingerprint fingerprint) { // 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. OutputPathsMode effectiveOutputPathsMode = - getEffectiveOutputPathsMode(outputPathsMode, mnemonic); + getEffectiveOutputPathsMode(outputPathsMode, mnemonic, executionInfo); if (effectiveOutputPathsMode == OutputPathsMode.STRIP) { fingerprint.addString(StrippingPathMapper.GUID); } @@ -85,14 +93,14 @@ public static void addToFingerprint( *

The returned {@link PathMapper} has to be passed to {@link * com.google.devtools.build.lib.actions.CommandLine#arguments(ArtifactExpander, PathMapper)}, * {@link com.google.devtools.build.lib.actions.CommandLines#expand(ArtifactExpander, - * PathFragment, PathMapper, CommandLineLimits)} or any other variants of these functions. The + * PathFragment, PathMapper, CommandLineLimits)} )} or any other variants of these functions. The * same instance should also be passed to the {@link Spawn} constructor so that the executor can * obtain it via {@link Spawn#getPathMapper()}. * *

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, + *

Actions calling this method should also call {@link #addToFingerprint(String, Map, * OutputPathsMode, Fingerprint)} from {@link Action#getKey(ActionKeyContext, ArtifactExpander)} * to ensure correct incremental builds. * @@ -102,7 +110,8 @@ public static void addToFingerprint( * PathMapper#NOOP} if path mapping is not applicable to the action. */ public static PathMapper create(Action action, OutputPathsMode outputPathsMode) { - if (getEffectiveOutputPathsMode(outputPathsMode, action.getMnemonic()) + if (getEffectiveOutputPathsMode( + outputPathsMode, action.getMnemonic(), action.getExecutionInfo()) != OutputPathsMode.STRIP) { return PathMapper.NOOP; } @@ -111,7 +120,8 @@ public static PathMapper create(Action action, OutputPathsMode outputPathsMode) public static PathMapper createPathMapperForTesting( Action action, OutputPathsMode outputPathsMode) { - if (getEffectiveOutputPathsMode(outputPathsMode, action.getMnemonic()) + if (getEffectiveOutputPathsMode( + outputPathsMode, action.getMnemonic(), action.getExecutionInfo()) != OutputPathsMode.STRIP) { return PathMapper.NOOP; } @@ -134,11 +144,13 @@ public static OutputPathsMode getOutputPathsMode( } private static OutputPathsMode getEffectiveOutputPathsMode( - OutputPathsMode outputPathsMode, String mnemonic) { - if (outputPathsMode != OutputPathsMode.STRIP || !SUPPORTED_MNEMONICS.contains(mnemonic)) { - return OutputPathsMode.OFF; + OutputPathsMode outputPathsMode, String mnemonic, Map executionInfo) { + if (outputPathsMode == OutputPathsMode.STRIP + && (SUPPORTED_MNEMONICS.contains(mnemonic) + || executionInfo.containsKey(ExecutionRequirements.SUPPORTS_PATH_MAPPING))) { + return OutputPathsMode.STRIP; } - return OutputPathsMode.STRIP; + return OutputPathsMode.OFF; } private PathMappers() {} 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 3afd323f18ad5f..a79834e25da8c2 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 @@ -385,7 +385,7 @@ protected void computeKey( } env.addTo(fp); fp.addStringMap(getExecutionInfo()); - PathMappers.addToFingerprint(getMnemonic(), outputPathsMode, fp); + PathMappers.addToFingerprint(getMnemonic(), getExecutionInfo(), outputPathsMode, fp); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 5e5754bb079908..79faa667066ec8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -768,7 +768,9 @@ public OutputPathsConverter() { help = "Which model to use for where in the output tree rules write their outputs, particularly " + "for multi-platform / multi-configuration builds. This is highly experimental. See " - + "https://github.com/bazelbuild/bazel/issues/6526 for details.") + + "https://github.com/bazelbuild/bazel/issues/6526 for details. Starlark actions can" + + "opt into path mapping by adding the key 'supports-path-mapping' to the " + + "'execution_requirements' dict.") public OutputPathsMode outputPathsMode; @Option( 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 379969febe2516..9705a43d26c369 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 @@ -244,7 +244,8 @@ protected void computeKey( } getEnvironment().addTo(fp); fp.addStringMap(executionInfo); - PathMappers.addToFingerprint(getMnemonic(), PathMappers.getOutputPathsMode(configuration), fp); + PathMappers.addToFingerprint( + getMnemonic(), getExecutionInfo(), PathMappers.getOutputPathsMode(configuration), fp); } /** diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java new file mode 100644 index 00000000000000..a92969358d8af5 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java @@ -0,0 +1,186 @@ +package com.google.devtools.build.lib.analysis.actions; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; + +import com.google.devtools.build.lib.actions.Artifact; +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.util.BuildViewTestCase; +import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; +import com.google.devtools.build.lib.rules.java.JavaInfo; +import java.io.IOException; +import net.starlark.java.eval.Dict; +import net.starlark.java.eval.Starlark; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link StrippingPathMapper}. */ +@RunWith(JUnit4.class) +public class StrippingPathMapperTest extends BuildViewTestCase { + + @Before + public void setUp() throws Exception { + useConfiguration("--experimental_output_paths=strip"); + } + + @Test + public void javaLibraryWithJavacopts() throws Exception { + scratch.file( + "java/com/google/test/BUILD", + "genrule(", + " name = 'gen_b',", + " outs = ['B.java'],", + " cmd = '',", + ")", + "genrule(", + " name = 'gen_c',", + " outs = ['C.java'],", + " cmd = '',", + ")", + "java_library(", + " name = 'a',", + " javacopts = [", + " '-XepOpt:foo:bar=$(location B.java)',", + " '-XepOpt:baz=$(location C.java),$(location B.java)',", + " ],", + " srcs = [", + " 'A.java',", + " 'B.java',", + " 'C.java',", + " ],", + ")"); + + ConfiguredTarget configuredTarget = getConfiguredTarget("//java/com/google/test:a"); + Artifact compiledArtifact = + JavaInfo.getProvider(JavaCompilationArgsProvider.class, configuredTarget) + .getDirectCompileTimeJars() + .toList() + .get(0); + SpawnAction action = (SpawnAction) getGeneratingAction(compiledArtifact); + Spawn spawn = action.getSpawn(new ActionExecutionContextBuilder().build()); + + assertThat(spawn.getPathMapper().isNoop()).isFalse(); + assertThat( + spawn.getArguments().stream() + .filter(arg -> arg.contains("java/com/google/test/")) + .collect(toImmutableList())) + .containsExactly( + "java/com/google/test/A.java", + "bazel-out/bin/java/com/google/test/B.java", + "bazel-out/bin/java/com/google/test/C.java", + "bazel-out/bin/java/com/google/test/liba-hjar.jar", + "bazel-out/bin/java/com/google/test/liba-hjar.jdeps", + "-XepOpt:foo:bar=bazel-out/bin/java/com/google/test/B.java", + "-XepOpt:baz=bazel-out/bin/java/com/google/test/C.java,bazel-out/bin/java/com/google/test/B.java"); + } + + private void addStarlarkRule(Dict executionRequirements) throws IOException { + scratch.file("defs/BUILD.bazel"); + scratch.file( + "defs/defs.bzl", + "def _my_rule_impl(ctx):", + " args = ctx.actions.args()", + " args.add(ctx.outputs.out)", + " args.add_all(", + " depset(ctx.files.srcs),", + " before_each = '-source',", + " format_each = '<%s>',", + " )", + " ctx.actions.run(", + " outputs = [ctx.outputs.out],", + " inputs = ctx.files.srcs,", + " executable = ctx.executable._tool,", + " arguments = [args],", + " mnemonic = 'MyRuleAction',", + String.format(" execution_requirements = %s,", Starlark.repr(executionRequirements)), + " )", + " return [DefaultInfo(files = depset([ctx.outputs.out]))]", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = {", + " 'srcs': attr.label_list(allow_files = True),", + " 'out': attr.output(mandatory = True),", + " '_tool': attr.label(", + " default = '//tool',", + " executable = True,", + " cfg = 'exec',", + " ),", + " },", + ")"); + scratch.file( + "pkg/BUILD", + "load('//defs:defs.bzl', 'my_rule')", + "genrule(", + " name = 'gen_src',", + " outs = ['gen_src.txt'],", + " cmd = '',", + ")", + "my_rule(", + " name = 'my_rule',", + " out = 'out.bin',", + " srcs = [", + " ':gen_src',", + " 'source.txt',", + " ],", + ")"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'tool',", + " srcs = ['tool.sh'],", + " visibility = ['//visibility:public'],", + ")"); + } + + @Test + public void starlarkRule_optedInViaExecutionRequirements() throws Exception { + addStarlarkRule( + Dict.builder().put("supports-path-mapping", "1").buildImmutable()); + + ConfiguredTarget configuredTarget = getConfiguredTarget("//pkg:my_rule"); + Artifact outputArtifact = + configuredTarget.getProvider(FileProvider.class).getFilesToBuild().toList().get(0); + SpawnAction action = (SpawnAction) getGeneratingAction(outputArtifact); + Spawn spawn = action.getSpawn(new ActionExecutionContextBuilder().build()); + + assertThat(spawn.getPathMapper().isNoop()).isFalse(); + assertThat(spawn.getArguments().stream().collect(toImmutableList())) + .containsExactly( + "bazel-out/bin/tool/tool", + "bazel-out/bin/pkg/out.bin", + "-source", + "", + "-source", + "") + .inOrder(); + } + + @Test + public void starlarkRule_optedInViaModifyExecutionInfo() throws Exception { + useConfiguration( + "--experimental_output_paths=strip", + "--modify_execution_info=MyRuleAction=+supports-path-mapping"); + addStarlarkRule(Dict.empty()); + + ConfiguredTarget configuredTarget = getConfiguredTarget("//pkg:my_rule"); + Artifact outputArtifact = + configuredTarget.getProvider(FileProvider.class).getFilesToBuild().toList().get(0); + SpawnAction action = (SpawnAction) getGeneratingAction(outputArtifact); + Spawn spawn = action.getSpawn(new ActionExecutionContextBuilder().build()); + + assertThat(spawn.getPathMapper().isNoop()).isFalse(); + assertThat(spawn.getArguments().stream().collect(toImmutableList())) + .containsExactly( + "bazel-out/bin/tool/tool", + "bazel-out/bin/pkg/out.bin", + "-source", + "", + "-source", + "") + .inOrder(); + } +}