From ef83f192f1dee973a73fd3fdf35792650386865e Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Mon, 11 Mar 2024 16:41:10 +0100 Subject: [PATCH] Refactor `--config` as `--settings` (#1627) * Refactor `ALR_CONFIG` to `ALIRE_SETTINGS` * Replace --config with --settings * Code review * Post-merge fix --- src/alire/alire-features.ads | 3 +- src/alire/alire-settings-edit-early_load.adb | 2 +- src/alire/alire-settings-edit.adb | 20 ++--- src/alire/alire-settings-edit.ads | 2 +- src/alire/alire_early_elaboration.adb | 74 +++++++++++++++---- src/alr/alr-commands-version.adb | 3 +- src/alr/alr-commands.adb | 16 +++- testsuite/drivers/alr.py | 20 ++--- testsuite/drivers/driver/python_script.py | 2 +- .../tests/config/missing-config-path/test.py | 23 ++++-- .../tests/settings/early-loading/test.py | 4 +- .../settings/relative_config_path/test.py | 2 +- testsuite/tests/toolchain/directories/test.py | 2 +- 13 files changed, 122 insertions(+), 51 deletions(-) diff --git a/src/alire/alire-features.ads b/src/alire/alire-features.ads index 0c97a29e..c55661ba 100644 --- a/src/alire/alire-features.ads +++ b/src/alire/alire-features.ads @@ -10,9 +10,10 @@ package Alire.Features is use type Min_Version; - Env_Alr_Config_Deprecated : constant On_Version := +"3.0"; + Config_Deprecated : constant On_Version := +"1.0"; -- We migrate ALR_CONFIG to ALIRE_SETTINGS_DIR, but allow the use of the -- former with a warning during our next major release to ease transition. + -- Likewise for the -c/--config switch package Index is diff --git a/src/alire/alire-settings-edit-early_load.adb b/src/alire/alire-settings-edit-early_load.adb index de3ed913..5740c75f 100644 --- a/src/alire/alire-settings-edit-early_load.adb +++ b/src/alire/alire-settings-edit-early_load.adb @@ -6,7 +6,7 @@ package body Alire.Settings.Edit.Early_Load is procedure Load_Settings is begin - Alire.Settings.Edit.Load_Config; + Alire.Settings.Edit.Load_Settings; end Load_Settings; begin diff --git a/src/alire/alire-settings-edit.adb b/src/alire/alire-settings-edit.adb index 6ce00c70..37bdc40b 100644 --- a/src/alire/alire-settings-edit.adb +++ b/src/alire/alire-settings-edit.adb @@ -39,7 +39,7 @@ package body Alire.Settings.Edit is end if; -- Reload after change - Load_Config; + Load_Settings; end Set_Locally; ------------------ @@ -56,7 +56,7 @@ package body Alire.Settings.Edit is end if; -- Reload after change - Load_Config; + Load_Settings; end Set_Globally; --------- @@ -86,7 +86,7 @@ package body Alire.Settings.Edit is if CLIC.Config.Edit.Unset (Filepath (Level), Key, Quiet => True) then Trace.Debug ("Config key " & Key & " unset from " & Level'Image & "configuration at " & Filepath (Level)); - Load_Config; + Load_Settings; else Trace.Debug ("Config key " & Key & " requested to be unset at level " & Level'Image & " but it was already unset at " @@ -109,7 +109,7 @@ package body Alire.Settings.Edit is Assert (Set_Boolean_Impl (Filepath (Level), Key, Value, Check), "Cannot set config key '" & Key & "' at level " & Level'Image); -- Reload after change - Load_Config; + Load_Settings; end Set_Boolean; -------------- @@ -180,11 +180,11 @@ package body Alire.Settings.Edit is return Location (Current); end Filepath; - ----------------- - -- Load_Config -- - ----------------- + ------------------- + -- Load_Settings -- + ------------------- - procedure Load_Config is + procedure Load_Settings is begin DB_Instance.Clear; @@ -206,7 +206,7 @@ package body Alire.Settings.Edit is if Platforms.Current.Disable_Distribution_Detection then Trace.Debug ("Distribution detection disabled by configuration"); end if; - end Load_Config; + end Load_Settings; Default_Config_Path : constant Absolute_Path := Platforms.Folders.Config; @@ -224,7 +224,7 @@ package body Alire.Settings.Edit is begin -- Warn or fail depending on version if OS_Lib.Getenv (Environment.Config, Unset) /= Unset then - if Version.Semver.Current < Features.Env_Alr_Config_Deprecated then + if Version.Semver.Current < Features.Config_Deprecated then Warnings.Warn_Once (Msg, Level => Warning); else Raise_Checked_Error (Msg); diff --git a/src/alire/alire-settings-edit.ads b/src/alire/alire-settings-edit.ads index 13959746..a80effcc 100644 --- a/src/alire/alire-settings-edit.ads +++ b/src/alire/alire-settings-edit.ads @@ -82,7 +82,7 @@ package Alire.Settings.Edit is private - procedure Load_Config; + procedure Load_Settings; -- Clear and reload all configuration. Also set some values elsewhere -- used to break circularities. Bottom line, this procedure must leave -- the program-wide configuration ready. This is done during startup from diff --git a/src/alire/alire_early_elaboration.adb b/src/alire/alire_early_elaboration.adb index ae97124d..d819982c 100644 --- a/src/alire/alire_early_elaboration.adb +++ b/src/alire/alire_early_elaboration.adb @@ -2,7 +2,9 @@ with AAA.Strings; with Ada.Directories; +with Alire.Features; with Alire.Settings.Edit.Early_Load; +with Alire.Version.Semver; with GNAT.Command_Line; with GNAT.OS_Lib; @@ -71,26 +73,26 @@ package body Alire_Early_Elaboration is procedure Check_Switches is - ------------------------- - -- Config_Switch_Error -- - ------------------------- + --------------------------- + -- Settings_Switch_Error -- + --------------------------- - procedure Config_Switch_Error (Switch : String) is + procedure Settings_Switch_Error (Switch : String) is begin GNAT.IO.Put_Line ("ERROR: Switch " & Switch & " requires argument (global)."); Early_Error ("try ""alr --help"" for more information."); - end Config_Switch_Error; + end Settings_Switch_Error; --------------------- -- Set_Config_Path -- --------------------- - procedure Set_Config_Path (Path : String) is + procedure Set_Config_Path (Switch, Path : String) is package Adirs renames Ada.Directories; begin if Path = "" then - Config_Switch_Error ("--config"); + Settings_Switch_Error (Switch); elsif not Adirs.Exists (Path) then Early_Error ("Invalid non-existing configuration path: " & Path); @@ -102,6 +104,42 @@ package body Alire_Early_Elaboration is end if; end Set_Config_Path; + ----------------------------- + -- Check_Config_Deprecated -- + ----------------------------- + + use type Alire.Version.Semver.Version; + Config_Deprecated : constant Boolean + := Alire.Version.Semver.Current >= Alire.Features.Config_Deprecated; + + procedure Check_Config_Deprecated is + begin + if Config_Deprecated then + Early_Error + ("--config is deprecated, use --settings instead"); + end if; + end Check_Config_Deprecated; + + ---------------- + -- Check_Seen -- + ---------------- + + Settings_Seen : Boolean := False; + + procedure Check_Settings_Seen is + begin + if Settings_Seen then + Early_Error + ("Only one of -s, --settings" + & (if not Config_Deprecated + then ", -c, --config" + else "") + & " allowed"); + else + Settings_Seen := True; + end if; + end Check_Settings_Seen; + ----------------------- -- Check_Long_Switch -- ----------------------- @@ -116,16 +154,22 @@ package body Alire_Early_Elaboration is Switch_D := True; Add_Scopes (Tail (Switch, "=")); elsif Has_Prefix (Switch, "--config") then - Set_Config_Path (Tail (Switch, "=")); + Check_Config_Deprecated; + Check_Settings_Seen; + Set_Config_Path (Switch, Tail (Switch, "=")); + elsif Has_Prefix (Switch, "--settings") then + Check_Settings_Seen; + Set_Config_Path (Switch, Tail (Switch, "=")); end if; end Check_Long_Switch; Option : Character; + begin loop -- We use the simpler Getopt form to avoid built-in help and other -- shenanigans. - Option := Getopt ("* d? --debug? q v c= --config="); + Option := Getopt ("* d? --debug? q v c= --config= s= --settings="); case Option is when ASCII.NUL => exit; @@ -133,9 +177,13 @@ package body Alire_Early_Elaboration is if not Subcommand_Seen then Check_Long_Switch (Full_Switch); end if; - when 'c' => + when 'c' | 's' => if not Subcommand_Seen then - Set_Config_Path (Parameter); + Check_Settings_Seen; + if Option = 'c' then + Check_Config_Deprecated; + end if; + Set_Config_Path ("-" & Option, Parameter); end if; when 'd' => if not Subcommand_Seen then @@ -164,8 +212,8 @@ package body Alire_Early_Elaboration is end loop; exception when GNAT.Command_Line.Invalid_Parameter => - if Option = 'c' then - Config_Switch_Error ("-c"); + if Option in 'c' | 's' then + Settings_Switch_Error ("-" & Option); end if; when Exit_From_Command_Line => -- Something unexpected happened but it will be properly dealt diff --git a/src/alr/alr-commands-version.adb b/src/alr/alr-commands-version.adb index a0d73117..bd145df5 100644 --- a/src/alr/alr-commands-version.adb +++ b/src/alr/alr-commands-version.adb @@ -72,7 +72,8 @@ package body Alr.Commands.Version is Table.Append ("").New_Row; Table.Append ("CONFIGURATION").New_Row; - Table.Append ("config folder:").Append (Settings.Edit.Path).New_Row; + Table.Append ("settings folder:") + .Append (Alire.Settings.Edit.Path).New_Row; Table.Append ("cache folder:") .Append (Alire.Settings.Edit.Cache_Path).New_Row; Table.Append ("vault folder:").Append (Paths.Vault.Path).New_Row; diff --git a/src/alr/alr-commands.adb b/src/alr/alr-commands.adb index 3aa8afa6..64d9155f 100644 --- a/src/alr/alr-commands.adb +++ b/src/alr/alr-commands.adb @@ -11,6 +11,7 @@ with Alire_Early_Elaboration; with Alire.Settings.Builtins; with Alire.Settings.Edit; with Alire.Errors; +with Alire.Features; with Alire.Index_On_Disk.Loading; with Alire.Index_On_Disk.Updates; with Alire.Lockfiles; @@ -19,6 +20,7 @@ with Alire.Platforms.Current; with Alire.Root; with Alire.Solutions; with Alire.Toolchains; +with Alire.Version.Semver; with Alr.Commands.Action; with Alr.Commands.Build; @@ -137,12 +139,22 @@ package body Alr.Commands is procedure Set_Global_Switches (Config : in out CLIC.Subcommand.Switches_Configuration) is + use Alire; use CLIC.Subcommand; + use type Alire.Version.Semver.Version; begin + if Alire.Version.Semver.Current < Features.Config_Deprecated then + Define_Switch (Config, + Command_Line_Config_Path'Access, + "-c=", "--config=", + TTY.Error ("Deprecated") + & ". See -s/--settings switch"); + end if; + Define_Switch (Config, Command_Line_Config_Path'Access, - "-c=", "--config=", - "Override configuration folder location"); + "-s=", "--settings=", + "Override settings folder location"); Define_Switch (Config, Command_Line_Chdir_Target_Path'Access, diff --git a/testsuite/drivers/alr.py b/testsuite/drivers/alr.py index 23528324..95da9140 100644 --- a/testsuite/drivers/alr.py +++ b/testsuite/drivers/alr.py @@ -44,47 +44,47 @@ def prepare_env(settings_dir, env): settings_dir = os.path.abspath(settings_dir) mkdir(settings_dir) env['ALIRE_SETTINGS_DIR'] = settings_dir - # We pass config location explicitly in the following calls since env is + # We pass settings location explicitly in the following calls since env is # not yet applied (it's just a dict to be passed later to subprocess) if platform.system() == "Windows": # Disable msys inadvertent installation - run_alr("-c", settings_dir, "settings", "--global", + run_alr("-s", settings_dir, "settings", "--global", "--set", "msys2.do_not_install", "true") # And configure the one set up in the environment so it is used by # tests that need it. - run_alr("-c", settings_dir, "settings", "--global", + run_alr("-s", settings_dir, "settings", "--global", "--set", "msys2.install_dir", os.path.join( os.environ.get("LocalAppData"), "alire", "cache", "msys64")) # Disable autoconfig of the community index, to prevent unintended use of # it in tests, besides the overload of fetching it - run_alr("-c", settings_dir, "settings", "--global", + run_alr(f"-s", settings_dir, "settings", "--global", "--set", "index.auto_community", "false") # Disable selection of toolchain to preserve older behavior. Tests that # require a configured compiler will have to set it up explicitly. - run_alr("-c", settings_dir, "toolchain", "--disable-assistant") + run_alr("-s", settings_dir, "toolchain", "--disable-assistant") # Disable warning on old index, to avoid having to update index versions # when they're still compatible. - run_alr("-c", settings_dir, "settings", "--global", + run_alr("-s", settings_dir, "settings", "--global", "--set", "warning.old_index", "false") # Disable shared dependencies (keep old pre-2.0 behavior) not to break lots # of tests. The post-2.0 behavior will have its own tests. - run_alr("-c", settings_dir, "settings", "--global", + run_alr("-s", settings_dir, "settings", "--global", "--set", "dependencies.shared", "false") # Disable index auto-updates, which is not expected by most tests - run_alr("-c", settings_dir, "settings", "--global", + run_alr("-s", settings_dir, "settings", "--global", "--set", "index.auto_update", "0") # If distro detection is disabled via environment, configure so in alr if "ALIRE_TESTSUITE_DISABLE_DISTRO" in env: - run_alr("-c", settings_dir, "settings", "--global", + run_alr("-s", settings_dir, "settings", "--global", "--set", "distribution.disable_detection", "true") @@ -616,4 +616,4 @@ def unselect_compiler(): assistant. """ run_alr("settings", "--global", "--unset", "toolchain.use.gnat") - run_alr("settings", "--global", "--unset", "toolchain.external.gnat") \ No newline at end of file + run_alr("settings", "--global", "--unset", "toolchain.external.gnat") diff --git a/testsuite/drivers/driver/python_script.py b/testsuite/drivers/driver/python_script.py index b08fdc1c..08279299 100644 --- a/testsuite/drivers/driver/python_script.py +++ b/testsuite/drivers/driver/python_script.py @@ -168,7 +168,7 @@ class PythonScriptDriver(BaseDriver): self.result.log.log += "Build mode: SHARED\n" # Activate shared builds. Using "-c" is needed as the environment # still isn't activated at the driver script level. - run_alr("-c", pristine_env["ALIRE_SETTINGS_DIR"], + run_alr(f"--settings={pristine_env['ALIRE_SETTINGS_DIR']}", "settings", "--global", "--set", "dependencies.shared", "true") p = self.run_script(copy.deepcopy(pristine_env)) diff --git a/testsuite/tests/config/missing-config-path/test.py b/testsuite/tests/config/missing-config-path/test.py index 0047db81..e9652409 100644 --- a/testsuite/tests/config/missing-config-path/test.py +++ b/testsuite/tests/config/missing-config-path/test.py @@ -1,18 +1,27 @@ """ -Verify that errors are properly handled when no config path is given +Verify that errors are properly handled when no settings path is given """ +import os from drivers.alr import run_alr from drivers.asserts import assert_match import re -p = run_alr("--config", complain_on_error=False) -assert p.status != 0, "command should fail" -assert_match("ERROR: Switch --config requires argument.*", p.out, flags=re.S) +switch = "--settings" +short = "-s" -p = run_alr("-c", complain_on_error=False) -assert p.status != 0, "command should fail" -assert_match("ERROR: Switch -c requires argument.*", p.out, flags=re.S) +p = run_alr(switch, complain_on_error=False) +assert_match(f"ERROR: Switch {switch} requires argument.*", p.out, flags=re.S) + +p = run_alr(short, complain_on_error=False) +assert_match(f"ERROR: Switch {short} requires argument.*", p.out, flags=re.S) + +# Check also failure in case of duplication of switch +path = os.getcwd() +p = run_alr(f"{short}", path, f"{switch}={path}", "version", + complain_on_error=False) +assert_match(".*Only one of .* allowed", + p.out) print('SUCCESS') diff --git a/testsuite/tests/settings/early-loading/test.py b/testsuite/tests/settings/early-loading/test.py index c3bdfa9c..263bfc1e 100644 --- a/testsuite/tests/settings/early-loading/test.py +++ b/testsuite/tests/settings/early-loading/test.py @@ -17,9 +17,9 @@ expected = "test_value=42\n" # Verify proper loading with both short and long config options assert_eq(expected, - run_alr("-c", custom_config, "settings", "--global").out) + run_alr("-s", custom_config, "settings", "--global").out) assert_eq(expected, - run_alr(f"--config={custom_config}", "settings", "--global").out) + run_alr(f"--settings={custom_config}", "settings", "--global").out) # Verify also when using environment variable os.environ["ALIRE_SETTINGS_DIR"] = os.path.abspath(custom_config) diff --git a/testsuite/tests/settings/relative_config_path/test.py b/testsuite/tests/settings/relative_config_path/test.py index 6f3d8ff6..a425515e 100644 --- a/testsuite/tests/settings/relative_config_path/test.py +++ b/testsuite/tests/settings/relative_config_path/test.py @@ -10,7 +10,7 @@ from drivers.alr import run_alr from drivers.asserts import assert_eq from drivers.helpers import lines_of -run_alr("--config=.", "settings", "--global", +run_alr("--settings=.", "settings", "--global", "--set", "some_config_key", "true") diff --git a/testsuite/tests/toolchain/directories/test.py b/testsuite/tests/toolchain/directories/test.py index cc6d305c..1d295403 100644 --- a/testsuite/tests/toolchain/directories/test.py +++ b/testsuite/tests/toolchain/directories/test.py @@ -11,7 +11,7 @@ from drivers.helpers import contents # Identify config location p = run_alr("version") -config_dir = re.search("config folder:([^\n]*)", p.out).group(1).strip() +config_dir = re.search("settings folder:([^\n]*)", p.out).group(1).strip() config_dir = config_dir.replace("\\", "/") cache_dir = os.path.join(config_dir, "cache") -- 2.39.5