From 04f88e37190b5374a14646c28d5ec0eb9121ac24 Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Thu, 20 Mar 2025 12:09:37 +0100 Subject: [PATCH] feat: `--builtin` safeguard for `alr settings` (#1912) * Add --builtin to `alr settings` * Test for new switch * Documentation * Apply also to --unset * Fix user changes --- BREAKING.md | 1 + doc/user-changes.md | 13 +++ src/alr/alr-commands-settings.adb | 69 ++++++++++--- src/alr/alr-commands-settings.ads | 7 +- .../tests/settings/builtin-check/test.py | 98 +++++++++++++++++++ .../tests/settings/builtin-check/test.yaml | 3 + 6 files changed, 173 insertions(+), 18 deletions(-) create mode 100644 testsuite/tests/settings/builtin-check/test.py create mode 100644 testsuite/tests/settings/builtin-check/test.yaml diff --git a/BREAKING.md b/BREAKING.md index 13e89e4a..fee6ae08 100644 --- a/BREAKING.md +++ b/BREAKING.md @@ -9,6 +9,7 @@ may change. - alr: added new behavior to `alr test` and a built-in test runner. - manifest: added `[test]` section. - manifest: array of licenses is no longer supported (SPDX expressions allow multiple licenses). +- alr: setting a builtin without `--builtin` will emit a new warning. ### We are here diff --git a/doc/user-changes.md b/doc/user-changes.md index fd26a630..44a7b8ce 100644 --- a/doc/user-changes.md +++ b/doc/user-changes.md @@ -6,6 +6,19 @@ stay on top of `alr` new features. ## Release `3.0` +### New `--builtin` switch for `alr settings` + +PR [#1912](https://github.com/alire-project/alire/pull/1912) + +A new `--builtin` switch has been added to the `alr settings` command to avoid +unintended silent errors. This switch can only be used with `--set`, `--get`, +or `--unset` and ensures that the setting being operated on is a built-in +setting. + +When `--builtin` is used with a non-built-in setting, an error is raised. +Conversely, when operating on a built-in setting without using `--builtin`, a +warning is printed suggesting the use of `--builtin`. + ### New test command PR [#1874](https://github.com/alire-project/alire/pull/1874) diff --git a/src/alr/alr-commands-settings.adb b/src/alr/alr-commands-settings.adb index bdd2581b..535161f4 100644 --- a/src/alr/alr-commands-settings.adb +++ b/src/alr/alr-commands-settings.adb @@ -1,4 +1,3 @@ -with Alire.Settings; with Alire.Settings.Edit; with Alire.Root; @@ -20,6 +19,25 @@ package body Alr.Commands.Settings is Lvl : constant Alire.Settings.Level := (if Cmd.Global then Alire.Settings.Global else Alire.Settings.Local); + + ------------------- + -- Check_Builtin -- + ------------------- + + procedure Check_Builtin (Key : String) is + begin + -- Check if the setting is a builtin when --builtin is given + if Cmd.Builtin and then not Alire.Settings.Is_Builtin (Key) then + Reportaise_Wrong_Arguments + ("'" & Key & "' is not a built-in setting"); + end if; + + -- Check if it is not a builtin when --builtin is not given + if not Cmd.Builtin and then Alire.Settings.Is_Builtin (Key) then + Trace.Warning ("'" & Key & "' is a built-in setting. " + & "Use --builtin to avoid this warning"); + end if; + end Check_Builtin; begin Cmd.Forbids_Structured_Output; @@ -44,6 +62,12 @@ package body Alr.Commands.Settings is ("--show-origin only valid with --list"); end if; + if Cmd.Builtin and then not (Cmd.Get or else Cmd.Set or else Cmd.Unset) + then + Reportaise_Wrong_Arguments + ("--builtin only valid with --get, --set or --unset"); + end if; + if Cmd.Builtins_Doc then Alire.Settings.Edit.Print_Builtins_Doc; return; @@ -76,22 +100,28 @@ package body Alr.Commands.Settings is elsif Cmd.Get then if Args.Count /= 1 then - Reportaise_Wrong_Arguments ("Unset expects one argument"); + Reportaise_Wrong_Arguments ("Get expects one argument"); end if; - if not CLIC.Config.Is_Valid_Config_Key (Args.First_Element) then - Reportaise_Wrong_Arguments ("Invalid setting key '" & - Args.First_Element & "'"); - end if; + declare + Key : constant String := Args.First_Element; + begin + if not CLIC.Config.Is_Valid_Config_Key (Key) then + Reportaise_Wrong_Arguments ("Invalid setting key '" & + Key & "'"); + end if; - if Alire.Settings.DB.Defined (Args.First_Element) then - Trace.Always - (Alire.Settings.DB.Get_As_String (Args.First_Element)); - else - Reportaise_Command_Failed ("Setting key '" & - Args.First_Element & + Check_Builtin (Key); + + if Alire.Settings.DB.Defined (Key) then + Trace.Always + (Alire.Settings.DB.Get_As_String (Key)); + else + Reportaise_Command_Failed ("Setting key '" & + Key & "' is not defined"); - end if; + end if; + end; elsif Cmd.Set then if Args.Count /= 2 then Reportaise_Wrong_Arguments ("Set expects two arguments"); @@ -107,6 +137,8 @@ package body Alr.Commands.Settings is Key & "'"); end if; + Check_Builtin (Key); + -- Check explicitly for booleans to store the proper TOML type -- regardless of the capitalization used by the user. if Is_Boolean (Val) then @@ -135,6 +167,8 @@ package body Alr.Commands.Settings is Key & "'"); end if; + Check_Builtin (Key); + if not CLIC.Config.Edit.Unset (Alire.Settings.Edit.Filepath (Lvl), Key) then @@ -229,8 +263,13 @@ package body Alr.Commands.Settings is (Config => Config, Output => Cmd.Builtins_Doc'Access, Long_Switch => "--builtins-doc", - Help => - "Print Markdown list of built-in settings"); + Help => "Print Markdown list of built-in settings"); + + Define_Switch + (Config => Config, + Output => Cmd.Builtin'Access, + Long_Switch => "--builtin", + Help => "Operate on built-in settings only"); end Setup_Switches; diff --git a/src/alr/alr-commands-settings.ads b/src/alr/alr-commands-settings.ads index b42a7e19..b65536bd 100644 --- a/src/alr/alr-commands-settings.ads +++ b/src/alr/alr-commands-settings.ads @@ -30,9 +30,9 @@ package Alr.Commands.Settings is overriding function Usage_Custom_Parameters (Cmd : Command) return String is ("[--list] [--show-origin] [key_regex] |" & - " --get |" & - " --set |" & - " --unset "); + " --get [--builtin] |" & + " --set [--builtin] |" & + " --unset [--builtin] "); private @@ -44,6 +44,7 @@ private Unset : aliased Boolean := False; Global : aliased Boolean := False; Builtins_Doc : aliased Boolean := False; + Builtin : aliased Boolean := False; end record; end Alr.Commands.Settings; diff --git a/testsuite/tests/settings/builtin-check/test.py b/testsuite/tests/settings/builtin-check/test.py new file mode 100644 index 00000000..1d6a14ac --- /dev/null +++ b/testsuite/tests/settings/builtin-check/test.py @@ -0,0 +1,98 @@ +""" +Verify the two behaviors of --builtin in `alr settings`: error when setting a +non-builtin, warning when setting a builtin. This is kind of like `overriding` +for Ada. +""" + +from drivers.alr import run_alr +from drivers.asserts import assert_substring, assert_not_substring + +WARNING_PREFIX = "Warning:" + +def test_builtin_behavior(key, use_builtin, expected_message): + """ + key: The setting key to test + use_builtin: Whether to use the --builtin switch + expected_message: The message to look for in the output + """ + # The setting value is not important, only its key + test_value = '"Test Value"' + + # We use user.name as the builtin setting for testing. We use quiet=False + # because we don't want to suppress the warning. + + is_error_case = use_builtin and not "user." in key + is_warning_case = not use_builtin and "user." in key + + # Test with --set first (so it is set for --get later) + set_args = ['settings', '--global'] + if use_builtin: + set_args.append('--builtin') + set_args.extend(['--set', key, test_value]) + + p = run_alr(*set_args, complain_on_error=not is_error_case, quiet=False) + if expected_message: + assert_substring(expected_message, p.out) + # Double-check warning format for when we check no warnings + if is_warning_case: + assert_substring(WARNING_PREFIX, p.out) + else: + # If expected_message is empty, check that there's no warning + assert_not_substring(WARNING_PREFIX, p.out) + + # Test with --get + get_args = ['settings', '--global'] + if use_builtin: + get_args.append('--builtin') + get_args.extend(['--get', key]) + + p = run_alr(*get_args, complain_on_error=not is_error_case, quiet=False) + if expected_message: + assert_substring(expected_message, p.out) + else: + # If expected_message is empty, check that there's no warning + assert_not_substring(WARNING_PREFIX, p.out) + + # Test with --unset + unset_args = ['settings', '--global'] + if use_builtin: + unset_args.append('--builtin') + unset_args.extend(['--unset', key]) + + p = run_alr(*unset_args, complain_on_error=not is_error_case, quiet=False) + if expected_message: + assert_substring(expected_message, p.out) + else: + # If expected_message is empty, check that there's no warning + assert_not_substring(WARNING_PREFIX, p.out) + + +# Using --builtin with a non-builtin setting (should fail) +test_builtin_behavior( + key='test.nonbuiltin', + use_builtin=True, + expected_message="is not a built-in setting" +) + +# Using a builtin setting without --builtin (should warn) +test_builtin_behavior( + key='user.name', + use_builtin=False, + expected_message="is a built-in setting" +) + +# Using a builtin setting with --builtin (should not warn) +test_builtin_behavior( + key='user.name', + use_builtin=True, + expected_message="" # No warning expected +) + +# Using a non-builtin setting without --builtin (should not warn) +test_builtin_behavior( + key='test.nonbuiltin', + use_builtin=False, + expected_message="" # No warning expected +) + +print("SUCCESS") diff --git a/testsuite/tests/settings/builtin-check/test.yaml b/testsuite/tests/settings/builtin-check/test.yaml new file mode 100644 index 00000000..fa855459 --- /dev/null +++ b/testsuite/tests/settings/builtin-check/test.yaml @@ -0,0 +1,3 @@ +driver: python-script +indexes: + compiler_only_index: {} -- 2.39.5