Skip to content

Commit

Permalink
Skylark: ctx.actions.run_shell uses helper script
Browse files Browse the repository at this point in the history 
If the shell command in
ctx.actions.run_shell.command is longer than the
platform's shell's limit, Bazel will dump the
command to a helper shell script and execute that
script in the run_shell action.

Genrules also write a helper script when
genrule.cmd is longer than the shell's limit, and
ctx.actions.run_shell now uses the same machinery.

Fixes #3589

Change-Id: Ib24dce90182ef69552deb2d400e00ae061537309
PiperOrigin-RevId: 167126560
  • Loading branch information
@laszlocsomor @vladmos
laszlocsomor authored and vladmos committed Aug 31, 2017
1 parent 2fec2c7 commit 36c2ac5
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 5 deletions.
21 changes: 21 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,27 @@ private static ImmutableList<String> buildCommandLineSimpleArgv(String command,
return ImmutableList.of(shellPath.getPathString(), "-c", command);
}

/**
* If {@code command} is too long, creates a helper shell script that runs that command.
*
* <p>Returns the {@link Artifact} corresponding to that script.
*
* <p>Otherwise, when {@code command} is shorter than the platform's shell's command length limit,
* this method does nothing and returns null.
*/
@Nullable
public static Artifact shellCommandHelperScriptMaybe(
RuleContext ruleCtx,
String command,
String scriptPostFix,
Map<String, String> executionInfo) {
if (command.length() <= maxCommandLength) {
return null;
} else {
return buildCommandLineArtifact(ruleCtx, command, scriptPostFix);
}
}

/**
* Builds the set of command-line arguments. Creates a bash script if the
* command line is longer than the allowed maximum {@link #maxCommandLength}.
Expand Down
32 changes: 27 additions & 5 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.extra.SpawnInfo;
import com.google.devtools.build.lib.analysis.CommandHelper;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.PseudoAction;
import com.google.devtools.build.lib.analysis.RuleContext;
Expand All @@ -34,6 +35,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.ParamType;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
Expand Down Expand Up @@ -72,6 +74,8 @@ public class SkylarkActionFactory implements SkylarkValue {
private final SkylarkRuleContext context;
private final SkylarkSemanticsOptions skylarkSemanticsOptions;
private RuleContext ruleContext;
/** Counter for actions.run_shell helper scripts. Every script must have a unique name. */
private int runShellOutputCounter = 0;

