Limit a genrule's runtime environment variables to those that appear in the command string.
Summary:
We are seeing problems in the wild where we are exceeding exec's ARG_MAX limit with
genrules that have extremely long lists of environment variables. ($DEPS is the biggest
offender, and will ultimately be deleted.)
This is intended as a quick fix. The long-term fix is to interpolate the env variables
in place in the command string as we do for `$(location)` and `$(exe)`. This will also
simplify Windows support where env variables appear as `$OUT` in the `bash` argument,
but `%OUT%` in the `cmd_exe` argument. Care must be taken to escape the interpolated
variables correctly.
Admittedly, this will introduce a regression for genrules that are using environment
variables in the scripts that they run, but do not reference them in the command string.
A notice will be sent out.
Test Plan:
Sandcastle builds.
Ran `buck build fb4a messenger instagram` with this diff.
Everything built successfully, which is good, as this exercises most of the genrules in the codebase.
This is evidence that there are few, if any, genrules in fbandroid that use environment variables
that are not referenced explicitly in the command string.
diff --git a/src/com/facebook/buck/shell/AbstractGenruleStep.java b/src/com/facebook/buck/shell/AbstractGenruleStep.java
index 760ac2d..8e13e21 100644
--- a/src/com/facebook/buck/shell/AbstractGenruleStep.java
+++ b/src/com/facebook/buck/shell/AbstractGenruleStep.java
@@ -102,6 +102,14 @@
@Override
protected ImmutableList<String> getShellCommandInternal(ExecutionContext context) {
+ ExecutionArgsAndCommand commandAndExecutionArgs = getCommandAndExecutionArgs(context);
+ return ImmutableList.<String>builder()
+ .addAll(commandAndExecutionArgs.executionArgs)
+ .add(commandAndExecutionArgs.command)
+ .build();
+ }
+
+ private ExecutionArgsAndCommand getCommandAndExecutionArgs(ExecutionContext context) {
// The priority sequence is
// "cmd.exe /c winCommand" (Windows Only)
// "/bin/bash -e -c shCommand" (Non-windows Only)
@@ -118,7 +126,7 @@
getFullyQualifiedName());
}
command = replaceMatches(context.getProjectFilesystem(), commandInUse);
- return ImmutableList.of("cmd.exe", "/c", command);
+ return new ExecutionArgsAndCommand(ImmutableList.of("cmd.exe", "/c"), command);
} else {
String commandInUse;
if (commandString.bash.isPresent()) {
@@ -130,17 +138,36 @@
getFullyQualifiedName());
}
command = replaceMatches(context.getProjectFilesystem(), commandInUse);
- return ImmutableList.of("/bin/bash", "-e", "-c", command);
+ return new ExecutionArgsAndCommand(ImmutableList.of("/bin/bash", "-e", "-c"), command);
}
}
@Override
public ImmutableMap<String, String> getEnvironmentVariables(ExecutionContext context) {
- ImmutableMap.Builder<String, String> environmentVariablesBuilder = ImmutableMap.builder();
+ ImmutableMap.Builder<String, String> allEnvironmentVariablesBuilder = ImmutableMap.builder();
+ addEnvironmentVariables(context, allEnvironmentVariablesBuilder);
+ ImmutableMap<String, String> allEnvironmentVariables = allEnvironmentVariablesBuilder.build();
- addEnvironmentVariables(context, environmentVariablesBuilder);
-
- return environmentVariablesBuilder.build();
+ // Long lists of environment variables can extend the length of the command such that it exceeds
+ // exec()'s ARG_MAX limit. Defend against this by filtering out variables that do not appear in
+ // the command string.
+ String command = getCommandAndExecutionArgs(context).command;
+ ImmutableMap.Builder<String, String> usedEnvironmentVariablesBuilder = ImmutableMap.builder();
+ for (Map.Entry<String, String> environmentVariable : allEnvironmentVariables.entrySet()) {
+ // We check for the presence of the variable without adornment for $ or %% so it works on both
+ // Windows and non-Windows environments. Eventually, we will require $ in the command string
+ // and modify the command directly rather than using envrionment variables.
+ String environmentVariableName = environmentVariable.getKey();
+ if (command.contains(environmentVariableName)) {
+ // I hate this $DEPS variable so much...
+ if ("DEPS".equals(environmentVariableName) &&
+ allEnvironmentVariables.containsKey("GEN_DIR")) {
+ usedEnvironmentVariablesBuilder.put("GEN_DIR", allEnvironmentVariables.get("GEN_DIR"));
+ }
+ usedEnvironmentVariablesBuilder.put(environmentVariable);
+ }
+ }
+ return usedEnvironmentVariablesBuilder.build();
}
abstract protected void addEnvironmentVariables(ExecutionContext context,
@@ -153,7 +180,7 @@
/**
* @return the cmd with binary and location build targets interpolated as either commands or the
- * location of the outputs of those targets.
+ * location of the outputs of those targets.
*/
@VisibleForTesting
String replaceMatches(ProjectFilesystem filesystem, String command) {
@@ -238,4 +265,14 @@
buildRule.getType().getName(),
getFullyQualifiedName());
}
+
+ private static class ExecutionArgsAndCommand {
+ private final ImmutableList<String> executionArgs;
+ private final String command;
+
+ private ExecutionArgsAndCommand(ImmutableList<String> executionArgs, String command) {
+ this.executionArgs = Preconditions.checkNotNull(executionArgs);
+ this.command = Preconditions.checkNotNull(command);
+ }
+ }
}
diff --git a/test/com/facebook/buck/android/ApkGenruleTest.java b/test/com/facebook/buck/android/ApkGenruleTest.java
index 5cdbadb..1544d44 100644
--- a/test/com/facebook/buck/android/ApkGenruleTest.java
+++ b/test/com/facebook/buck/android/ApkGenruleTest.java
@@ -214,13 +214,7 @@
ShellStep genruleCommand = (ShellStep) seventhStep;
assertEquals("genrule", genruleCommand.getShortName());
assertEquals(new ImmutableMap.Builder<String, String>()
- .put("SRCS", "/opt/local/fbandroid/src/com/facebook/signer.py " +
- "/opt/local/fbandroid/src/com/facebook/key.properties")
.put("APK", GEN_DIR + "/fb4a.apk")
- .put("GEN_DIR", "/opt/local/fbandroid/" + GEN_DIR)
- .put("DEPS", "")
- .put("TMP", "/opt/local/fbandroid/" + relativePathToTmpDir)
- .put("SRCDIR", "/opt/local/fbandroid/" + relativePathToSrcDir)
.put("OUT", expectedApkOutput).build(),
genruleCommand.getEnvironmentVariables(executionContext));
assertEquals(
diff --git a/test/com/facebook/buck/rules/FakeBuildRule.java b/test/com/facebook/buck/rules/FakeBuildRule.java
index 016c560..04e6706 100644
--- a/test/com/facebook/buck/rules/FakeBuildRule.java
+++ b/test/com/facebook/buck/rules/FakeBuildRule.java
@@ -50,6 +50,10 @@
this.type = Preconditions.checkNotNull(type);
}
+ public FakeBuildRule(BuildRuleType type, BuildTarget buildTarget) {
+ this(type, new FakeBuildRuleParams(buildTarget));
+ }
+
@Override
public Buildable getBuildable() {
return this;
diff --git a/test/com/facebook/buck/shell/GenruleTest.java b/test/com/facebook/buck/shell/GenruleTest.java
index df5cb81..ff5b34e 100644
--- a/test/com/facebook/buck/shell/GenruleTest.java
+++ b/test/com/facebook/buck/shell/GenruleTest.java
@@ -33,11 +33,13 @@
import com.facebook.buck.parser.NoSuchBuildTargetException;
import com.facebook.buck.parser.NonCheckingBuildRuleFactoryParams;
import com.facebook.buck.parser.ParseContext;
+import com.facebook.buck.rules.AbstractBuildRuleBuilderParams;
import com.facebook.buck.rules.BuildContext;
import com.facebook.buck.rules.BuildRule;
import com.facebook.buck.rules.BuildRuleResolver;
import com.facebook.buck.rules.BuildRuleType;
import com.facebook.buck.rules.FakeAbstractBuildRuleBuilderParams;
+import com.facebook.buck.rules.FakeBuildRule;
import com.facebook.buck.rules.FakeBuildableContext;
import com.facebook.buck.shell.Genrule.Builder;
import com.facebook.buck.step.ExecutionContext;
@@ -218,20 +220,9 @@
ShellStep genruleCommand = (ShellStep) sixthStep;
assertEquals("genrule", genruleCommand.getShortName());
assertEquals(ImmutableMap.<String, String>builder()
- .put("SRCS",
- getAbsolutePathInBase("/src/com/facebook/katana/convert_to_katana.py ").toString() +
- getAbsolutePathInBase("/src/com/facebook/katana/AndroidManifest.xml").toString())
.put("OUT",
getAbsolutePathInBase(
GEN_DIR + "/src/com/facebook/katana/AndroidManifest.xml").toString())
- .put("GEN_DIR",
- getAbsolutePathInBase(GEN_DIR).toString())
- .put("DEPS",
- "$GEN_DIR/java/com/facebook/util/lib__util__output/util.jar")
- .put("TMP",
- getAbsolutePathInBase(pathToTmpDir).toString())
- .put("SRCDIR",
- getAbsolutePathInBase(pathToSrcDir).toString())
.build(),
genruleCommand.getEnvironmentVariables(executionContext));
assertEquals(
@@ -239,6 +230,42 @@
genruleCommand.getShellCommand(executionContext));
}
+ @Test
+ public void testDepsEnvironmentVariableIsComplete() {
+ BuildTarget depTarget = new BuildTarget("//foo", "bar");
+ BuildRule dep = new FakeBuildRule(BuildRuleType.JAVA_LIBRARY, depTarget) {
+ @Override
+ public String getPathToOutputFile() {
+ return "buck-out/gen/foo/bar.jar";
+ }
+ };
+ BuildRuleResolver ruleResolver = new BuildRuleResolver(ImmutableMap.of(depTarget, dep));
+
+ AbstractBuildRuleBuilderParams params = new FakeAbstractBuildRuleBuilderParams();
+ Builder builder = Genrule.newGenruleBuilder(params);
+ builder.setBuildTarget(new BuildTarget("//foo", "baz"));
+ builder.setBash(Optional.of("cat $DEPS > $OUT"));
+ builder.setOut("deps.txt");
+ builder.addDep(depTarget);
+
+ Genrule genrule = builder.build(ruleResolver);
+ AbstractGenruleStep genruleStep = genrule.createGenruleStep();
+ ExecutionContext context = newEmptyExecutionContext(Platform.LINUX);
+ Map<String, String> environmentVariables = genruleStep.getEnvironmentVariables(context);
+ assertEquals(
+ "Make sure that the use of $DEPS pulls in $GEN_DIR, as well.",
+ ImmutableMap.of(
+ "DEPS", "$GEN_DIR/foo/bar.jar",
+ "GEN_DIR", "buck-out/gen",
+ "OUT", "buck-out/gen/foo/deps.txt"),
+ environmentVariables);
+
+ // Ensure that $GEN_DIR is declared before $DEPS.
+ List<String> keysInOrder = ImmutableList.copyOf(environmentVariables.keySet());
+ assertEquals("GEN_DIR", keysInOrder.get(1));
+ assertEquals("DEPS", keysInOrder.get(2));
+ }
+
private ExecutionContext newEmptyExecutionContext(Platform platform) {
return ExecutionContext.builder()
.setConsole(new Console(Verbosity.SILENT, System.out, System.err, Ansi.withoutTty()))
