From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3510644111; Thu, 30 May 2024 17:30:47 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1578340691; Thu, 30 May 2024 17:30:47 +0200 (CEST) Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by mails.dpdk.org (Postfix) with ESMTP id 178E1402DE for ; Thu, 30 May 2024 17:30:46 +0200 (CEST) Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-a62ef52e837so109683266b.3 for ; Thu, 30 May 2024 08:30:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1717083046; x=1717687846; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=e0qnFwQDHjxOVeD/n2EBFTbCxM1EL2KKcCDPMiYSMxE=; b=Yyyb5Ln3kkgBhv/lciv/RHTSxM1MO1VfTTKXklMrAeqFjHXhgII39BbBLbbJpsrDIB 0y1zU116n1flMqaWIP3DrApjiojdS0i/lQ0t6KZ9c+Bly7OPWB0oUKqo6gRBwovOqalr i9ND0btv6YAY4Yd0GAoDUqDYtvOUago/86oQh0XZ7cnadSjw7rKYUE+6Zt4Rim0hNkxJ UDl1tnKD1bXbi+ASDThCGiyxc2BT0XYKL9BEsVco7gLyVyFib9MKqAudtSWdh7kL7kV0 J8OYMcH3jSLSXCpTraCFDJbjUABLBdL3vrVze3HJ69p1Opc+Tvrymx386ayiP6N7Iywl 0BJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717083046; x=1717687846; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=e0qnFwQDHjxOVeD/n2EBFTbCxM1EL2KKcCDPMiYSMxE=; b=BmsUWypt22nxguVBQqEafPoJvSGIdqcQWtQpOntVnCy0YMqnHWdzjzMuhdVCjaN7T0 TPl4Xnc61zU/UItSZJbMFkNSGI+swEdqpqSNFFpuUsB1RUAEmBO5Ci1FO8OP55y0PQDd AfA58m/R2gFOJ2v64PB8OZG21PUaslI0u/NS29uaumE6Es8N0HeQDjbvEDgJW3kDrVZl cOhQmQmSWwKrxbovTdUfWItoRpz8i3vtpFxYh/1uQHAFxZm5ujcHZLE4dJsc2G8TqS+O 89JsizsBxkq7Dac+pdoiQtj/D8sCMui6I2Rm1UUENkhnxumKnE/IHnn5/snKyuwCyjSl hhUw== X-Gm-Message-State: AOJu0YzJx7aXYRF3SsyviwrHAhQY1ps2IXgiWS2M7QezKDxswgfiAEGP w4j6xnY4b5uMQakdRs3l2wOyvklBoKQnxis640lZ8kFkrt45StSmg5wfRZoq3VTbA9a3dhueIaa 5/dLmYag285jA0h1VxlKZQqVE0bG8O1nrDG9o4Q== X-Google-Smtp-Source: AGHT+IF4AAmJIsySTsAaJcQ6Xslk37KcmCdqJS1BFWELZ/eZZcr9bZMMqCagddeNvsJ5ghTJZNXdO7Zjdg21wCYbMXY= X-Received: by 2002:a17:907:7007:b0:a63:5b2e:56c6 with SMTP id a640c23a62f3a-a65e90e82f8mr165393766b.57.1717083045544; Thu, 30 May 2024 08:30:45 -0700 (PDT) MIME-Version: 1.0 References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240514121023.1957025-1-luca.vizzarro@arm.com> <20240514121023.1957025-2-luca.vizzarro@arm.com> In-Reply-To: <20240514121023.1957025-2-luca.vizzarro@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Thu, 30 May 2024 17:30:34 +0200 Message-ID: Subject: Re: [PATCH v5 1/3] dts: rework arguments framework To: Luca Vizzarro Cc: dev@dpdk.org, Jeremy Spewock , Paul Szczepanek Content-Type: text/plain; charset="UTF-8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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. > 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'