From d51a88fdc43636e995bb3e34fd2881c5010b9920 Mon Sep 17 00:00:00 2001 From: mgeisler Date: Fri, 9 Apr 2021 06:17:17 -0700 Subject: [PATCH] Simplify build failure output by always using `NNN arguments` (plural) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When Blaze reports a compilation error, it shows the first bit of the command line used to invoke the compiler. When more than 200 characters have been shown (`APPROXIMATE_MAXIMUM_MESSAGE_LENGTH`), the rest of the command line is shown as: ``` (remaining NNN argument(s) skipped) ``` When I see the message in my console, it's normally because 100+ arguments are being skipped. So the correct form is then: ``` (remaining NNN arguments skipped) ``` I find this more readable since it avoids a nested parenthesis and it uses the correct plural form. Put differently, the `argument(s)` form is asking the reader to do the mental work of selecting between `argument` and `arguments` — which is unnecessary in the vast majority of cases. RELNOTES: Simplify build failure output by always using `NNN arguments`. PiperOrigin-RevId: 367620498 --- .../build/lib/util/CommandFailureUtils.java | 7 ++++-- .../lib/util/CommandFailureUtilsTest.java | 24 ++++++++++++++++++- .../build/lib/util/CommandUtilsTest.java | 16 ++++++------- 3 files changed, 36 insertions(+), 11 deletions(-) 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 5e74c58b2fd7d7..0ff99ad9d5c919 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 @@ -218,8 +218,11 @@ public static String describeCommand( for (String commandElement : commandLineElements) { if (form == CommandDescriptionForm.ABBREVIATED && message.length() + commandElement.length() > APPROXIMATE_MAXIMUM_MESSAGE_LENGTH) { - message.append( - " ... (remaining " + numberRemaining + " argument(s) skipped)"); + message + .append(" ... (remaining ") + .append(numberRemaining) + .append(numberRemaining == 1 ? " argument" : " arguments") + .append(" skipped)"); break; } else { if (numberRemaining < size) { 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 17798487902a32..9b960dd64b5f87 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 @@ -56,7 +56,7 @@ public void describeCommandError() throws Exception { + "arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 " + "arg19 arg20 arg21 arg22 arg23 arg24 arg25 arg26 " + "arg27 arg28 arg29 arg30 arg31 " - + "... (remaining 8 argument(s) skipped)"); + + "... (remaining 8 arguments skipped)"); assertThat(verboseMessage) .isEqualTo( "error executing command \n" @@ -72,6 +72,28 @@ public void describeCommandError() throws Exception { + "Execution platform: //platform:exec"); } + @Test + public void describeCommandErrorWithSingleSkippedArgument() throws Exception { + String[] args = new String[35]; // Long enough to make us skip 1 argument below. + args[0] = "some_command"; + for (int i = 1; i < args.length; i++) { + args[i] = "arg" + i; + } + Map env = new LinkedHashMap<>(); + String cwd = "/my/working/directory"; + PlatformInfo executionPlatform = + PlatformInfo.builder().setLabel(Label.parseAbsoluteUnchecked("//platform:exec")).build(); + String message = + CommandFailureUtils.describeCommandError( + false, Arrays.asList(args), env, cwd, executionPlatform); + assertThat(message) + .isEqualTo( + "error executing command some_command arg1 arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9" + + " arg10 arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 arg19 arg20 arg21 arg22" + + " arg23 arg24 arg25 arg26 arg27 arg28 arg29 arg30 arg31 arg32 arg33 ..." + + " (remaining 1 argument skipped)"); + } + @Test public void describeCommandFailure() throws Exception { String[] args = new String[3]; diff --git a/src/test/java/com/google/devtools/build/lib/util/CommandUtilsTest.java b/src/test/java/com/google/devtools/build/lib/util/CommandUtilsTest.java index 1f88c4952098e5..9acdd856d25439 100644 --- a/src/test/java/com/google/devtools/build/lib/util/CommandUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/CommandUtilsTest.java @@ -43,14 +43,14 @@ public void longCommand() throws Exception { assertThrows(CommandException.class, () -> new Command(args, env, directory).execute()); String message = CommandUtils.describeCommandError(false, exception.getCommand()); String verboseMessage = CommandUtils.describeCommandError(true, exception.getCommand()); - assertThat(message) - .isEqualTo( - "error executing command this_command_will_not_be_found arg1 " - + "arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9 arg10 " - + "arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 " - + "arg19 arg20 arg21 arg22 arg23 arg24 arg25 arg26 " - + "arg27 arg28 arg29 arg30 " - + "... (remaining 9 argument(s) skipped)"); + assertThat(message) + .isEqualTo( + "error executing command this_command_will_not_be_found arg1 " + + "arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9 arg10 " + + "arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 " + + "arg19 arg20 arg21 arg22 arg23 arg24 arg25 arg26 " + + "arg27 arg28 arg29 arg30 " + + "... (remaining 9 arguments skipped)"); assertThat(verboseMessage) .isEqualTo( "error executing command \n"