Skip to content

Commit

Permalink
Allow actions to opt into path mapping via execution requirements
Browse files Browse the repository at this point in the history
This provides a simple and backwards compatible way for actions to
experiment with path mapping:
* rules authors can add `supports-path-mapping` to the
  `execution_requirements` of a Starlark action
* end users can use tags and `--modify_execution_info` to opt individual
  targets or actions in and out of path mapping.

Also includes StrippingPathMapperTest, which wasn't included in the
merge of #18098, with an additional test for a Starlark rule that opts
in via the mechanisms described above.
  • Loading branch information
fmeum committed Oct 4, 2023
1 parent 9925a5b commit fb56b64
Show file tree
Hide file tree
Showing 6 changed files with 220 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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<String, String> 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);
}
Expand All @@ -85,14 +93,14 @@ public static void addToFingerprint(
* <p>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()}.
*
* <p>Note: This method flattens nested sets and should thus not be called from methods that are
* executed in the analysis phase.
*
* <p>Actions calling this method should also call {@link #addToFingerprint(String,
* <p>Actions calling this method should also call {@link #addToFingerprint(String, Map,
* OutputPathsMode, Fingerprint)} from {@link Action#getKey(ActionKeyContext, ArtifactExpander)}
* to ensure correct incremental builds.
*
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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<String, String> 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() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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 = '<some command>',",
")",
"genrule(",
" name = 'gen_c',",
" outs = ['C.java'],",
" cmd = '<some command>',",
")",
"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<String, String> 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 = '<some command>',",
")",
"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.<String, String>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",
"<bazel-out/bin/pkg/gen_src.txt>",
"-source",
"<pkg/source.txt>")
.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",
"<bazel-out/bin/pkg/gen_src.txt>",
"-source",
"<pkg/source.txt>")
.inOrder();
}
}

0 comments on commit fb56b64

Please sign in to comment.