diff options
13 files changed, 142 insertions, 44 deletions
diff --git a/site/docs/user-manual.html b/site/docs/user-manual.html index de4363c18b..5488052e1d 100644 --- a/site/docs/user-manual.html +++ b/site/docs/user-manual.html @@ -2329,6 +2329,12 @@ either to the terminal, or to additional log files. </p> <p> + <code class='flag'>--subcommands=pretty_print</code> may be passed to print + the arguments of the command as a list rather than as a single line. This may + help make long command lines more readable. +</p> + +<p> See also <a href="#flag--verbose_failures">--verbose_failures</a>, below. </p> diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index 2cea7155d4..037f47b1ad 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -43,6 +43,19 @@ import javax.annotation.Nullable; */ public class ActionExecutionContext implements Closeable { + /** Enum for --subcommands flag */ + public enum ShowSubcommands { + TRUE(true, false), PRETTY_PRINT(true, true), FALSE(false, false); + + private final boolean shouldShowSubcommands; + private final boolean prettyPrintArgs; + + private ShowSubcommands(boolean shouldShowSubcommands, boolean prettyPrintArgs) { + this.shouldShowSubcommands = shouldShowSubcommands; + this.prettyPrintArgs = prettyPrintArgs; + } + } + private final Executor executor; private final MetadataProvider actionInputFileCache; private final ActionInputPrefetcher actionInputPrefetcher; @@ -236,19 +249,15 @@ public class ActionExecutionContext implements Closeable { } /** - * Whether this Executor reports subcommands. If not, reportSubcommand has no effect. - * This is provided so the caller of reportSubcommand can avoid wastefully constructing the - * subcommand string. - */ - public boolean reportsSubcommands() { - return executor.reportsSubcommands(); - } - - /** * Report a subcommand event to this Executor's Reporter and, if action * logging is enabled, post it on its EventBus. */ - public void reportSubcommand(Spawn spawn) { + public void maybeReportSubcommand(Spawn spawn) { + ShowSubcommands showSubcommands = executor.reportsSubcommands(); + if (!showSubcommands.shouldShowSubcommands) { + return; + } + String reason; ActionOwner owner = spawn.getResourceOwner().getOwner(); if (owner == null) { @@ -257,7 +266,7 @@ public class ActionExecutionContext implements Closeable { reason = Label.print(owner.getLabel()) + " [" + spawn.getResourceOwner().prettyPrint() + "]"; } - String message = Spawns.asShellCommand(spawn, getExecRoot()); + String message = Spawns.asShellCommand(spawn, getExecRoot(), showSubcommands.prettyPrintArgs); getEventHandler().handle(Event.of(EventKind.SUBCOMMAND, null, "# " + reason + "\n" + message)); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Executor.java b/src/main/java/com/google/devtools/build/lib/actions/Executor.java index a2de3d6a78..2c90014454 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Executor.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Executor.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.actions; import com.google.common.eventbus.EventBus; +import com.google.devtools.build.lib.actions.ActionExecutionContext.ShowSubcommands; import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.FileSystem; @@ -74,7 +75,7 @@ public interface Executor { * This is provided so the caller of reportSubcommand can avoid wastefully constructing the * subcommand string. */ - boolean reportsSubcommands(); + ShowSubcommands reportsSubcommands(); /** * An event listener to report messages to. Errors that signal a action failure should use diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java index 3174d5b112..0ca66b71c2 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java @@ -89,17 +89,29 @@ public final class Spawns { } /** Convert a spawn into a Bourne shell command. */ - public static String asShellCommand(Spawn spawn, Path workingDirectory) { - return asShellCommand(spawn.getArguments(), workingDirectory, spawn.getEnvironment()); + public static String asShellCommand(Spawn spawn, Path workingDirectory, boolean prettyPrintArgs) { + return asShellCommand( + spawn.getArguments(), + workingDirectory, + spawn.getEnvironment(), + prettyPrintArgs); } /** Convert a working dir + environment map + arg list into a Bourne shell command. */ public static String asShellCommand( - Collection<String> arguments, Path workingDirectory, Map<String, String> environment) { + Collection<String> arguments, + Path workingDirectory, + Map<String, String> environment, + boolean prettyPrintArgs) { + // We print this command out in such a way that it can safely be // copied+pasted as a Bourne shell command. This is extremely valuable for // debugging. return CommandFailureUtils.describeCommand( - CommandDescriptionForm.COMPLETE, arguments, environment, workingDirectory.getPathString()); + CommandDescriptionForm.COMPLETE, + prettyPrintArgs, + arguments, + environment, + workingDirectory.getPathString()); } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index 0930675181..cb40215ad8 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -70,9 +70,9 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte ActionExecutionContext actionExecutionContext, AtomicReference<Class<? extends SpawnActionContext>> writeOutputFiles) throws ExecException, InterruptedException { - if (actionExecutionContext.reportsSubcommands()) { - actionExecutionContext.reportSubcommand(spawn); - } + + actionExecutionContext.maybeReportSubcommand(spawn); + final Duration timeout = Spawns.getTimeout(spawn); SpawnExecutionContext context = new SpawnExecutionContextImpl(spawn, actionExecutionContext, writeOutputFiles, timeout); diff --git a/src/main/java/com/google/devtools/build/lib/exec/BlazeExecutor.java b/src/main/java/com/google/devtools/build/lib/exec/BlazeExecutor.java index 5786a48bb9..52b0b35b64 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BlazeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/exec/BlazeExecutor.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.exec; import com.google.common.base.Preconditions; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.ActionContext; +import com.google.devtools.build.lib.actions.ActionExecutionContext.ShowSubcommands; import com.google.devtools.build.lib.actions.Executor; import com.google.devtools.build.lib.actions.ExecutorInitException; import com.google.devtools.build.lib.clock.Clock; @@ -44,7 +45,7 @@ import java.util.concurrent.atomic.AtomicBoolean; public final class BlazeExecutor implements Executor { private final boolean verboseFailures; - private final boolean showSubcommands; + private final ShowSubcommands showSubcommands; private final FileSystem fileSystem; private final Path execRoot; private final Reporter reporter; @@ -123,7 +124,7 @@ public final class BlazeExecutor implements Executor { } @Override - public boolean reportsSubcommands() { + public ShowSubcommands reportsSubcommands() { return showSubcommands; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index 735832bbb1..953639932f 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -14,11 +14,13 @@ package com.google.devtools.build.lib.exec; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.ActionExecutionContext.ShowSubcommands; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.analysis.config.PerLabelOptions; import com.google.devtools.build.lib.util.OptionsUtils; import com.google.devtools.build.lib.util.RegexFilter; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.BoolOrEnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -71,11 +73,12 @@ public class ExecutionOptions extends OptionsBase { name = "subcommands", abbrev = 's', defaultValue = "false", + converter = ShowSubcommandsConverter.class, documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.UNKNOWN}, help = "Display the subcommands executed during a build." ) - public boolean showSubcommands; + public ShowSubcommands showSubcommands; @Option( name = "check_up_to_date", @@ -363,4 +366,13 @@ public class ExecutionOptions extends OptionsBase { + "This flag may be passed more than once"; } } + + /** Converter for --subcommands */ + public static class ShowSubcommandsConverter extends BoolOrEnumConverter<ShowSubcommands> { + public ShowSubcommandsConverter() { + super( + ShowSubcommands.class, "subcommand option", ShowSubcommands.TRUE, ShowSubcommands.FALSE); + } + } + } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index 4a5849c61c..67a00b6e58 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -419,7 +419,10 @@ public class RunCommand implements BlazeCommand { if (runOptions.scriptPath != null) { String unisolatedCommand = CommandFailureUtils.describeCommand( CommandDescriptionForm.COMPLETE_UNISOLATED, - cmdLine, runEnvironment, workingDir.getPathString()); + /* prettyPrintArgs= */ false, + cmdLine, + runEnvironment, + workingDir.getPathString()); if (writeScript(env, shExecutable, runOptions.scriptPath, unisolatedCommand)) { return BlazeCommandResult.exitCode(ExitCode.SUCCESS); } else { diff --git a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java index dad0d72db6..5317e818c9 100644 --- a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java +++ b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java @@ -139,17 +139,23 @@ public class CommandFailureUtils { * @param form Form of the command to generate; see the documentation of the * {@link CommandDescriptionForm} values. */ - public static String describeCommand(CommandDescriptionForm form, + public static String describeCommand( + CommandDescriptionForm form, + boolean prettyPrintArgs, Collection<String> commandLineElements, - @Nullable Map<String, String> environment, @Nullable String cwd) { + @Nullable Map<String, String> environment, + @Nullable String cwd) { + Preconditions.checkNotNull(form); final int APPROXIMATE_MAXIMUM_MESSAGE_LENGTH = 200; StringBuilder message = new StringBuilder(); int size = commandLineElements.size(); int numberRemaining = size; + if (form == CommandDescriptionForm.COMPLETE) { describeCommandImpl.describeCommandBeginIsolate(message); } + if (form != CommandDescriptionForm.ABBREVIATED) { if (cwd != null) { describeCommandImpl.describeCommandCwd(cwd, message); @@ -195,6 +201,7 @@ public class CommandFailureUtils { } } } + for (String commandElement : commandLineElements) { if (form == CommandDescriptionForm.ABBREVIATED && message.length() + commandElement.length() > APPROXIMATE_MAXIMUM_MESSAGE_LENGTH) { @@ -203,15 +210,17 @@ public class CommandFailureUtils { break; } else { if (numberRemaining < size) { - message.append(' '); + message.append(prettyPrintArgs ? " \\\n " : " "); } describeCommandImpl.describeCommandElement(message, commandElement); numberRemaining--; } } + if (form == CommandDescriptionForm.COMPLETE) { describeCommandImpl.describeCommandEndIsolate(message); } + return message.toString(); } @@ -220,14 +229,17 @@ public class CommandFailureUtils { * Currently this returns a message of the form "error executing command foo * bar baz". */ - public static String describeCommandError(boolean verbose, - Collection<String> commandLineElements, - Map<String, String> env, String cwd) { + public static String describeCommandError( + boolean verbose, + Collection<String> commandLineElements, + Map<String, String> env, + String cwd) { + CommandDescriptionForm form = verbose ? CommandDescriptionForm.COMPLETE : CommandDescriptionForm.ABBREVIATED; return "error executing command " + (verbose ? "\n " : "") - + describeCommand(form, commandLineElements, env, cwd); + + describeCommand(form, /* prettyPrintArgs= */false, commandLineElements, env, cwd); } /** @@ -235,9 +247,12 @@ public class CommandFailureUtils { * Currently this returns a message of the form "foo failed: error executing * command /dir/foo bar baz". */ - public static String describeCommandFailure(boolean verbose, - Collection<String> commandLineElements, - Map<String, String> env, String cwd) { + public static String describeCommandFailure( + boolean verbose, + Collection<String> commandLineElements, + Map<String, String> env, + String cwd) { + String commandName = commandLineElements.iterator().next(); // Extract the part of the command name after the last "/", if any. String shortCommandName = new File(commandName).getName(); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java index 4211419531..55b0910239 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java @@ -123,6 +123,6 @@ final class WorkerKey { @Override public String toString() { - return Spawns.asShellCommand(args, execRoot, env); + return Spawns.asShellCommand(args, execRoot, env, /* prettyPrintArgs= */ false); } } diff --git a/src/main/java/com/google/devtools/common/options/BoolOrEnumConverter.java b/src/main/java/com/google/devtools/common/options/BoolOrEnumConverter.java index 3e16f89a51..17bf7a9a48 100644 --- a/src/main/java/com/google/devtools/common/options/BoolOrEnumConverter.java +++ b/src/main/java/com/google/devtools/common/options/BoolOrEnumConverter.java @@ -17,13 +17,16 @@ package com.google.devtools.common.options; import com.google.devtools.common.options.Converters.BooleanConverter; /** - * Converter that can also convert from booleans and enumerations. + * Converter that can convert both the standard set of boolean string values and enumerations. If + * there is an overlap in values, those from the underlying enumeration will be taken. * - * <p> This is able to additionally convert from the standard set of - * boolean string values. If there is an overlap in values, those from - * the underlying enumeration will be taken. + * <p>Note that for the flag to take one of its enum values on the command line, it must be of the + * form "--flag=value". That is, "--flag value" and "-f value" (if the flag has a short-form of "f") + * will result in "value" being left as residue on the command line. This maintains compatibility + * with boolean flags where "--flag true" and "-f true" also leave "true" as residue on the command + * line. */ -public abstract class BoolOrEnumConverter<T extends Enum<T>> extends EnumConverter<T>{ +public abstract class BoolOrEnumConverter<T extends Enum<T>> extends EnumConverter<T> { private T falseValue; private T trueValue; @@ -55,6 +58,8 @@ public abstract class BoolOrEnumConverter<T extends Enum<T>> extends EnumConvert boolean value = booleanConverter.convert(input); return value ? trueValue : falseValue; } catch (OptionsParsingException eBoolean) { + // TODO(b/111883901): Rethrowing the exception from the enum converter does not report the + // allowable boolean values. throw eEnum; } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/DummyExecutor.java b/src/test/java/com/google/devtools/build/lib/actions/util/DummyExecutor.java index 7b09b6b3f5..6703fdd413 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/DummyExecutor.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/DummyExecutor.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.actions.util; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.ActionContext; +import com.google.devtools.build.lib.actions.ActionExecutionContext.ShowSubcommands; import com.google.devtools.build.lib.actions.Executor; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.clock.Clock; @@ -87,7 +88,7 @@ public final class DummyExecutor implements Executor { } @Override - public boolean reportsSubcommands() { + public ShowSubcommands reportsSubcommands() { throw new UnsupportedOperationException(); } } diff --git a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java index 7ce30ec51d..f13758eecc 100644 --- a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java @@ -16,7 +16,7 @@ package com.google.devtools.build.lib.util; import static com.google.common.truth.Truth.assertThat; import java.util.Arrays; -import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -34,9 +34,9 @@ public class CommandFailureUtilsTest { } args[7] = "with spaces"; // Test embedded spaces in argument. args[9] = "*"; // Test shell meta characters. - Map<String, String> env = new HashMap<>(); - env.put("PATH", "/usr/bin:/bin:/sbin"); + Map<String, String> env = new LinkedHashMap<>(); env.put("FOO", "foo"); + env.put("PATH", "/usr/bin:/bin:/sbin"); String cwd = "/my/working/directory"; String message = CommandFailureUtils.describeCommandError(false, Arrays.asList(args), env, cwd); String verboseMessage = @@ -69,7 +69,7 @@ public class CommandFailureUtilsTest { args[0] = "/bin/sh"; args[1] = "-c"; args[2] = "echo Some errors 1>&2; echo Some output; exit 42"; - Map<String, String> env = new HashMap<>(); + Map<String, String> env = new LinkedHashMap<>(); env.put("FOO", "foo"); env.put("PATH", "/usr/bin:/bin:/sbin"); String cwd = null; @@ -89,4 +89,37 @@ public class CommandFailureUtilsTest { + " PATH=/usr/bin:/bin:/sbin \\\n" + " /bin/sh -c 'echo Some errors 1>&2; echo Some output; exit 42')"); } + + @Test + public void describeCommandPrettyPrintArgs() throws Exception { + + String[] args = new String[6]; + args[0] = "some_command"; + for (int i = 1; i < args.length; i++) { + args[i] = "arg" + i; + } + args[3] = "with spaces"; // Test embedded spaces in argument. + args[4] = "*"; // Test shell meta characters. + + Map<String, String> env = new LinkedHashMap<>(); + env.put("FOO", "foo"); + env.put("PATH", "/usr/bin:/bin:/sbin"); + + String cwd = "/my/working/directory"; + String message = CommandFailureUtils.describeCommand( + CommandDescriptionForm.COMPLETE, true, Arrays.asList(args), env, cwd); + + assertThat(message) + .isEqualTo( + "(cd /my/working/directory && \\\n" + + " exec env - \\\n" + + " FOO=foo \\\n" + + " PATH=/usr/bin:/bin:/sbin \\\n" + + " some_command \\\n" + + " arg1 \\\n" + + " arg2 \\\n" + + " 'with spaces' \\\n" + + " '*' \\\n" + + " arg5)"); + } } |