Re: [PATCH v5 1/3] dts: rework arguments framework

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

The overall approach looks solid. Maybe we can do some minor
improvements on it, but it's honestly fine the way it's now.

There is a difference in behavior when I pass no arguments and then I
either have or don't have an env var set:
./main.py
usage: main.py [-h] [--config-file FILE_PATH] ...
...

-----------------------------
DTS_SKIP_SETUP=Y ./main.py
main.py: error: one of the arguments --tarball/--snapshot
--revision/--rev/--git-ref is required

For help and usage, run the command with the --help flag.

I think this is because we're adding the env vars as actions (the
second scenario thus has at least one) and the first scenario doesn't
have any actions, which leads to the different output. I don't know
whether we want to unify these, but it's a bit confusing, as the error
applies to both cases.

<snip>

> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index 688e8679a7..b19f274f9d 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py

> @@ -109,104 +116,224 @@ class Settings:
>
>  SETTINGS: Settings = Settings()
>
> +P = ParamSpec("P")
>
> -def _get_parser() -> argparse.ArgumentParser:
> -    """Create the argument parser for DTS.
> +#: Attribute name representing the env variable name to augment :class:`~argparse.Action` with.
> +_ENV_VAR_NAME_ATTR = "env_var_name"
> +#: Attribute name representing the value origin to augment :class:`~argparse.Action` with.
> +_IS_FROM_ENV_ATTR = "is_from_env"
>
> -    Command line options take precedence over environment variables, which in turn take precedence
> -    over default values.
> +#: The prefix to be added to all of the environment variables.
> +ENV_PREFIX = "DTS_"
> +
> +
> +def is_action_in_args(action: Action) -> bool:

I don't think there's any expectation that these functions will be
used outside this module, so I'd make them all private, in which case
the short docstring would be fine.

The ParamSpec also maybe should be private. Same goes for ENV_PREFIX.

I'm also looking at the order of the various functions, classes and
variables in the module and it looks all over the place. Maybe we can
tidy it up a bit.

> +    """Check if the action is invoked in the command line arguments."""
> +    for option in action.option_strings:
> +        if option in sys.argv:
> +            return True
> +    return False
> +
> +
> +def make_env_var_name(action: Action, env_var_name: str | None) -> str:
> +    """Make and assign an environment variable name to the given action."""
> +    env_var_name = f"{ENV_PREFIX}{env_var_name or action.dest.upper()}"
> +    setattr(action, _ENV_VAR_NAME_ATTR, env_var_name)
> +    return env_var_name
> +
> +
> +def get_env_var_name(action: Action) -> str | None:
> +    """Get the environment variable name of the given action."""
> +    return getattr(action, _ENV_VAR_NAME_ATTR, None)
> +
> +
> +def set_is_from_env(action: Action) -> None:
> +    """Make the environment the given action's value origin."""
> +    setattr(action, _IS_FROM_ENV_ATTR, True)
>
> -    Returns:
> -        argparse.ArgumentParser: The configured argument parser with defined options.
> -    """
>
> -    def env_arg(env_var: str, default: Any) -> Any:
> -        """A helper function augmenting the argparse with environment variables.
> +def is_from_env(action: Action) -> bool:
> +    """Check if the given action's value originated from the environment."""
> +    return getattr(action, _IS_FROM_ENV_ATTR, False)
>
> -        If the supplied environment variable is defined, then the default value
> -        of the argument is modified. This satisfies the priority order of
> -        command line argument > environment variable > default value.
>
> -        Args:
> -            env_var: Environment variable name.
> -            default: Default value.
> +def augment_add_argument_with_env(
> +    add_argument_fn: Callable[P, Action],
> +):
> +    """Augment any :class:`~argparse._ActionsContainer.add_argument` with environment variables."""
>
> -        Returns:
> -            Environment variable or default value.
> +    def _add_argument(
> +        *args: P.args,
> +        env_var_name: str | None = None,
> +        **kwargs: P.kwargs,
> +    ) -> Action:
> +        """Add an argument with an environment variable to the parser."""
> +        action = add_argument_fn(*args, **kwargs)
> +        env_var_name = make_env_var_name(action, env_var_name)
> +
> +        if not is_action_in_args(action):
> +            env_var_value = os.environ.get(env_var_name)
> +            if env_var_value:
> +                set_is_from_env(action)
> +                sys.argv[1:0] = [action.format_usage(), env_var_value]
> +
> +        return action
> +
> +    return _add_argument
> +
> +
> +class ArgumentParser(argparse.ArgumentParser):

I'd rename this to DTSArgumentParser and maybe also make the classes
(this one and the formatter) private.

> +    """ArgumentParser with a custom error message.
> +
> +    This custom version of ArgumentParser changes the error message to
> +    accurately reflect its origin if an environment variable is used
> +    as an argument.
> +
> +    Instead of printing usage on every error, it prints instructions
> +    on how to do it.

This sentence is confusing - how to do what?

> +    """
> +
> +    def find_action(
> +        self, action_name: str, filter_fn: Callable[[Action], bool] | None = None
> +    ) -> Action | None:
> +        """Find and return an action by its name.
> +
> +        Arguments:
> +            action_name: the name of the action to find.
> +            filter_fn: a filter function to use in the search.

It's not very clear from the description how this filter is applied.
We should mention that the found action (and not action_name) is going
to be passed to filter_fn.

>          """
> -        return os.environ.get(env_var) or default
> +        it = (action for action in self._actions if action_name == _get_action_name(action))
> +        action = next(it, None)
> +
> +        if action is not None and filter_fn is not None:

