diff options
author | Philipp Wollermann <philwo@google.com> | 2016-09-08 15:22:04 +0000 |
---|---|---|
committer | Damien Martin-Guillerez <dmarting@google.com> | 2016-09-09 09:02:48 +0000 |
commit | c9ee30213b8b59953ac993789cc1899716a8faf8 (patch) | |
tree | e41680cbe5e3fd7816f84b736183f42d6c5d36d9 | |
parent | 94d905836c167a21d2321d26c646fde07f5fc522 (diff) |
Make sure to always use a parameter file when building Java deploy jars with SingleJar.
Otherwise the WorkerSpawnStrategy (which requires the action to use a parameter file) will fail to execute JavaDeployJar Spawn actions when its CommandLine decides not to use a parameter file, because the arguments are short enough.
Also adds a convenience method SpawnAction#getArgumentsFromParamFile() for use in tests, because I needed it here to fix up the deploy jar tests.
--
MOS_MIGRATED_REVID=132557693
5 files changed, 92 insertions, 55 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java index 53adca39bd..c01afcf211 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis.actions; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -53,9 +54,19 @@ public abstract class CommandLine { } /** - * A default implementation of a command line backed by a copy of the given list of arguments. + * Returns the {@link ParameterFileWriteAction} that generates the parameter file used in this + * command line, or null if no parameter file is used. */ - static CommandLine ofInternal(Iterable<String> arguments, final boolean isShellCommand) { + @VisibleForTesting + public ParameterFileWriteAction parameterFileWriteAction() { + return null; + } + + /** A default implementation of a command line backed by a copy of the given list of arguments. */ + static CommandLine ofInternal( + Iterable<String> arguments, + final boolean isShellCommand, + final ParameterFileWriteAction paramFileWriteAction) { final Iterable<String> immutableArguments = CollectionUtils.makeImmutable(arguments); return new CommandLine() { @Override @@ -67,6 +78,11 @@ public abstract class CommandLine { public boolean isShellCommand() { return isShellCommand; } + + @Override + public ParameterFileWriteAction parameterFileWriteAction() { + return paramFileWriteAction; + } }; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java index 20834f921d..f1c7c16174 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis.actions; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ParameterFile; @@ -88,15 +87,17 @@ public final class ParamFileHelper { * @param isShellCommand true if this is a shell command * @param paramFileInfo parameter file information * @param parameterFile the output parameter file artifact + * @param paramFileWriteAction the action that generates the parameter file */ public static CommandLine createWithParamsFile( List<String> executableArgs, boolean isShellCommand, ParamFileInfo paramFileInfo, - Artifact parameterFile) { + Artifact parameterFile, + ParameterFileWriteAction paramFileWriteAction) { String pathWithFlag = paramFileInfo.getFlag() + parameterFile.getExecPathString(); Iterable<String> commandArgv = Iterables.concat(executableArgs, ImmutableList.of(pathWithFlag)); - return CommandLine.ofInternal(commandArgv, isShellCommand); + return CommandLine.ofInternal(commandArgv, isShellCommand, paramFileWriteAction); } /** @@ -104,19 +105,19 @@ public final class ParamFileHelper { * * @param arguments arguments to the command (in addition to executableArgs), OR * @param commandLine a {@link CommandLine} that provides the arguments (in addition to - * executableArgs) + * executableArgs) * @param owner owner of the action * @param parameterFile the output parameter file artifact * @param paramFileInfo parameter file information */ - public static Action createParameterFileWriteAction( + public static ParameterFileWriteAction createParameterFileWriteAction( @Nullable Iterable<String> arguments, @Nullable CommandLine commandLine, ActionOwner owner, Artifact parameterFile, ParamFileInfo paramFileInfo) { CommandLine paramFileContents = - (commandLine != null) ? commandLine : CommandLine.ofInternal(arguments, false); + (commandLine != null) ? commandLine : CommandLine.ofInternal(arguments, false, null); return new ParameterFileWriteAction(owner, parameterFile, paramFileContents, paramFileInfo.getFileType(), paramFileInfo.getCharset()); @@ -137,7 +138,7 @@ public final class ParamFileHelper { Iterable<String> arguments, CommandLine commandLine, boolean isShellCommand) { if (commandLine == null) { Iterable<String> commandArgv = Iterables.concat(executableArgs, arguments); - return CommandLine.ofInternal(commandArgv, isShellCommand); + return CommandLine.ofInternal(commandArgv, isShellCommand, null); } if (executableArgs.isEmpty()) { 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 999f094430..3cb899a175 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 @@ -208,9 +208,19 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie } /** - * Returns command argument, argv[0]. + * Returns the list of options written to the parameter file. Don't use this method outside tests. + * The list is often huge, resulting in significant garbage collection overhead. */ @VisibleForTesting + public List<String> getArgumentsFromParamFile() { + if (argv.parameterFileWriteAction() != null) { + return ImmutableList.copyOf(argv.parameterFileWriteAction().getContents()); + } + return ImmutableList.of(); + } + + /** Returns command argument, argv[0]. */ + @VisibleForTesting public String getCommandFilename() { return Iterables.getFirst(argv.arguments(), null); } @@ -567,20 +577,24 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie analysisEnvironment, outputs); - List<Action> actions = new ArrayList<>(2); - - // Set up the SpawnAction itself. - actions.add(buildSpawnAction(owner, configuration.getLocalShellEnvironment(), - configuration.getShExecutable(), paramsFile)); - // If param file is to be used, set up the param file write action as well. + ParameterFileWriteAction paramFileWriteAction = null; if (paramsFile != null) { - actions.add(ParamFileHelper.createParameterFileWriteAction( - arguments, - commandLine, - owner, - paramsFile, - paramFileInfo)); + paramFileWriteAction = + ParamFileHelper.createParameterFileWriteAction( + arguments, commandLine, owner, paramsFile, paramFileInfo); + } + + List<Action> actions = new ArrayList<>(2); + actions.add( + buildSpawnAction( + owner, + configuration.getLocalShellEnvironment(), + configuration.getShExecutable(), + paramsFile, + paramFileWriteAction)); + if (paramFileWriteAction != null) { + actions.add(paramFileWriteAction); } return actions.toArray(new Action[actions.size()]); @@ -600,21 +614,25 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie * @param defaultShellEnvironment the default shell environment to use. May be null if not used. * @param defaultShellExecutable the default shell executable path. May be null if not used. * @param paramsFile the parameter file for the SpawnAction. May be null if not used. + * @param paramFileWriteAction the action generating the parameter file. May be null if not + * used. * @return the SpawnAction and any actions required by it, with the first item always being the - * SpawnAction itself. + * SpawnAction itself. */ SpawnAction buildSpawnAction( ActionOwner owner, @Nullable Map<String, String> defaultShellEnvironment, @Nullable PathFragment defaultShellExecutable, - @Nullable Artifact paramsFile) { + @Nullable Artifact paramsFile, + @Nullable ParameterFileWriteAction paramFileWriteAction) { List<String> argv = buildExecutableArgs(defaultShellExecutable); Iterable<String> arguments = argumentsBuilder.build(); CommandLine actualCommandLine; if (paramsFile != null) { inputsBuilder.add(paramsFile); - actualCommandLine = ParamFileHelper.createWithParamsFile(argv, isShellCommand, - paramFileInfo, paramsFile); + actualCommandLine = + ParamFileHelper.createWithParamsFile( + argv, isShellCommand, paramFileInfo, paramsFile, paramFileWriteAction); } else { actualCommandLine = ParamFileHelper.createWithoutParamsFile(argv, arguments, commandLine, isShellCommand); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java index 0d72686cc1..65724891bc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis.actions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Actions; @@ -167,7 +166,8 @@ public final class SpawnActionTemplate implements ActionAnalysisMetadata { getOwner(), /*defaultShellEnvironment=*/ null, /*defaultShellExecutable=*/ null, - /*paramsFile=*/ null); + /*paramsFile=*/ null, + /*paramFileWriteAction=*/ null); } private static void checkActionAndArtifactConflicts(Iterable<ActionAnalysisMetadata> actions) @@ -266,7 +266,7 @@ public final class SpawnActionTemplate implements ActionAnalysisMetadata { @Override public Iterable<String> getClientEnvironmentVariables() { return spawnActionBuilder - .buildSpawnAction(getOwner(), null, null, null) + .buildSpawnAction(getOwner(), null, null, null, null) .getClientEnvironmentVariables(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java index edff8492e2..2a72321d61 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java @@ -239,33 +239,35 @@ public class DeployArchiveBuilder { // eliminate this check, allowing only native singlejar. Artifact singlejar = getSingleJar(ruleContext); if (singlejar.getFilename().endsWith(".jar")) { - ruleContext.registerAction(new SpawnAction.Builder() - .addInputs(inputs.build()) - .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext)) - .addOutput(outputJar) - .setResources(resourceSet) - .setJarExecutable( - ruleContext.getHostConfiguration().getFragment(Jvm.class).getJavaExecutable(), - singlejar, - jvmArgs) - .setCommandLine(commandLine) - .useParameterFile(ParameterFileType.SHELL_QUOTED) - .setProgressMessage("Building deploy jar " + outputJar.prettyPrint()) - .setMnemonic("JavaDeployJar") - .setExecutionInfo(ImmutableMap.of("supports-workers", "1")) - .build(ruleContext)); + ruleContext.registerAction( + new SpawnAction.Builder() + .addInputs(inputs.build()) + .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext)) + .addOutput(outputJar) + .setResources(resourceSet) + .setJarExecutable( + ruleContext.getHostConfiguration().getFragment(Jvm.class).getJavaExecutable(), + singlejar, + jvmArgs) + .setCommandLine(commandLine) + .alwaysUseParameterFile(ParameterFileType.SHELL_QUOTED) + .setProgressMessage("Building deploy jar " + outputJar.prettyPrint()) + .setMnemonic("JavaDeployJar") + .setExecutionInfo(ImmutableMap.of("supports-workers", "1")) + .build(ruleContext)); } else { - ruleContext.registerAction(new SpawnAction.Builder() - .addInputs(inputs.build()) - .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext)) - .addOutput(outputJar) - .setResources(resourceSet) - .setExecutable(singlejar) - .setCommandLine(commandLine) - .useParameterFile(ParameterFileType.SHELL_QUOTED) - .setProgressMessage("Building deploy jar " + outputJar.prettyPrint()) - .setMnemonic("JavaDeployJar") - .build(ruleContext)); + ruleContext.registerAction( + new SpawnAction.Builder() + .addInputs(inputs.build()) + .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext)) + .addOutput(outputJar) + .setResources(resourceSet) + .setExecutable(singlejar) + .setCommandLine(commandLine) + .alwaysUseParameterFile(ParameterFileType.SHELL_QUOTED) + .setProgressMessage("Building deploy jar " + outputJar.prettyPrint()) + .setMnemonic("JavaDeployJar") + .build(ruleContext)); } } |