public SkylarkActionFactory(
SkylarkRuleContext context,
Expand Down Expand Up @@ -573,7 +577,23 @@ public void runShell(
}

if (commandUnchecked instanceof String) {
builder.setShellCommand((String) commandUnchecked);
Map<String, String> executionInfo =
ImmutableMap.copyOf(TargetUtils.getExecutionInfo(ruleContext.getRule()));
String helperScriptSuffix = String.format(".run_shell_%d.sh", runShellOutputCounter++);
String command = (String) commandUnchecked;
Artifact helperScript =
CommandHelper.shellCommandHelperScriptMaybe(
ruleContext, command, helperScriptSuffix, executionInfo);
if (helperScript == null) {
builder.setShellCommand(command);
} else {
builder.setShellCommand(helperScript.getExecPathString());
builder.addInput(helperScript);
FilesToRunProvider provider = context.getExecutableRunfiles(helperScript);
if (provider != null) {
builder.addTool(provider);
}
}
} else if (commandUnchecked instanceof SkylarkList) {
SkylarkList commandList = (SkylarkList) commandUnchecked;
if (commandList.size() < 3) {
Expand Down Expand Up @@ -601,18 +621,20 @@ public void runShell(
}

/**
* Stup for spawn actions common between {@link #run} and {@link #runShell}.
* Setup for spawn actions common between {@link #run} and {@link #runShell}.
*
* {@code builder} should have either executable or a command set.
* <p>{@code builder} should have either executable or a command set.
*/
private void registerSpawnAction(
SkylarkList outputs,
Object inputs,
Object mnemonicUnchecked,
Object progressMessage,
Boolean useDefaultShellEnv, Object envUnchecked,
Boolean useDefaultShellEnv,
Object envUnchecked,
Object executionRequirementsUnchecked,
Object inputManifestsUnchecked, SpawnAction.Builder builder)
Object inputManifestsUnchecked,
SpawnAction.Builder builder)
throws EvalException {
// TODO(bazel-team): builder still makes unnecessary copies of inputs, outputs and args.
Iterable<Artifact> inputArtifacts;
Expand Down
86 changes: 86 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.devtools.build.lib.analysis.ActionsProvider;
import com.google.devtools.build.lib.analysis.FileConfiguredTarget;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleContext;
import com.google.devtools.build.lib.cmdline.Label;
Expand Down Expand Up @@ -1675,6 +1676,91 @@ public void testSpawnActionInterface() throws Exception {
assertThat((Boolean) result).isTrue();
}

@Test
public void testRunShellUsesHelperScriptForLongCommand() throws Exception {
// createRuleContext() gives us the context for a rule upon entry into its analysis function.
// But we need to inspect the result of calling created_actions() after the rule context has
// been modified by creating actions. So we'll call created_actions() from within the analysis
// function and pass it along as a provider.
scratch.file(
"test/rules.bzl",
"def _undertest_impl(ctx):",
" out1 = ctx.outputs.out1",
" out2 = ctx.outputs.out2",
" out3 = ctx.outputs.out3",
" ctx.actions.run_shell(outputs=[out1],",
" command='( %s ; ) > $1' % (",
" ' ; '.join(['echo xxx%d' % i for i in range(0, 7000)])),",
" mnemonic='mnemonic1',",
" arguments=[out1.path])",
" ctx.actions.run_shell(outputs=[out2],",
" command='echo foo > ' + out2.path,",
" mnemonic='mnemonic2')",
" ctx.actions.run_shell(outputs=[out3],",
" command='( %s ; ) > $1' % (",
" ' ; '.join(['echo yyy%d' % i for i in range(0, 7000)])),",
" mnemonic='mnemonic3',",
" arguments=[out3.path])",
" v = ctx.created_actions().by_file",
" return struct(v=v, out1=out1, out2=out2, out3=out3)",
"",
"undertest_rule = rule(",
" implementation=_undertest_impl,",
" outputs={'out1': '%{name}1.txt',",
" 'out2': '%{name}2.txt',",
" 'out3': '%{name}3.txt'},",
" _skylark_testable = True,",
")",
testingRuleDefinition);
scratch.file("test/BUILD", simpleBuildDefinition);
SkylarkRuleContext ruleContext = createRuleContext("//test:testing");

Object mapUnchecked = evalRuleContextCode(ruleContext, "ruleContext.attr.dep.v");
assertThat(mapUnchecked).isInstanceOf(SkylarkDict.class);
SkylarkDict<?, ?> map = (SkylarkDict<?, ?>) mapUnchecked;
Object out1 = eval("ruleContext.attr.dep.out1");
Object out2 = eval("ruleContext.attr.dep.out2");
Object out3 = eval("ruleContext.attr.dep.out3");
// 5 actions in total: 3 SpawnActions and 2 FileWriteActions for the two long commands.
assertThat(map).hasSize(5);
assertThat(map).containsKey(out1);
assertThat(map).containsKey(out2);
assertThat(map).containsKey(out3);
Object action1Unchecked = map.get(out1);
Object action2Unchecked = map.get(out2);
Object action3Unchecked = map.get(out3);
assertThat(action1Unchecked).isInstanceOf(ActionAnalysisMetadata.class);
assertThat(action2Unchecked).isInstanceOf(ActionAnalysisMetadata.class);
assertThat(action3Unchecked).isInstanceOf(ActionAnalysisMetadata.class);
ActionAnalysisMetadata spawnAction1 = (ActionAnalysisMetadata) action1Unchecked;
ActionAnalysisMetadata spawnAction2 = (ActionAnalysisMetadata) action2Unchecked;
ActionAnalysisMetadata spawnAction3 = (ActionAnalysisMetadata) action3Unchecked;
assertThat(spawnAction1.getMnemonic()).isEqualTo("mnemonic1");
assertThat(spawnAction2.getMnemonic()).isEqualTo("mnemonic2");
assertThat(spawnAction3.getMnemonic()).isEqualTo("mnemonic3");
Artifact helper1 =
Iterables.getOnlyElement(
Iterables.filter(
spawnAction1.getInputs(), a -> a.getFilename().equals("undertest.run_shell_0.sh")));
assertThat(
Iterables.filter(spawnAction2.getInputs(), a -> a.getFilename().contains("run_shell_")))
.isEmpty();
Artifact helper3 =
Iterables.getOnlyElement(
Iterables.filter(
spawnAction3.getInputs(), a -> a.getFilename().equals("undertest.run_shell_2.sh")));
assertThat(map).containsKey(helper1);
assertThat(map).containsKey(helper3);
Object action4Unchecked = map.get(helper1);
Object action5Unchecked = map.get(helper3);
assertThat(action4Unchecked).isInstanceOf(FileWriteAction.class);
assertThat(action5Unchecked).isInstanceOf(FileWriteAction.class);
FileWriteAction fileWriteAction1 = (FileWriteAction) action4Unchecked;
FileWriteAction fileWriteAction2 = (FileWriteAction) action5Unchecked;
assertThat(fileWriteAction1.getFileContents()).contains("echo xxx6999 ;");
assertThat(fileWriteAction2.getFileContents()).contains("echo yyy6999 ;");
}

@Test
public void testFileWriteActionInterface() throws Exception {
scratch.file("test/rules.bzl",
Expand Down

0 comments on commit 36c2ac5

Please sign in to comment.