Would a simplified condition "if action and filter_fn" work?

> +            return action if filter_fn(action) else None
> +
> +        return action
>
> -    parser = argparse.ArgumentParser(
> +    def error(self, message):
> +        """Augments :meth:`~argparse.ArgumentParser.error` with environment variable awareness."""
> +        for action in self._actions:
> +            if is_from_env(action):
> +                action_name = _get_action_name(action)
> +                env_var_name = get_env_var_name(action)
> +                env_var_value = os.environ.get(env_var_name)
> +
> +                message = message.replace(
> +                    f"argument {action_name}",
> +                    f"environment variable {env_var_name} (value: {env_var_value})",
> +                )
> +
> +        print(f"{self.prog}: error: {message}\n", file=sys.stderr)
> +        self.exit(2, "For help and usage, " "run the command with the --help flag.\n")
> +
> +
> +class EnvVarHelpFormatter(ArgumentDefaultsHelpFormatter):
> +    """Custom formatter to add environment variables in the help page."""

to the help page

> +
> +    def _get_help_string(self, action):
> +        """Overrides :meth:`ArgumentDefaultsHelpFormatter._get_help_string`."""
> +        help = super()._get_help_string(action)
> +
> +        env_var_name = get_env_var_name(action)
> +        if env_var_name is not None:
> +            help = f"[{env_var_name}] {help}"
> +
> +            env_var_value = os.environ.get(env_var_name)
> +            if env_var_value is not None:
> +                help += f" (env value: {env_var_value})"

Let's do this the same way as four lines above: help = f"{help} (env
value: {env_var_value})"

> +
> +        return help
> +
> +
> +def _get_parser() -> ArgumentParser:
> +    """Create the argument parser for DTS.
> +
> +    Command line options take precedence over environment variables, which in turn take precedence
> +    over default values.
> +
> +    Returns:
> +        ArgumentParser: The configured argument parser with defined options.
> +    """
> +    parser = ArgumentParser(
>          description="Run DPDK test suites. All options may be specified with the environment "
>          "variables provided in brackets. Command line arguments have higher priority.",
> -        formatter_class=argparse.ArgumentDefaultsHelpFormatter,
> +        formatter_class=EnvVarHelpFormatter,
> +        allow_abbrev=False,
>      )
>
> -    parser.add_argument(
> +    add_argument_to_parser_with_env = augment_add_argument_with_env(parser.add_argument)

I'm wondering whether we could modify the actual add_argument methods
of ArgumentParser and _MutuallyExclusiveGroup, either the class
methods or instance methods. Have you tried that? It could be easier
to understand.

Or, alternatively, we could do:

action = parser.add_argument(
    "--config-file",
    default=SETTINGS.config_file_path,
    type=Path,
    help="The configuration file that describes the test cases, SUTs
and targets.",
    metavar="FILE_PATH",
)
add_env_var_to_action(action, env_var_name="CFG_FILE")

This makes what we're trying to do a bit clearer, but requires two
calls instead of one, so maybe it's not better. I'm not sure.

> +
> +    add_argument_to_parser_with_env(
>          "--config-file",

We should rename Settings.config_file_path to correspond with this.
Otherwise it's going to be ignored by get_settings().

> -        default=env_arg("DTS_CFG_FILE", SETTINGS.config_file_path),
> +        default=SETTINGS.config_file_path,
>          type=Path,
> -        help="[DTS_CFG_FILE] The configuration file that describes the test cases, "
> -        "SUTs and targets.",
> +        help="The configuration file that describes the test cases, SUTs and targets.",
> +        metavar="FILE_PATH",
> +        env_var_name="CFG_FILE",
>      )
>

> @@ -240,17 +369,12 @@ def _process_test_suites(args: str | list[list[str]]) -> list[TestSuiteConfig]:
>      Returns:
>          A list of test suite configurations to execute.
>      """
> -    if isinstance(args, str):
> -        # Environment variable in the form of "suite case case, suite case, suite, ..."
> -        args = [suite_with_cases.split() for suite_with_cases in args.split(",")]
> +    test_suites = parser.find_action("test_suites", is_from_env)
> +    if test_suites is not None:
> +        # Environment variable in the form of "SUITE1 CASE1 CASE2, SUITE2 CASE1, SUITE3, ..."
> +        args = [suite_with_cases.split() for suite_with_cases in args[0][0].split(",")]
>
> -    test_suites_to_run = []
> -    for suite_with_cases in args:
> -        test_suites_to_run.append(
> -            TestSuiteConfig(test_suite=suite_with_cases[0], test_cases=suite_with_cases[1:])
> -        )
> -
> -    return test_suites_to_run
> +    return [TestSuiteConfig(test_suite, test_cases) for [test_suite, *test_cases] in args]

This doesn't work properly, if I use:
DTS_TEST_SUITES="hello_world test_hello_world_single_core, os_udp test_os_udp"
I'm getting:
execution_setup - dts - ERROR - Invalid test suite configuration
found: [TestSuiteConfig(test_suite='hello_world
test_hello_world_single_core, os_udp test_os_udp', test_cases=[])].
...
ModuleNotFoundError: No module named 'tests.TestSuite_hello_world
test_hello_world_single_core, os_udp test_os_udp'