From 00c88e20feb87cdacb6150317b31d73b1e214038 Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Fri, 14 Aug 2020 18:07:41 +0200 Subject: [PATCH] Split long errors in several lines (#491) * Pretty print long errors in several lines This is done by using newline characters to insert a new error prefix. A couple of new facilities to encourage splitting of long errors are defined: Errors.Wrap and Errors.Set (Wrapping_Message, Wrapped_Exception) * Rename Last_Chance_Handler to Alr.Last_Chance_... To avoid possible conflicts with other libraries and follow the naming of other packages. * Fix testsuite output matches on new line breaks * Code review fixes --- src/alire/alire-errors.adb | 35 +++++++++++++++++++ src/alire/alire-errors.ads | 20 +++++++++++ src/alire/alire-lockfiles.adb | 3 +- src/alire/alire-releases.adb | 7 ++-- src/alire/alire-root.adb | 3 +- src/alire/alire-toml_adapters.adb | 7 ++-- src/alire/alire-toml_adapters.ads | 3 +- src/alire/alire-toml_load.adb | 18 ++++++++-- src/alire/alire-toml_load.ads | 4 +++ src/alr/alr-commands.adb | 9 ++--- ...andler.adb => alr-last_chance_handler.adb} | 6 ++-- ...andler.ads => alr-last_chance_handler.ads} | 2 +- src/alr/alr-main.adb | 3 +- testsuite/drivers/asserts.py | 2 +- testsuite/tests/index/bad-license/test.py | 4 +-- testsuite/tests/index/bad-tag/test.py | 4 +-- testsuite/tests/index/empty-tag/test.py | 3 +- testsuite/tests/index/external-msys2/test.py | 8 ++--- testsuite/tests/index/long-tag/test.py | 4 +-- testsuite/tests/index/maint-bad-email/test.py | 2 +- testsuite/tests/index/maint-bad-login/test.py | 2 +- .../index/too-long-short-description/test.py | 6 ++-- testsuite/tests/misc/bad-tomlfile/test.py | 3 +- testsuite/tests/misc/clean-end/test.py | 2 +- 24 files changed, 117 insertions(+), 43 deletions(-) rename src/alr/{last_chance_handler.adb => alr-last_chance_handler.adb} (64%) rename src/alr/{last_chance_handler.ads => alr-last_chance_handler.ads} (54%) diff --git a/src/alire/alire-errors.adb b/src/alire/alire-errors.adb index 7f520734..38c51f24 100644 --- a/src/alire/alire-errors.adb +++ b/src/alire/alire-errors.adb @@ -114,4 +114,39 @@ package body Alire.Errors is end if; end Get; + ------------------ + -- Pretty_Print -- + ------------------ + + procedure Pretty_Print (Error : String) is + use Utils; + Lines : constant String_Vector := Split (Error, ASCII.LF); + begin + for I in Lines.First_Index .. Lines.Last_Index loop + declare + Line : constant String := Trim (Lines (I)); + begin + Trace.Error ((if I > Lines.First_Index then " " else "") + -- Indentation + + & Line + -- The error proper + + & (if I < Lines.Last_Index + and then Line (Line'Last) /= ':' + then ":" + else "") + -- Trailing ':' except for last line + ); + end; + end loop; + end Pretty_Print; + + ---------- + -- Wrap -- + ---------- + + function Wrap (Upper, Lower : String) return String + is (Upper & ASCII.LF & Lower); + end Alire.Errors; diff --git a/src/alire/alire-errors.ads b/src/alire/alire-errors.ads index d49238db..2bcc5970 100644 --- a/src/alire/alire-errors.ads +++ b/src/alire/alire-errors.ads @@ -51,6 +51,26 @@ package Alire.Errors with Preelaborate is -- Returns the error for Ex if it exists, or defaults to Exception_Message. -- The stored error is cleared. + procedure Pretty_Print (Error : String); + -- Split Error at LFs to prefix each sub-error in a new line with the + -- appropriate tracing prefix. Also, from the second line on, messages are + -- indented. This way, several top-level errors are easier to distinguish. + -- Finally, ':' is appended to every line but the last one if not already + -- in the text. + + function Wrap (Upper, Lower : String) return String; + -- Compose a more general (Upper) error with a more detailed error (Lower). + -- These are later apt for pretty printing. Even if not pretty printed, + -- these merely show in two lines. + + function Set (Wrapper : String; + Wrapped : Ada.Exceptions.Exception_Occurrence) + return String + is (Set (Wrap (Wrapper, Get (Wrapped)))) + with Post => Is_Error_Id (Set'Result); + -- Convenience to concatenate two error messages: a new wrapping text and + -- an existing error within a exception being wrapped. + private Id_Marker : constant String := "alire-stored-error:"; diff --git a/src/alire/alire-lockfiles.adb b/src/alire/alire-lockfiles.adb index cede133c..f76e8d28 100644 --- a/src/alire/alire-lockfiles.adb +++ b/src/alire/alire-lockfiles.adb @@ -3,6 +3,7 @@ with Ada.Text_IO; with Alire.Directories; with Alire.Paths; +with Alire.TOML_Load; with TOML.File_IO; @@ -71,7 +72,7 @@ package body Alire.Lockfiles is (TOML_Adapters.From (Result.Value, Filename & ":"))); end return; else - Raise_Checked_Error (TOML.Format_Error (Result)); + Raise_Checked_Error (TOML_Load.Format_Error (Filename, Result)); end if; end; end Read; diff --git a/src/alire/alire-releases.adb b/src/alire/alire-releases.adb index 729fd1b6..496c5051 100644 --- a/src/alire/alire-releases.adb +++ b/src/alire/alire-releases.adb @@ -640,12 +640,11 @@ package body Alire.Releases is when Manifest.Index => raise Program_Error with Errors.Set - ("Cannot load manifest from index with proper version: " - & Errors.Get (E)); + ("Cannot load manifest from index with proper version: ", E); when Manifest.Local => raise Checked_Error with - Errors.Set ("Cannot load manifest, please review contents: " - & Errors.Get (E)); + Errors.Set + ("Cannot load manifest, please review contents: ", E); end case; end From_TOML; diff --git a/src/alire/alire-root.adb b/src/alire/alire-root.adb index b7fed107..0334ca47 100644 --- a/src/alire/alire-root.adb +++ b/src/alire/alire-root.adb @@ -31,7 +31,8 @@ package body Alire.Root is Log_Exception (E, Debug); return Roots.New_Invalid_Root.With_Reason - ("Failed to load " & File & ": " & Errors.Get (E)); + (Errors.Wrap ("Failed to load " & File, + Errors.Get (E))); end; else return Roots.New_Invalid_Root.With_Reason diff --git a/src/alire/alire-toml_adapters.adb b/src/alire/alire-toml_adapters.adb index b1cdcdcb..8d2b23ea 100644 --- a/src/alire/alire-toml_adapters.adb +++ b/src/alire/alire-toml_adapters.adb @@ -1,5 +1,3 @@ -with Alire.Errors; - package body Alire.TOML_Adapters is ------------ @@ -103,7 +101,7 @@ package body Alire.TOML_Adapters is function Descend (Parent : Key_Queue; Value : TOML.TOML_Value; Context : String) return Key_Queue is - (From (Value, (+Parent.Context) & ": " & Context)); + (From (Value, (+Parent.Context) & ASCII.LF & Context)); --------- -- Pop -- @@ -226,7 +224,8 @@ package body Alire.TOML_Adapters is function Report_Extra_Keys (Queue : Key_Queue) return Outcome is use UStrings; - Message : UString := Queue.Context & ": forbidden extra entries: "; + Message : UString := +Errors.Wrap (+Queue.Context, + "forbidden extra entries: "); Is_First : Boolean := True; Errored : Boolean := False; begin diff --git a/src/alire/alire-toml_adapters.ads b/src/alire/alire-toml_adapters.ads index bc4e169b..a8021cdf 100644 --- a/src/alire/alire-toml_adapters.ads +++ b/src/alire/alire-toml_adapters.ads @@ -1,3 +1,4 @@ +with Alire.Errors; with Alire.Utils; with TOML; use all type TOML.Any_Value_Kind; @@ -174,7 +175,7 @@ private ------------- function Message (Queue : Key_Queue; Message : String) return String is - (+Queue.Context & ": " & Message); + (Errors.Wrap (+Queue.Context, Message)); ------------- -- Descend -- diff --git a/src/alire/alire-toml_load.adb b/src/alire/alire-toml_load.adb index 5a55eb88..905db71d 100644 --- a/src/alire/alire-toml_load.adb +++ b/src/alire/alire-toml_load.adb @@ -1,6 +1,8 @@ +with Alire.Errors; with Alire.Properties.From_TOML; with Alire.TOML_Expressions.Cases; with Alire.TOML_Keys; +with Alire.Utils; with TOML.File_IO; @@ -21,6 +23,17 @@ package body Alire.TOML_Load is (Available => True, Dependencies => False)); + ------------------ + -- Format_Error -- + ------------------ + + function Format_Error (File : Any_Path; + Result : TOML.Read_Result) return String + is ((+Result.Message) & " at " + & File & ":" + & Utils.Trim (Result.Location.Line'Img) & ":" + & Utils.Trim (Result.Location.Column'Img)); + ------------------------ -- Load_Crate_Section -- ------------------------ @@ -111,8 +124,9 @@ package body Alire.TOML_Load is if TOML_Result.Success then return TOML_Result.Value; else - Raise_Checked_Error ("Invalid TOML contents in " & File_Name - & ": " & TOML.Format_Error (TOML_Result)); + Raise_Checked_Error + (Errors.Wrap ("Invalid TOML contents in file", + Format_Error (File_Name, TOML_Result))); end if; end Load_File; diff --git a/src/alire/alire-toml_load.ads b/src/alire/alire-toml_load.ads index b007c64b..2534e613 100644 --- a/src/alire/alire-toml_load.ads +++ b/src/alire/alire-toml_load.ads @@ -10,6 +10,10 @@ package Alire.TOML_Load is -- Separate package to avoid a circularity, since this is used by both -- Crates and Releases. + function Format_Error (File : Any_Path; + Result : TOML.Read_Result) return String with + Pre => not Result.Success; + function Load_File (File_Name : Any_Path) return TOML.TOML_Value; -- Will raise Checked_Error if file contents aren't valid TOML diff --git a/src/alr/alr-commands.adb b/src/alr/alr-commands.adb index 9dc9651a..ff57f16a 100644 --- a/src/alr/alr-commands.adb +++ b/src/alr/alr-commands.adb @@ -381,7 +381,7 @@ package body Alr.Commands is procedure Reportaise_Command_Failed (Message : String) is begin - Trace.Error (Message); + Alire.Errors.Pretty_Print (Message); raise Command_Failed with Alire.Errors.Set (Message); end Reportaise_Command_Failed; @@ -391,7 +391,7 @@ package body Alr.Commands is procedure Reportaise_Wrong_Arguments (Message : String) is begin - Trace.Error (Message); + Alire.Errors.Pretty_Print (Message); raise Wrong_Command_Arguments with Alire.Errors.Set (Message); end Reportaise_Wrong_Arguments; @@ -418,7 +418,8 @@ package body Alr.Commands is begin if not Checked.Is_Valid then Reportaise_Command_Failed - ("Cannot continue with invalid session: " & Checked.Invalid_Reason); + (Errors.Wrap ("Cannot continue with invalid session", + Checked.Invalid_Reason)); end if; -- For workspaces created pre-lockfiles, or with older format, recreate: @@ -701,7 +702,7 @@ package body Alr.Commands is Log ("alr " & What_Command & " done", Detail); exception when E : Alire.Checked_Error => - Trace.Error (Alire.Errors.Get (E, Clear => False)); + Alire.Errors.Pretty_Print (Alire.Errors.Get (E, Clear => False)); if Alire.Log_Level = Debug then raise; else diff --git a/src/alr/last_chance_handler.adb b/src/alr/alr-last_chance_handler.adb similarity index 64% rename from src/alr/last_chance_handler.adb rename to src/alr/alr-last_chance_handler.adb index f8e39728..510a95ab 100644 --- a/src/alr/last_chance_handler.adb +++ b/src/alr/alr-last_chance_handler.adb @@ -3,12 +3,12 @@ with Alire.Errors; with Alr; with Alr.OS_Lib; -procedure Last_Chance_Handler (E : Ada.Exceptions.Exception_Occurrence) is +procedure Alr.Last_Chance_Handler (E : Ada.Exceptions.Exception_Occurrence) is begin -- Ensure we do not show an exception trace to unsuspecting users Alire.Log_Exception (E); - Alr.Trace.Error (Alire.Errors.Get (E)); + Alire.Errors.Pretty_Print (Alire.Errors.Get (E)); Alr.Trace.Error ("alr encountered an unexpected error," & " re-run with -d for details."); Alr.OS_Lib.Bailout (1); -end Last_Chance_Handler; +end Alr.Last_Chance_Handler; diff --git a/src/alr/last_chance_handler.ads b/src/alr/alr-last_chance_handler.ads similarity index 54% rename from src/alr/last_chance_handler.ads rename to src/alr/alr-last_chance_handler.ads index eeb015c6..403f513b 100644 --- a/src/alr/last_chance_handler.ads +++ b/src/alr/alr-last_chance_handler.ads @@ -1,4 +1,4 @@ with Ada.Exceptions; -procedure Last_Chance_Handler (E : Ada.Exceptions.Exception_Occurrence); +procedure Alr.Last_Chance_Handler (E : Ada.Exceptions.Exception_Occurrence); pragma Export (C, Last_Chance_Handler, "__gnat_last_chance_handler"); diff --git a/src/alr/alr-main.adb b/src/alr/alr-main.adb index 962f60aa..5ad7944c 100644 --- a/src/alr/alr-main.adb +++ b/src/alr/alr-main.adb @@ -4,11 +4,10 @@ with Alire.Root; with Alr.Bootstrap; with Alr.Commands; +with Alr.Last_Chance_Handler; with Alr.Platform.Init; with Alr.Platforms.Current; -with Last_Chance_Handler; - procedure Alr.Main is begin Alr.Platform.Init (Alr.Platforms.Current.New_Platform); diff --git a/testsuite/drivers/asserts.py b/testsuite/drivers/asserts.py index 47219b59..7db3ebea 100644 --- a/testsuite/drivers/asserts.py +++ b/testsuite/drivers/asserts.py @@ -23,7 +23,7 @@ def assert_eq(expected, actual, label=None): assert False, '\n'.join(text) -def assert_match(expected_re, actual, label=None, flags=0): +def assert_match(expected_re, actual, label=None, flags=re.S): if not re.match(expected_re, actual, flags=flags): text = ['Unexpected {}'.format(label or 'output'), 'Expecting a match on:', diff --git a/testsuite/tests/index/bad-license/test.py b/testsuite/tests/index/bad-license/test.py index 6739625c..e4a65731 100644 --- a/testsuite/tests/index/bad-license/test.py +++ b/testsuite/tests/index/bad-license/test.py @@ -8,8 +8,8 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) -assert_match('ERROR:.*Loading .*hello_world-0.1.0.toml: ' - 'licenses: unknown license: \'Invalid license ID\'\n', +assert_match('.*Loading .*hello_world-0.1.0.toml:' + '.*licenses:.*unknown license: \'Invalid license ID\'\n', p.out) print('SUCCESS') diff --git a/testsuite/tests/index/bad-tag/test.py b/testsuite/tests/index/bad-tag/test.py index 4e3dc743..6afe8faf 100644 --- a/testsuite/tests/index/bad-tag/test.py +++ b/testsuite/tests/index/bad-tag/test.py @@ -9,8 +9,8 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) assert_match( - 'ERROR:.*Loading .*hello_world-0.1.0.toml: tags: ' - 'Tag string is not valid\n', + '.*Loading .*hello_world-0.1.0.toml:.*tags:' + '.*Tag string is not valid\n', p.out) print('SUCCESS') diff --git a/testsuite/tests/index/empty-tag/test.py b/testsuite/tests/index/empty-tag/test.py index 41df2623..d1f0b55c 100644 --- a/testsuite/tests/index/empty-tag/test.py +++ b/testsuite/tests/index/empty-tag/test.py @@ -9,8 +9,7 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) assert_match( - 'ERROR:.*Loading .*hello_world-0.1.0.toml: tags: ' - 'Tag string is empty\n', + '.*Loading .*hello_world-0.1.0.toml:.*tags:.*Tag string is empty\n', p.out) print('SUCCESS') diff --git a/testsuite/tests/index/external-msys2/test.py b/testsuite/tests/index/external-msys2/test.py index 3e9aa3cc..0083910d 100644 --- a/testsuite/tests/index/external-msys2/test.py +++ b/testsuite/tests/index/external-msys2/test.py @@ -8,15 +8,15 @@ import platform if platform.system() == 'Windows': # Should silently retrieve everything - p = run_alr('--non-interactive', '-v', 'get', 'main', quiet=False, debug=True) - + p = run_alr('--non-interactive', '-v', 'get', 'main', + quiet=False, debug=True) + checks = 0 for line in p.out.splitlines(): if line.startswith("cdialog (ComeOn Dialog!) version "): print("dialog output matched") checks += 1 - + assert checks == 1, 'Only %d match in the output : "%s"' % (checks, p.out) print('SUCCESS') - \ No newline at end of file diff --git a/testsuite/tests/index/long-tag/test.py b/testsuite/tests/index/long-tag/test.py index 8c0742f9..be7e9b81 100644 --- a/testsuite/tests/index/long-tag/test.py +++ b/testsuite/tests/index/long-tag/test.py @@ -8,8 +8,8 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) -assert_match('ERROR:.*Loading .*hello_world-0.1.0.toml:' - ' tags: Tag string is too long ' +assert_match('.*Loading .*hello_world-0.1.0.toml:' + '.*tags:.*Tag string is too long ' '\(must be no more than [0-9]+\)\n', p.out) diff --git a/testsuite/tests/index/maint-bad-email/test.py b/testsuite/tests/index/maint-bad-email/test.py index 2cd130df..50e5da1b 100644 --- a/testsuite/tests/index/maint-bad-email/test.py +++ b/testsuite/tests/index/maint-bad-email/test.py @@ -9,7 +9,7 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) assert_match( - 'ERROR:.*Loading .*hello_world-0.1.0.toml: maintainers: ' + '.*Loading .*hello_world-0.1.0.toml:.*maintainers:.*' 'Maintainers must have a valid email, but got: Mr. User\n', p.out) diff --git a/testsuite/tests/index/maint-bad-login/test.py b/testsuite/tests/index/maint-bad-login/test.py index 3f790d54..2e37dd91 100644 --- a/testsuite/tests/index/maint-bad-login/test.py +++ b/testsuite/tests/index/maint-bad-login/test.py @@ -9,7 +9,7 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) assert_match( - 'ERROR:.*Loading .*hello_world-0.1.0.toml: maintainers-logins: ' + '.*Loading .*hello_world-0.1.0.toml:.*maintainers-logins:.*' 'maintainers-logins must be a valid GitHub login, but got: mr.user\n', p.out) diff --git a/testsuite/tests/index/too-long-short-description/test.py b/testsuite/tests/index/too-long-short-description/test.py index e4ca2180..891e5e8c 100644 --- a/testsuite/tests/index/too-long-short-description/test.py +++ b/testsuite/tests/index/too-long-short-description/test.py @@ -8,10 +8,10 @@ from drivers.asserts import assert_match p = run_alr('show', 'hello_world', complain_on_error=False, debug=False, quiet=True) -assert_match('ERROR:.*Loading .*hello_world-0.1.0.toml:' - ' description: Description string is too long' +assert_match('.*Loading .*hello_world-0.1.0.toml:' + '.*description:.*Description string is too long' ' \(must be no more than [0-9]+\)\n' - 'ERROR: alr encountered an unexpected error,' + '.*alr encountered an unexpected error,' ' re-run with -d for details.', p.out) diff --git a/testsuite/tests/misc/bad-tomlfile/test.py b/testsuite/tests/misc/bad-tomlfile/test.py index 3e6dd3fb..e6feb7aa 100644 --- a/testsuite/tests/misc/bad-tomlfile/test.py +++ b/testsuite/tests/misc/bad-tomlfile/test.py @@ -20,7 +20,8 @@ with open("alire/xxx.toml", "a") as myfile: # Verify that the expected error is given p = run_alr('show', complain_on_error=False) -assert_match('.*Cannot continue with invalid session: Failed to load.*', +assert_match('.*Cannot continue with invalid session.*' + 'Failed to load.*', p.out, flags=re.S) print('SUCCESS') diff --git a/testsuite/tests/misc/clean-end/test.py b/testsuite/tests/misc/clean-end/test.py index afbfc893..4b9052fc 100644 --- a/testsuite/tests/misc/clean-end/test.py +++ b/testsuite/tests/misc/clean-end/test.py @@ -11,7 +11,7 @@ from drivers.asserts import assert_eq, assert_match # Check a few commands for unexpected output # Commands that require session -assert_match(".*Cannot continue with invalid session:" # skip logging prefix +assert_match(".*Cannot continue with invalid session:.*" # skip logging prefix " Could not detect a session folder" " at current or parent locations\n", run_alr('with', quiet=False, complain_on_error=False).out) -- 2.39.5