From 19933e4b27bcc8504be19549f6100f3f7b337cc3 Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Fri, 14 Aug 2020 17:53:52 +0200 Subject: [PATCH] Use manifest provenance (local/index) for finer diagnostics (#485) * Use manifest source (local, index) for finer errors * Generate `metadata-version` during release saving * Script to prepend manifest metadata versions Not really necessary since we don't expect index manifests to include such metadata version. Still, it makes clear the change. * Testsuite adjustments for extra error output Since we now take advantage of the manifest provenance (index/local), some error messages require tweaking. * Review: remove `metadata-version` field --- scripts/python/manifest-version.py | 27 +++++++++++ src/alire/alire-dependencies-states.adb | 7 +-- src/alire/alire-manifest.adb | 8 ++-- src/alire/alire-manifest.ads | 6 ++- src/alire/alire-releases.adb | 45 ++++++++++++------- src/alire/alire-releases.ads | 14 ++++-- src/alire/alire-root.adb | 7 +-- src/alire/alire-roots.adb | 3 +- src/alire/alire-toml_index.adb | 6 ++- src/alr/alr-commands.adb | 4 +- .../manifest/version-mismatch/test.py | 34 ++++++++++++++ .../manifest/version-mismatch/test.yaml | 1 + .../disabled/manifest/version-missing/test.py | 32 +++++++++++++ .../manifest/version-missing/test.yaml | 1 + .../tests/index/bad-action-command/test.py | 2 +- testsuite/tests/index/bad-license/test.py | 2 +- testsuite/tests/index/bad-tag/test.py | 2 +- testsuite/tests/index/empty-tag/test.py | 2 +- testsuite/tests/index/long-tag/test.py | 2 +- testsuite/tests/index/maint-bad-email/test.py | 2 +- testsuite/tests/index/maint-bad-login/test.py | 2 +- .../index/origin-filesystem-bad-path/test.py | 4 +- .../index/too-long-short-description/test.py | 6 ++- testsuite/tests/misc/bad-tomlfile/test.py | 8 ++-- 24 files changed, 175 insertions(+), 52 deletions(-) create mode 100755 scripts/python/manifest-version.py create mode 100644 testsuite/disabled/manifest/version-mismatch/test.py create mode 100644 testsuite/disabled/manifest/version-mismatch/test.yaml create mode 100644 testsuite/disabled/manifest/version-missing/test.py create mode 100644 testsuite/disabled/manifest/version-missing/test.yaml diff --git a/scripts/python/manifest-version.py b/scripts/python/manifest-version.py new file mode 100755 index 00000000..b3cb0713 --- /dev/null +++ b/scripts/python/manifest-version.py @@ -0,0 +1,27 @@ +#!/usr/bin/env python3 +# Find all crates in new index format and move origin-related fields to their own table +# +# The update is done in place and for all indexes found under the current directory. + +import alire.index +import os +import rtoml + +from alire import index, utils +from pathlib import Path + +FROM_VERSION = "0.2" +INTO_VERSION = "0.3" + + +def fix_manifest(file): + # file is the absolute path to a manifest + print(f" Patching {file}...") + with open(file, "r") as orig: + data = orig.read() + with open(file, "w") as updated: + updated.write(f'metadata-version = "{INTO_VERSION}"\n') + updated.write(data) + + +alire.index.migrate_indexes(os.getcwd(), FROM_VERSION, INTO_VERSION, fix_manifest) diff --git a/src/alire/alire-dependencies-states.adb b/src/alire/alire-dependencies-states.adb index d023eed4..67e97921 100644 --- a/src/alire/alire-dependencies-states.adb +++ b/src/alire/alire-dependencies-states.adb @@ -1,6 +1,6 @@ -with Alire.Roots; - with Alire.Crates; +with Alire.Manifest; +with Alire.Roots; package body Alire.Dependencies.States is @@ -92,7 +92,8 @@ package body Alire.Dependencies.States is (Releases.From_TOML (From.Descend (From.Checked_Pop (Keys.Release, TOML_Table), - "release: " & (+Crate)))); + "release: " & (+Crate)), + Manifest.Index)); end case; return Data; diff --git a/src/alire/alire-manifest.adb b/src/alire/alire-manifest.adb index 50817a07..1bc05925 100644 --- a/src/alire/alire-manifest.adb +++ b/src/alire/alire-manifest.adb @@ -35,7 +35,7 @@ package body Alire.Manifest is Close (File); -- Attempt loading of the new file as a double check - if not Is_Valid (Replacer.Editable_Name) then + if not Is_Valid (Replacer.Editable_Name, Local) then raise Program_Error with Errors.Set ("Addition of dependencies to manifest failed"); end if; @@ -57,10 +57,10 @@ package body Alire.Manifest is -- Is_Valid -- -------------- - function Is_Valid (Name : Any_Path) return Boolean is + function Is_Valid (Name : Any_Path; Source : Sources) return Boolean is begin -- Check we are able to load the manifest file - if Releases.From_Manifest (Name).Version_Image /= "" then + if Releases.From_Manifest (Name, Source).Version_Image /= "" then Trace.Debug ("Checked valid manifest at " & Name); return True; else @@ -254,7 +254,7 @@ package body Alire.Manifest is -- Attempt loading of the new file as a double check. This should never -- fail because we won't touch anything that's clearly removable. - if not Is_Valid (Replacer.Editable_Name) then + if not Is_Valid (Replacer.Editable_Name, Local) then raise Program_Error with Errors.Set ("Removal of dependencies in manifest failed"); end if; diff --git a/src/alire/alire-manifest.ads b/src/alire/alire-manifest.ads index 409adbea..f127e8cd 100644 --- a/src/alire/alire-manifest.ads +++ b/src/alire/alire-manifest.ads @@ -2,6 +2,10 @@ with Alire.Dependencies.Containers; package Alire.Manifest is + type Sources is (Index, Local); + -- We may have slightly different mandatory fields for a manifest that is + -- coming from an index or from a local crate (initialized, pinned...) + -- Subprograms for manipulation of the manifest file procedure Append (Name : Any_Path; @@ -13,7 +17,7 @@ package Alire.Manifest is -- dependency with enough guarantees of not botching the manifest, -- no change will be applied and Checked_Error will be raised. - function Is_Valid (Name : Any_Path) return Boolean; + function Is_Valid (Name : Any_Path; Source : Sources) return Boolean; -- Check that the given Name is a loadable manifest end Alire.Manifest; diff --git a/src/alire/alire-releases.adb b/src/alire/alire-releases.adb index 6d19065d..729fd1b6 100644 --- a/src/alire/alire-releases.adb +++ b/src/alire/alire-releases.adb @@ -2,6 +2,7 @@ with Ada.Strings.Fixed; with Alire.Crates; with Alire.Defaults; +with Alire.Errors; with Alire.Requisites.Booleans; with Alire.TOML_Load; with Alire.Utils.YAML; @@ -571,25 +572,30 @@ package body Alire.Releases is -- From_Manifest -- ------------------- - function From_Manifest (File_Name : Any_Path) return Release + function From_Manifest (File_Name : Any_Path; + Source : Manifest.Sources) + return Release is (From_TOML (TOML_Adapters.From (TOML_Load.Load_File (File_Name), - "Loading release from manifest: " & File_Name))); + "Loading release from manifest: " & File_Name), + Source)); --------------- -- From_TOML -- --------------- - function From_TOML (From : TOML_Adapters.Key_Queue) return Release is + function From_TOML (From : TOML_Adapters.Key_Queue; + Source : Manifest.Sources) + return Release is begin - From.Assert_Key (TOML_Keys.Name, TOML.TOML_String); + From.Assert_Key (TOML_Keys.Name, TOML.TOML_String); return This : Release := New_Empty_Release - (Name => +From.Unwrap.Get (TOML_Keys.Name).As_String) + (Name => +From.Unwrap.Get (TOML_Keys.Name).As_String) do - Assert (This.From_TOML (From)); + Assert (This.From_TOML (From, Source)); end return; end From_TOML; @@ -597,8 +603,9 @@ package body Alire.Releases is -- From_TOML -- --------------- - function From_TOML (This : in out Release; - From : TOML_Adapters.Key_Queue) + function From_TOML (This : in out Release; + From : TOML_Adapters.Key_Queue; + Source : Manifest.Sources) return Outcome is package Labeled renames Alire.Properties.Labeled; @@ -607,13 +614,7 @@ package body Alire.Releases is -- Origin - declare - Result : constant Outcome := This.Origin.From_TOML (From); - begin - if not Result.Success then - return Result; - end if; - end; + This.Origin.From_TOML (From).Assert; -- Properties @@ -633,6 +634,19 @@ package body Alire.Releases is -- Check for remaining keys, which must be erroneous: return From.Report_Extra_Keys; + exception + when E : others => + case Source is + when Manifest.Index => + raise Program_Error with + Errors.Set + ("Cannot load manifest from index with proper version: " + & Errors.Get (E)); + when Manifest.Local => + raise Checked_Error with + Errors.Set ("Cannot load manifest, please review contents: " + & Errors.Get (E)); + end case; end From_TOML; ------------------- @@ -657,6 +671,7 @@ package body Alire.Releases is use TOML_Adapters; Root : constant TOML.TOML_Value := R.Properties.To_TOML; begin + -- Name Root.Set (TOML_Keys.Name, +R.Name_Str); diff --git a/src/alire/alire-releases.ads b/src/alire/alire-releases.ads index 284318fa..1f0846d4 100644 --- a/src/alire/alire-releases.ads +++ b/src/alire/alire-releases.ads @@ -4,6 +4,7 @@ with Ada.Tags; with Alire.Conditional; with Alire.Dependencies; with Alire.Interfaces; +with Alire.Manifest; with Alire.Milestones; with Alire.Origins; with Alire.Properties.Actions; @@ -272,9 +273,13 @@ package Alire.Releases is -- Return the dependency that represents this very release (crate=version), -- wrapped as a dependency tree with a single value. - function From_Manifest (File_Name : Any_Path) return Release; + function From_Manifest (File_Name : Any_Path; + Source : Manifest.Sources) + return Release; - function From_TOML (From : TOML_Adapters.Key_Queue) return Release; + function From_TOML (From : TOML_Adapters.Key_Queue; + Source : Manifest.Sources) + return Release; -- Load a release from a TOML table overriding @@ -313,8 +318,9 @@ private Available : Requisites.Tree; end record; - function From_TOML (This : in out Release; - From : TOML_Adapters.Key_Queue) + function From_TOML (This : in out Release; + From : TOML_Adapters.Key_Queue; + Source : Manifest.Sources) return Outcome; -- Fill in an already existing release diff --git a/src/alire/alire-root.adb b/src/alire/alire-root.adb index 28465029..b7fed107 100644 --- a/src/alire/alire-root.adb +++ b/src/alire/alire-root.adb @@ -1,5 +1,6 @@ with Alire.Directories; with Alire.Errors; +with Alire.Manifest; with Alire.Paths; with Alire.Releases; @@ -21,7 +22,7 @@ package body Alire.Root is Extension => Paths.Crate_File_Extension_With_Dot); begin return Roots.New_Root - (Releases.From_Manifest (File), + (Releases.From_Manifest (File, Manifest.Local), Path, Platform_Properties); exception @@ -29,10 +30,6 @@ package body Alire.Root is Trace.Debug ("Exception while loading crate file is:"); Log_Exception (E, Debug); - Trace.Warning ("Could not load crate information from " & File); - Trace.Warning ("If this workspace was created with a previous" - & " alr version you may need to recreate it."); - return Roots.New_Invalid_Root.With_Reason ("Failed to load " & File & ": " & Errors.Get (E)); end; diff --git a/src/alire/alire-roots.adb b/src/alire/alire-roots.adb index 24c1efb6..a8cd7b6a 100644 --- a/src/alire/alire-roots.adb +++ b/src/alire/alire-roots.adb @@ -4,6 +4,7 @@ with Ada.Directories; with Alire.Directories; with Alire.Environment; with Alire.Lockfiles; +with Alire.Manifest; with Alire.OS_Lib; with Alire.Paths; with Alire.Root; @@ -47,7 +48,7 @@ package body Alire.Roots is if Crate_File /= "" then declare Release : constant Releases.Release := - Releases.From_Manifest (Crate_File); + Releases.From_Manifest (Crate_File, Manifest.Local); begin -- Crate loaded properly, we can return a valid root here Trace.Debug ("Valid root found at " & Path); diff --git a/src/alire/alire-toml_index.adb b/src/alire/alire-toml_index.adb index bcb3c62b..332beff9 100644 --- a/src/alire/alire-toml_index.adb +++ b/src/alire/alire-toml_index.adb @@ -10,6 +10,7 @@ with Alire.Hashes.SHA512_Impl; pragma Unreferenced (Alire.Hashes.SHA512_Impl); -- index, this seems a decent place to force inclusion in the build closure. with Alire.Index; +with Alire.Manifest; with Alire.Origins.Deployers.Filesystem; with Alire.Origins.Tweaks; with Alire.TOML_Keys; @@ -395,7 +396,8 @@ package body Alire.TOML_Index is (File_Name, Releases.From_TOML (TOML_Adapters.From (Value, - Context => "Loading release from " & File_Name))); + Context => "Loading release from " & File_Name), + Manifest.Index)); end if; end Load_From_Catalog_Internal; @@ -424,7 +426,7 @@ package body Alire.TOML_Index is if not Origins.Deployers.Filesystem.Is_Valid_Local_Crate (VFS.Create (+Origin.Path)) then - raise Checked_Error with + raise Constraint_Error with -- not an expected error in an index ("Local origin path is not a valid directory: " & Origin.Path); end if; diff --git a/src/alr/alr-commands.adb b/src/alr/alr-commands.adb index ba6785b5..9dc9651a 100644 --- a/src/alr/alr-commands.adb +++ b/src/alr/alr-commands.adb @@ -382,7 +382,7 @@ package body Alr.Commands is procedure Reportaise_Command_Failed (Message : String) is begin Trace.Error (Message); - raise Command_Failed with Message; + raise Command_Failed with Alire.Errors.Set (Message); end Reportaise_Command_Failed; -------------------------------- @@ -392,7 +392,7 @@ package body Alr.Commands is procedure Reportaise_Wrong_Arguments (Message : String) is begin Trace.Error (Message); - raise Wrong_Command_Arguments with Message; + raise Wrong_Command_Arguments with Alire.Errors.Set (Message); end Reportaise_Wrong_Arguments; ------------------------- diff --git a/testsuite/disabled/manifest/version-mismatch/test.py b/testsuite/disabled/manifest/version-mismatch/test.py new file mode 100644 index 00000000..7f11a31b --- /dev/null +++ b/testsuite/disabled/manifest/version-mismatch/test.py @@ -0,0 +1,34 @@ +""" +Check that a local manifest with wrong metadata version is indeed reported +""" + +from drivers.alr import run_alr +from drivers.asserts import assert_match +from os import chdir + +import re + +# Create a regular working release +run_alr("init", "--bin", "xxx") +chdir("xxx") + +# Change the version +with open("alire/xxx.toml", "r") as file: + lines = file.readlines() +with open("alire/xxx.toml", "w") as file: + for line in lines: + if line.startswith("metadata-version"): + file.write("metadata-version = '0.0'\n") + else: + file.write(line) + +# Verify the error +p = run_alr('show', complain_on_error=False) +assert p.status != 0, "command should have errored" +assert_match(".*" + + "Mismatch between manifest version" + + " \(.*\) and alr index version \(.*\)" + + ".*", + p.out, flags=re.S) + +print("SUCCESS") diff --git a/testsuite/disabled/manifest/version-mismatch/test.yaml b/testsuite/disabled/manifest/version-mismatch/test.yaml new file mode 100644 index 00000000..32c747b3 --- /dev/null +++ b/testsuite/disabled/manifest/version-mismatch/test.yaml @@ -0,0 +1 @@ +driver: python-script diff --git a/testsuite/disabled/manifest/version-missing/test.py b/testsuite/disabled/manifest/version-missing/test.py new file mode 100644 index 00000000..b51893a6 --- /dev/null +++ b/testsuite/disabled/manifest/version-missing/test.py @@ -0,0 +1,32 @@ +""" +Check that a local manifest without version is reported as old +""" + +from drivers.alr import run_alr +from drivers.asserts import assert_match +from os import chdir + +import re + +# Create a regular working release +run_alr("init", "--bin", "xxx") +chdir("xxx") + +# Remove the version +with open("alire/xxx.toml", "r") as file: + lines = file.readlines() +with open("alire/xxx.toml", "w") as file: + for line in lines: + if not line.startswith("metadata-version"): + file.write(line) + +# Verify the error +p = run_alr('show', complain_on_error=False) +assert p.status != 0, "command should have errored" +assert_match(".*" + + "Local manifest file is for an old alr version" + + re.escape(" (lacks metadata-version field)") + + ".*", + p.out, flags=re.S) + +print("SUCCESS") diff --git a/testsuite/disabled/manifest/version-missing/test.yaml b/testsuite/disabled/manifest/version-missing/test.yaml new file mode 100644 index 00000000..32c747b3 --- /dev/null +++ b/testsuite/disabled/manifest/version-missing/test.yaml @@ -0,0 +1 @@ +driver: python-script diff --git a/testsuite/tests/index/bad-action-command/test.py b/testsuite/tests/index/bad-action-command/test.py index c1a42421..201fa7b3 100644 --- a/testsuite/tests/index/bad-action-command/test.py +++ b/testsuite/tests/index/bad-action-command/test.py @@ -8,7 +8,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 .* actions command must' +assert_match('ERROR:.*Loading .* actions command must' ' be an array of string\(s\)\n', p.out) diff --git a/testsuite/tests/index/bad-license/test.py b/testsuite/tests/index/bad-license/test.py index 7d1e4bb5..6739625c 100644 --- a/testsuite/tests/index/bad-license/test.py +++ b/testsuite/tests/index/bad-license/test.py @@ -8,7 +8,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: ' +assert_match('ERROR:.*Loading .*hello_world-0.1.0.toml: ' 'licenses: unknown license: \'Invalid license ID\'\n', p.out) diff --git a/testsuite/tests/index/bad-tag/test.py b/testsuite/tests/index/bad-tag/test.py index 856506ad..4e3dc743 100644 --- a/testsuite/tests/index/bad-tag/test.py +++ b/testsuite/tests/index/bad-tag/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: tags: ' + 'ERROR:.*Loading .*hello_world-0.1.0.toml: tags: ' 'Tag string is not valid\n', p.out) diff --git a/testsuite/tests/index/empty-tag/test.py b/testsuite/tests/index/empty-tag/test.py index b73230df..41df2623 100644 --- a/testsuite/tests/index/empty-tag/test.py +++ b/testsuite/tests/index/empty-tag/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: tags: ' + 'ERROR:.*Loading .*hello_world-0.1.0.toml: tags: ' 'Tag string is empty\n', p.out) diff --git a/testsuite/tests/index/long-tag/test.py b/testsuite/tests/index/long-tag/test.py index 1466971b..8c0742f9 100644 --- a/testsuite/tests/index/long-tag/test.py +++ b/testsuite/tests/index/long-tag/test.py @@ -8,7 +8,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:' +assert_match('ERROR:.*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 d3af0ad8..2cd130df 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: ' + 'ERROR:.*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 229bc09b..3f790d54 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: ' + 'ERROR:.*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/origin-filesystem-bad-path/test.py b/testsuite/tests/index/origin-filesystem-bad-path/test.py index 027e54cc..2feda224 100644 --- a/testsuite/tests/index/origin-filesystem-bad-path/test.py +++ b/testsuite/tests/index/origin-filesystem-bad-path/test.py @@ -15,7 +15,9 @@ def run(i, error): config_dir, '.', {'bad_index_{}'.format(i): {'in_fixtures': False}}) p = run_alr('list', complain_on_error=False, debug=False) assert_match( - 'ERROR: {}\n$'.format(error), + 'ERROR: {}\n' + 'ERROR: alr encountered an unexpected error,' + ' re-run with -d for details.\n$'.format(error), 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 2953e40a..e4ca2180 100644 --- a/testsuite/tests/index/too-long-short-description/test.py +++ b/testsuite/tests/index/too-long-short-description/test.py @@ -8,9 +8,11 @@ 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:' +assert_match('ERROR:.*Loading .*hello_world-0.1.0.toml:' ' description: Description string is too long' - ' \(must be no more than [0-9]+\)\n', + ' \(must be no more than [0-9]+\)\n' + 'ERROR: alr encountered an unexpected error,' + ' re-run with -d for details.', p.out) print('SUCCESS') diff --git a/testsuite/tests/misc/bad-tomlfile/test.py b/testsuite/tests/misc/bad-tomlfile/test.py index 96e8e51c..3e6dd3fb 100644 --- a/testsuite/tests/misc/bad-tomlfile/test.py +++ b/testsuite/tests/misc/bad-tomlfile/test.py @@ -17,12 +17,10 @@ os.chdir('xxx') with open("alire/xxx.toml", "a") as myfile: myfile.write("SHOULND'T BE HERE") -# Verify that the expected warning is given -p = run_alr('show', complain_on_error=False, quiet=False) # Let warn through +# Verify that the expected error is given +p = run_alr('show', complain_on_error=False) -assert_match('.*Could not load crate information from.*' - 'If this workspace was created with a' - ' previous alr version you may need to recreate it.*', +assert_match('.*Cannot continue with invalid session: Failed to load.*', p.out, flags=re.S) print('SUCCESS') -- 2.39.5