From patchwork Mon Mar 18 17:17:02 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luca Vizzarro X-Patchwork-Id: 138455 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DC08E43CE8; Mon, 18 Mar 2024 18:17:26 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C5CB740633; Mon, 18 Mar 2024 18:17:22 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 3FD7B402B4 for ; Mon, 18 Mar 2024 18:17:21 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9B0E9106F; Mon, 18 Mar 2024 10:17:55 -0700 (PDT) Received: from localhost.localdomain (unknown [10.57.16.1]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 643CA3F67D; Mon, 18 Mar 2024 10:17:19 -0700 (PDT) From: Luca Vizzarro To: dev@dpdk.org Cc: =?utf-8?q?Juraj_Linke=C5=A1?= , Luca Vizzarro , Paul Szczepanek , Jack Bond-Preston Subject: [PATCH v2 1/3] dts: rework arguments framework Date: Mon, 18 Mar 2024 17:17:02 +0000 Message-Id: <20240318171704.798634-2-luca.vizzarro@arm.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20240318171704.798634-1-luca.vizzarro@arm.com> References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240318171704.798634-1-luca.vizzarro@arm.com> MIME-Version: 1.0 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 existing argument handling in the code relies on basic argparse functionality and a custom argparse action to integrate environment variables. This commit improves the current handling by augmenting argparse through a custom implementation of the arguments. This rework implements the following improvements: - There are duplicate expressions scattered throughout the code. To improve readability and maintainability, these are refactored into list/dict comprehensions or factory functions. - Arguments are augmented with their own class allowing for more flexibility, enabling manipulation while reducing redundancy. - Arguments are grouped within a new class. This class would track the origin of each argument, ensuring consistent input handling and facilitating debugging. - Instead of relying solely on argument flags, error messages now accurately reference environment variables when applicable, enhancing user experience. For instance: error: environment variable DTS_DPDK_TARBALL: Invalid file - Knowing the number of environment variables and arguments set allow for a useful help page display when none are provided. - A display of which environment variables have been detected and their corresponding values in the help page, aiding user awareness. Signed-off-by: Luca Vizzarro Reviewed-by: Paul Szczepanek Reviewed-by: Jack Bond-Preston --- doc/guides/tools/dts.rst | 53 +++-- dts/framework/settings.py | 393 ++++++++++++++++++++++++++++---------- 2 files changed, 324 insertions(+), 122 deletions(-) diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst index 47b218b2c6..6993443389 100644 --- a/doc/guides/tools/dts.rst +++ b/doc/guides/tools/dts.rst @@ -215,30 +215,41 @@ DTS is run with ``main.py`` located in the ``dts`` directory after entering Poet .. code-block:: console (dts-py3.10) $ ./main.py --help - usage: main.py [-h] [--config-file CONFIG_FILE] [--output-dir OUTPUT_DIR] [-t TIMEOUT] [-v] [-s] [--tarball TARBALL] [--compile-timeout COMPILE_TIMEOUT] [--test-suite TEST_SUITE [TEST_CASES ...]] [--re-run RE_RUN] + usage: main.py [-h] [--config-file FILE_PATH] [--output-dir DIR_PATH] [-t SECONDS] [-v] [-s] [--tarball FILE_PATH] + [--compile-timeout SECONDS] [--test-suite TEST_SUITE [TEST_CASES ...]] [--re-run N_TIMES] - Run DPDK test suites. All options may be specified with the environment variables provided in brackets. Command line arguments have higher priority. + Run DPDK test suites. All options may be specified with the environment variables provided in brackets. Command + line arguments have higher priority. options: - -h, --help show this help message and exit - --config-file CONFIG_FILE - [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets. (default: ./conf.yaml) - --output-dir OUTPUT_DIR, --output OUTPUT_DIR - [DTS_OUTPUT_DIR] Output directory where DTS logs and results are saved. (default: output) - -t TIMEOUT, --timeout TIMEOUT - [DTS_TIMEOUT] The default timeout for all DTS operations except for compiling DPDK. (default: 15) - -v, --verbose [DTS_VERBOSE] Specify to enable verbose output, logging all messages to the console. (default: False) - -s, --skip-setup [DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes. (default: False) - --tarball TARBALL, --snapshot TARBALL, --git-ref TARBALL - [DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID, tag ID or tree ID to test. To test local changes, first commit them, then use the commit ID with this option. (default: dpdk.tar.xz) - --compile-timeout COMPILE_TIMEOUT - [DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK. (default: 1200) - --test-suite TEST_SUITE [TEST_CASES ...] - [DTS_TEST_SUITES] A list containing a test suite with test cases. The first parameter is the test suite name, and the rest are test case names, which are optional. May be specified multiple times. To specify multiple test suites in the environment - variable, join the lists with a comma. Examples: --test-suite suite case case --test-suite suite case ... | DTS_TEST_SUITES='suite case case, suite case, ...' | --test-suite suite --test-suite suite case ... | DTS_TEST_SUITES='suite, suite case, ...' - (default: []) - --re-run RE_RUN, --re_run RE_RUN - [DTS_RERUN] Re-run each test case the specified number of times if a test failure occurs. (default: 0) + -h, --help show this help message and exit + --config-file FILE_PATH + [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets. + (default: conf.yaml) + --output-dir DIR_PATH, --output DIR_PATH + [DTS_OUTPUT_DIR] Output directory where DTS logs and results are saved. (default: output) + -t SECONDS, --timeout SECONDS + [DTS_TIMEOUT] The default timeout for all DTS operations except for compiling DPDK. + (default: 15) + -v, --verbose [DTS_VERBOSE] Specify to enable verbose output, logging all messages to the console. + (default: False) + -s, --skip-setup [DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes. (default: False) + --tarball FILE_PATH, --snapshot FILE_PATH, --git-ref FILE_PATH + [DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID,tag ID or tree ID to + test. To test local changes, first commit them, then use the commit ID with this option. + (default: dpdk.tar.xz) + --compile-timeout SECONDS + [DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK. (default: 1200) + --test-suite TEST_SUITE [TEST_CASES ...] + [DTS_TEST_SUITES] A list containing a test suite with test cases. The first parameter is + the test suite name, and the rest are test case names, which are optional. May be specified + multiple times. To specify multiple test suites in the environment variable, join the lists + with a comma. Examples: --test-suite SUITE1 CASE1 CASE2 --test-suite SUITE2 CASE1 ... | + DTS_TEST_SUITES='SUITE1 CASE1 CASE2, SUITE2 CASE1, ...' | --test-suite SUITE1 --test-suite SUITE2 + CASE1 ... | DTS_TEST_SUITES='SUITE1, SUITE2 CASE1, ...' (default: []) + --re-run N_TIMES, --re_run N_TIMES + [DTS_RERUN] Re-run each test case the specified number of times if a test failure occurs. + (default: 0) The brackets contain the names of environment variables that set the same thing. diff --git a/dts/framework/settings.py b/dts/framework/settings.py index 688e8679a7..421a9cb15b 100644 --- a/dts/framework/settings.py +++ b/dts/framework/settings.py @@ -2,6 +2,7 @@ # Copyright(c) 2010-2021 Intel Corporation # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. # Copyright(c) 2022 University of New Hampshire +# Copyright(c) 2024 Arm Limited """Environment variables and command line arguments parsing. @@ -62,6 +63,7 @@ The module provides one key module-level variable: Attributes: + ARGS: The module level variable storing the state of the DTS arguments. SETTINGS: The module level variable storing framework-wide DTS settings. Typical usage example:: @@ -72,14 +74,30 @@ import argparse import os +import sys from dataclasses import dataclass, field from pathlib import Path -from typing import Any +from typing import Any, Generator, NamedTuple from .config import TestSuiteConfig from .utils import DPDKGitTarball +#: The prefix to be added to all of the environment variables. +ENV_PREFIX = "DTS_" + + +DPDK_TARBALL_PATH_ARGUMENT_NAME = "dpdk_tarball_path" +CONFIG_FILE_ARGUMENT_NAME = "config_file" +OUTPUT_DIR_ARGUMENT_NAME = "output_dir" +TIMEOUT_ARGUMENT_NAME = "timeout" +VERBOSE_ARGUMENT_NAME = "verbose" +SKIP_SETUP_ARGUMENT_NAME = "skip_setup" +COMPILE_TIMEOUT_ARGUMENT_NAME = "compile_timeout" +TEST_SUITES_ARGUMENT_NAME = "test_suites" +RERUN_ARGUMENT_NAME = "re_run" + + @dataclass(slots=True) class Settings: """Default framework-wide user settings. @@ -110,126 +128,296 @@ class Settings: SETTINGS: Settings = Settings() -def _get_parser() -> argparse.ArgumentParser: - """Create the argument parser for DTS. +class ArgumentEnvPair(NamedTuple): + """A named tuple pairing the argument identifiers with its environment variable.""" - Command line options take precedence over environment variables, which in turn take precedence - over default values. + #: The argument name. + arg_name: str - Returns: - argparse.ArgumentParser: The configured argument parser with defined options. - """ + #: The argument's associated environment variable name. + env_var_name: str + + #: The argument's associated :class:`argparse.Action` name. + action_name: str | None + + +@dataclass +class Argument: + """A class representing a DTS argument.""" + + #: The identifying name of the argument. + #: It also translates into the corresponding :class:`Settings` attribute. + name: str + + #: A list of flags to pass to :meth:`argparse.ArgumentParser.add_argument`. + flags: tuple[str, ...] + + #: Any other keyword arguments to pass to :meth:`argparse.ArgumentParser.add_argument`. + kwargs: dict[str, Any] + + #: The corresponding environment variable name. + #: It is prefixed with the value stored in `ENV_PREFIX`. + #: If not specified, it is automatically generated from the :attr:`~name`. + env_var_name: str + + _from_env: bool = False + + #: A reference to its corresponding :class:`argparse.Action`. + _action: argparse.Action | None = None + + def __init__(self, name: str, *flags: str, env_var_name: str | None = None, **kwargs: Any): + """Class constructor. + + If the `help` argument is passed, this is prefixed with the + argument's corresponding environment variable in square brackets.""" + + self.name = name + self.flags = flags + self.kwargs = kwargs + self.env_var_name = self._make_env_var(env_var_name) + + if "help" in self.kwargs: + self.kwargs["help"] = f"[{self.env_var_name}] {self.kwargs['help']}" + + def add_to(self, parser: argparse._ActionsContainer): + """Adds this argument to an :class:`argparse.ArgumentParser` instance.""" + self._action = parser.add_argument(*self.flags, dest=self.name, **self.kwargs) + + def _make_env_var(self, env_var_name: str | None) -> str: + """Make the environment variable name.""" + return f"{ENV_PREFIX}{env_var_name or self.name.upper()}" + + def get_env_var(self) -> str | None: + """Get environment variable if it was supplied instead of a command line flag.""" + + env_var_value = os.environ.get(self.env_var_name) - def env_arg(env_var: str, default: Any) -> Any: - """A helper function augmenting the argparse with environment variables. + if env_var_value: + # check if the user has supplied any of this argument's flags in the command line + for flag in self.flags: + if flag in sys.argv: + return None - 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. + return env_var_value - Args: - env_var: Environment variable name. - default: Default value. + @property + def from_env(self) -> bool: + """Indicates if the argument value originated from the environment.""" + return self._from_env - Returns: - Environment variable or default value. + def inject_env_var(self, env_value: str) -> ArgumentEnvPair: + """Injects the environment variable as a program argument. + + Injects this argument's flag with the supplied environment variable's value and + returns an :class:`ArgumentEnvPair` object pairing this argument to its environment + variable and :class:`argparse.Action`. + + The help notice of the argument is updated to display that the environment variable + has been correctly picked up by showing its recorded value. + + .. note:: This method **must** be called after the argument has been added to the parser. """ - return os.environ.get(env_var) or default - parser = argparse.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, - ) + assert self._action is not None + + sys.argv[1:0] = [self.flags[0], env_value] + + self._from_env = True + + if "help" in self.kwargs: + self.kwargs["help"] = f"{self.kwargs['help']} (env value: {env_value})" + else: + self.kwargs["help"] = f"(env value: {env_value})" + + self._action.help = self.kwargs["help"] + + return ArgumentEnvPair( + arg_name=self.name, + env_var_name=self.env_var_name, + action_name=argparse._get_action_name(self._action), + ) + + +@dataclass +class ArgumentGroup: + """A class grouping all the instances of :class:`Argument`. + + This class provides indexing to access an :class:`Argument` by name: + + >>> args["dpdk_revision_id"].env_var_name + DTS_DPDK_REVISION_ID + + And can be iterated to access all the arguments: + + >>> arg_env_vars = [arg.env_var_name for arg in args] + ['DPDK_TARBALL', ..] + """ + + #: The arguments values as parsed by :class:`argparse.ArgumentParse`. + values: argparse.Namespace + + #: A dictionary pairing argument names to :class:`Argument` instances. + _args: dict[str, Argument] + + #: A list of :class:`ArgumentEnvPair` containing all the successfully injected environment variables. + _env_vars: list[ArgumentEnvPair] + + def __init__(self, *args: Argument): + self._args = {arg.name: arg for arg in args} + self._env_vars = [] + + def __getitem__(self, arg_name: str) -> Argument: + return self._args.__getitem__(arg_name) - parser.add_argument( + def __iter__(self) -> Generator[Argument, None, None]: + yield from self._args.values() + + def add_environment_fed_argument(self, env_pair: ArgumentEnvPair): + """Add an injected environment variable.""" + self._env_vars.append(env_pair) + + @property + def environment_fed_arguments(self) -> list[ArgumentEnvPair]: + """Returns the list of all the successfully injected environment variables.""" + return self._env_vars + + +ARGS = ArgumentGroup( + Argument( + CONFIG_FILE_ARGUMENT_NAME, "--config-file", - 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.", - ) - - parser.add_argument( + help="The configuration file that describes the test cases, SUTs and targets.", + metavar="FILE_PATH", + env_var_name="CFG_FILE", + ), + Argument( + OUTPUT_DIR_ARGUMENT_NAME, "--output-dir", "--output", - default=env_arg("DTS_OUTPUT_DIR", SETTINGS.output_dir), - help="[DTS_OUTPUT_DIR] Output directory where DTS logs and results are saved.", - ) - - parser.add_argument( + default=SETTINGS.output_dir, + help="Output directory where DTS logs and results are saved.", + metavar="DIR_PATH", + ), + Argument( + TIMEOUT_ARGUMENT_NAME, "-t", "--timeout", - default=env_arg("DTS_TIMEOUT", SETTINGS.timeout), + default=SETTINGS.timeout, type=float, - help="[DTS_TIMEOUT] The default timeout for all DTS operations except for compiling DPDK.", - ) - - parser.add_argument( + help="The default timeout for all DTS operations except for compiling DPDK.", + metavar="SECONDS", + ), + Argument( + VERBOSE_ARGUMENT_NAME, "-v", "--verbose", action="store_true", - default=env_arg("DTS_VERBOSE", SETTINGS.verbose), - help="[DTS_VERBOSE] Specify to enable verbose output, logging all messages " - "to the console.", - ) - - parser.add_argument( + help="Specify to enable verbose output, logging all messages " "to the console.", + ), + Argument( + SKIP_SETUP_ARGUMENT_NAME, "-s", "--skip-setup", action="store_true", - default=env_arg("DTS_SKIP_SETUP", SETTINGS.skip_setup), - help="[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes.", - ) - - parser.add_argument( + help="Specify to skip all setup steps on SUT and TG nodes.", + ), + Argument( + DPDK_TARBALL_PATH_ARGUMENT_NAME, "--tarball", "--snapshot", "--git-ref", - default=env_arg("DTS_DPDK_TARBALL", SETTINGS.dpdk_tarball_path), type=Path, - help="[DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID, " + default=SETTINGS.dpdk_tarball_path, + help="Path to DPDK source code tarball or a git commit ID," "tag ID or tree ID to test. To test local changes, first commit them, " "then use the commit ID with this option.", - ) - - parser.add_argument( + metavar="FILE_PATH", + env_var_name="DPDK_TARBALL", + ), + Argument( + COMPILE_TIMEOUT_ARGUMENT_NAME, "--compile-timeout", - default=env_arg("DTS_COMPILE_TIMEOUT", SETTINGS.compile_timeout), + default=SETTINGS.compile_timeout, type=float, - help="[DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK.", - ) - - parser.add_argument( + help="The timeout for compiling DPDK.", + metavar="SECONDS", + ), + Argument( + TEST_SUITES_ARGUMENT_NAME, "--test-suite", action="append", nargs="+", - metavar=("TEST_SUITE", "TEST_CASES"), - default=env_arg("DTS_TEST_SUITES", SETTINGS.test_suites), - help="[DTS_TEST_SUITES] A list containing a test suite with test cases. " + default=SETTINGS.test_suites, + help="A list containing a test suite with test cases. " "The first parameter is the test suite name, and the rest are test case names, " "which are optional. May be specified multiple times. To specify multiple test suites in " "the environment variable, join the lists with a comma. " "Examples: " - "--test-suite suite case case --test-suite suite case ... | " - "DTS_TEST_SUITES='suite case case, suite case, ...' | " - "--test-suite suite --test-suite suite case ... | " - "DTS_TEST_SUITES='suite, suite case, ...'", - ) - - parser.add_argument( + "--test-suite SUITE1 CASE1 CASE2 --test-suite SUITE2 CASE1 ... | " + "DTS_TEST_SUITES='SUITE1 CASE1 CASE2, SUITE2 CASE1, ...' | " + "--test-suite SUITE1 --test-suite SUITE2 CASE1 ... | " + "DTS_TEST_SUITES='SUITE1, SUITE2 CASE1, ...'", + metavar=("TEST_SUITE", "TEST_CASES"), + ), + Argument( + RERUN_ARGUMENT_NAME, "--re-run", "--re_run", - default=env_arg("DTS_RERUN", SETTINGS.re_run), + default=SETTINGS.re_run, type=int, - help="[DTS_RERUN] Re-run each test case the specified number of times " - "if a test failure occurs.", + help="Re-run each test case the specified number of times if a test failure occurs.", + env_var_name="RERUN", + metavar="N_TIMES", + ), +) + + +class ArgumentParser(argparse.ArgumentParser): + """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. + """ + + def error(self, message): + for _, env_var_name, action_name in ARGS.environment_fed_arguments: + message = message.replace( + f"argument {action_name}", f"environment variable {env_var_name}" + ) + + 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") + + +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, + allow_abbrev=False, ) + for arg in ARGS: + arg.add_to(parser) return parser -def _process_test_suites(args: str | list[list[str]]) -> list[TestSuiteConfig]: +def _process_test_suites(args: list[list[str]]) -> list[TestSuiteConfig]: """Process the given argument to a list of :class:`TestSuiteConfig` to execute. Args: @@ -240,17 +428,11 @@ 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_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:]) - ) + if ARGS[TEST_SUITES_ARGUMENT_NAME].from_env: + # 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(",")] - return test_suites_to_run + return [TestSuiteConfig(test_suite, test_cases) for [test_suite, *test_cases] in args] def get_settings() -> Settings: @@ -261,19 +443,28 @@ def get_settings() -> Settings: Returns: The new settings object. """ - parsed_args = _get_parser().parse_args() - return Settings( - config_file_path=parsed_args.config_file, - output_dir=parsed_args.output_dir, - timeout=parsed_args.timeout, - verbose=parsed_args.verbose, - skip_setup=parsed_args.skip_setup, - dpdk_tarball_path=Path( - Path(DPDKGitTarball(parsed_args.tarball, parsed_args.output_dir)) - if not os.path.exists(parsed_args.tarball) - else Path(parsed_args.tarball) - ), - compile_timeout=parsed_args.compile_timeout, - test_suites=_process_test_suites(parsed_args.test_suite), - re_run=parsed_args.re_run, + + parser = _get_parser() + + for arg in ARGS: + env_value = arg.get_env_var() + if env_value: + env_pair = arg.inject_env_var(env_value) + ARGS.add_environment_fed_argument(env_pair) + + if len(sys.argv) == 1: + parser.print_help() + sys.exit(1) + + ARGS.values = parser.parse_args() + + ARGS.values.dpdk_tarball_path = Path( + Path(DPDKGitTarball(ARGS.values.dpdk_tarball_path, ARGS.values.output_dir)) + if not os.path.exists(ARGS.values.dpdk_tarball_path) + else Path(ARGS.values.dpdk_tarball_path) ) + + ARGS.values.test_suites = _process_test_suites(ARGS.values.test_suites) + + kwargs = {k: v for k, v in vars(ARGS.values).items() if hasattr(SETTINGS, k)} + return Settings(**kwargs) From patchwork Mon Mar 18 17:17:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Luca Vizzarro X-Patchwork-Id: 138456 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2F93243CE8; Mon, 18 Mar 2024 18:17:37 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5BBA84069D; Mon, 18 Mar 2024 18:17:25 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 585EF40633 for ; Mon, 18 Mar 2024 18:17:22 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D39DC1FB; Mon, 18 Mar 2024 10:17:56 -0700 (PDT) Received: from localhost.localdomain (unknown [10.57.16.1]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id EFE043F67D; Mon, 18 Mar 2024 10:17:20 -0700 (PDT) From: Luca Vizzarro To: dev@dpdk.org Cc: =?utf-8?q?Juraj_Linke=C5=A1?= , Luca Vizzarro , Paul Szczepanek , Jack Bond-Preston Subject: [PATCH v2 2/3] dts: constrain DPDK source argument Date: Mon, 18 Mar 2024 17:17:03 +0000 Message-Id: <20240318171704.798634-3-luca.vizzarro@arm.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20240318171704.798634-1-luca.vizzarro@arm.com> References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240318171704.798634-1-luca.vizzarro@arm.com> MIME-Version: 1.0 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 DTS needs an input to gather the DPDK source code from. This is then built on the remote target. This commit makes sure that this input is more constrained, separating the Git revision ID – used to create a tarball using Git – and providing tarballed source code directly, while retaining mutual exclusion. This makes the code more readable and easier to handle for input validation, of which this commit introduces a basic one based on the pre-existing code. Moreover it ensures that these flags are explicitly required to be set by the user, dropping a default value. Signed-off-by: Luca Vizzarro Reviewed-by: Paul Szczepanek Reviewed-by: Jack Bond-Preston --- doc/guides/tools/dts.rst | 14 +++--- dts/framework/settings.py | 90 ++++++++++++++++++++++++++++----------- dts/framework/utils.py | 43 +++++++++++-------- 3 files changed, 98 insertions(+), 49 deletions(-) diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst index 6993443389..ea2bb34ddc 100644 --- a/doc/guides/tools/dts.rst +++ b/doc/guides/tools/dts.rst @@ -215,14 +215,20 @@ DTS is run with ``main.py`` located in the ``dts`` directory after entering Poet .. code-block:: console (dts-py3.10) $ ./main.py --help - usage: main.py [-h] [--config-file FILE_PATH] [--output-dir DIR_PATH] [-t SECONDS] [-v] [-s] [--tarball FILE_PATH] - [--compile-timeout SECONDS] [--test-suite TEST_SUITE [TEST_CASES ...]] [--re-run N_TIMES] + usage: main.py [-h] (--tarball FILE_PATH | --revision ID) [--config-file FILE_PATH] [--output-dir DIR_PATH] + [-t SECONDS] [-v] [-s] [--compile-timeout SECONDS] [--test-suite TEST_SUITE [TEST_CASES ...]] + [--re-run N_TIMES] Run DPDK test suites. All options may be specified with the environment variables provided in brackets. Command line arguments have higher priority. options: -h, --help show this help message and exit + --tarball FILE_PATH, --snapshot FILE_PATH + [DTS_DPDK_TARBALL] Path to DPDK source code tarball to test. (default: None) + --revision ID, --rev ID, --git-ref ID + [DTS_DPDK_REVISION_ID] Git revision ID to test. Could be commit, tag, tree ID etc. To test + local changes, first commit them, then use their commit ID. (default: None) --config-file FILE_PATH [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets. (default: conf.yaml) @@ -234,10 +240,6 @@ DTS is run with ``main.py`` located in the ``dts`` directory after entering Poet -v, --verbose [DTS_VERBOSE] Specify to enable verbose output, logging all messages to the console. (default: False) -s, --skip-setup [DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes. (default: False) - --tarball FILE_PATH, --snapshot FILE_PATH, --git-ref FILE_PATH - [DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID,tag ID or tree ID to - test. To test local changes, first commit them, then use the commit ID with this option. - (default: dpdk.tar.xz) --compile-timeout SECONDS [DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK. (default: 1200) --test-suite TEST_SUITE [TEST_CASES ...] diff --git a/dts/framework/settings.py b/dts/framework/settings.py index 421a9cb15b..ec238cea33 100644 --- a/dts/framework/settings.py +++ b/dts/framework/settings.py @@ -14,6 +14,17 @@ The command line arguments along with the supported environment variables are: +.. option:: --tarball, --snapshot +.. envvar:: DTS_DPDK_TARBALL + + Path to DPDK source code tarball to test. + +.. option:: --revision, --rev, --git-ref +.. envvar:: DTS_DPDK_REVISION_ID + + Git revision ID to test. Could be commit, tag, tree ID etc. + To test local changes, first commit them, then use their commit ID. + .. option:: --config-file .. envvar:: DTS_CFG_FILE @@ -44,11 +55,6 @@ Set to any value to skip building DPDK. -.. option:: --tarball, --snapshot, --git-ref -.. envvar:: DTS_DPDK_TARBALL - - The path to a DPDK tarball, git commit ID, tag ID or tree ID to test. - .. option:: --test-suite .. envvar:: DTS_TEST_SUITES @@ -79,8 +85,9 @@ from pathlib import Path from typing import Any, Generator, NamedTuple +from .exception import ConfigurationError from .config import TestSuiteConfig -from .utils import DPDKGitTarball +from .utils import DPDKGitTarball, get_commit_id #: The prefix to be added to all of the environment variables. @@ -88,6 +95,7 @@ DPDK_TARBALL_PATH_ARGUMENT_NAME = "dpdk_tarball_path" +DPDK_REVISION_ID_ARGUMENT_NAME = "dpdk_revision_id" CONFIG_FILE_ARGUMENT_NAME = "config_file" OUTPUT_DIR_ARGUMENT_NAME = "output_dir" TIMEOUT_ARGUMENT_NAME = "timeout" @@ -98,6 +106,24 @@ RERUN_ARGUMENT_NAME = "re_run" +def _parse_tarball_path(file_path: str) -> Path: + """Validate whether `file_path` is valid and return a Path object.""" + + path = Path(file_path) + if not path.exists() or not path.is_file(): + raise argparse.ArgumentTypeError("The file path provided is not a valid file") + return path + + +def _parse_revision_id(rev_id: str) -> str: + """Validate revision ID and retrieve corresponding commit ID.""" + + try: + return get_commit_id(rev_id) + except ConfigurationError: + raise argparse.ArgumentTypeError("The Git revision ID supplied is invalid or ambiguous") + + @dataclass(slots=True) class Settings: """Default framework-wide user settings. @@ -116,7 +142,7 @@ class Settings: #: skip_setup: bool = False #: - dpdk_tarball_path: Path | str = "dpdk.tar.xz" + dpdk_tarball_path: Path | str = "" #: compile_timeout: float = 1200 #: @@ -283,6 +309,25 @@ def environment_fed_arguments(self) -> list[ArgumentEnvPair]: ARGS = ArgumentGroup( + Argument( + DPDK_TARBALL_PATH_ARGUMENT_NAME, + "--tarball", + "--snapshot", + type=_parse_tarball_path, + help="Path to DPDK source code tarball to test.", + metavar="FILE_PATH", + env_var_name="DPDK_TARBALL", + ), + Argument( + DPDK_REVISION_ID_ARGUMENT_NAME, + "--revision", + "--rev", + "--git-ref", + type=_parse_revision_id, + help="Git revision ID to test. Could be commit, tag, tree ID etc. " + "To test local changes, first commit them, then use their commit ID.", + metavar="ID", + ), Argument( CONFIG_FILE_ARGUMENT_NAME, "--config-file", @@ -323,19 +368,6 @@ def environment_fed_arguments(self) -> list[ArgumentEnvPair]: action="store_true", help="Specify to skip all setup steps on SUT and TG nodes.", ), - Argument( - DPDK_TARBALL_PATH_ARGUMENT_NAME, - "--tarball", - "--snapshot", - "--git-ref", - type=Path, - default=SETTINGS.dpdk_tarball_path, - help="Path to DPDK source code tarball or a git commit ID," - "tag ID or tree ID to test. To test local changes, first commit them, " - "then use the commit ID with this option.", - metavar="FILE_PATH", - env_var_name="DPDK_TARBALL", - ), Argument( COMPILE_TIMEOUT_ARGUMENT_NAME, "--compile-timeout", @@ -411,7 +443,14 @@ def _get_parser() -> ArgumentParser: formatter_class=argparse.ArgumentDefaultsHelpFormatter, allow_abbrev=False, ) - for arg in ARGS: + + dpdk_source = parser.add_mutually_exclusive_group(required=True) + dpdk_source_arg_group = [DPDK_TARBALL_PATH_ARGUMENT_NAME, DPDK_REVISION_ID_ARGUMENT_NAME] + for arg_name in dpdk_source_arg_group: + ARGS[arg_name].add_to(dpdk_source) + + arg_group = [arg for arg in ARGS if arg.name not in dpdk_source_arg_group] + for arg in arg_group: arg.add_to(parser) return parser @@ -458,11 +497,10 @@ def get_settings() -> Settings: ARGS.values = parser.parse_args() - ARGS.values.dpdk_tarball_path = Path( - Path(DPDKGitTarball(ARGS.values.dpdk_tarball_path, ARGS.values.output_dir)) - if not os.path.exists(ARGS.values.dpdk_tarball_path) - else Path(ARGS.values.dpdk_tarball_path) - ) + if ARGS.values.dpdk_revision_id: + ARGS.values.dpdk_tarball_path = DPDKGitTarball( + ARGS.values.dpdk_revision_id, ARGS.values.output_dir + ) ARGS.values.test_suites = _process_test_suites(ARGS.values.test_suites) diff --git a/dts/framework/utils.py b/dts/framework/utils.py index cc5e458cc8..3976f92335 100644 --- a/dts/framework/utils.py +++ b/dts/framework/utils.py @@ -2,6 +2,7 @@ # Copyright(c) 2010-2014 Intel Corporation # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. # Copyright(c) 2022-2023 University of New Hampshire +# Copyright(c) 2024 Arm Limited """Various utility classes and functions. @@ -70,6 +71,31 @@ def get_packet_summaries(packets: list[Packet]) -> str: return f"Packet contents: \n{packet_summaries}" +def get_commit_id(rev_id: str) -> str: + """Given a Git revision ID, return the corresponding commit ID. + + Args: + rev_id: The Git revision ID. + + Raises: + ConfigurationError: The ``git rev-parse`` command failed, suggesting + an invalid or ambiguous revision ID was supplied. + """ + result = subprocess.run( + ["git", "rev-parse", "--verify", rev_id], + text=True, + capture_output=True, + ) + if result.returncode != 0: + raise ConfigurationError( + f"{rev_id} is not a valid git reference.\n" + f"Command: {result.args}\n" + f"Stdout: {result.stdout}\n" + f"Stderr: {result.stderr}" + ) + return result.stdout.strip() + + class StrEnum(Enum): """Enum with members stored as strings.""" @@ -170,7 +196,6 @@ def __init__( self._tarball_dir = Path(output_dir, "tarball") - self._get_commit_id() self._create_tarball_dir() self._tarball_name = ( @@ -180,22 +205,6 @@ def __init__( if not self._tarball_path: self._create_tarball() - def _get_commit_id(self) -> None: - result = subprocess.run( - ["git", "rev-parse", "--verify", self._git_ref], - text=True, - capture_output=True, - ) - if result.returncode != 0: - raise ConfigurationError( - f"{self._git_ref} is neither a path to an existing DPDK " - "archive nor a valid git reference.\n" - f"Command: {result.args}\n" - f"Stdout: {result.stdout}\n" - f"Stderr: {result.stderr}" - ) - self._git_ref = result.stdout.strip() - def _create_tarball_dir(self) -> None: os.makedirs(self._tarball_dir, exist_ok=True) From patchwork Mon Mar 18 17:17:04 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luca Vizzarro X-Patchwork-Id: 138457 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2F9BA43CE8; Mon, 18 Mar 2024 18:17:44 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A9E2C406B4; Mon, 18 Mar 2024 18:17:26 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 98F6C4027F for ; Mon, 18 Mar 2024 18:17:23 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 25C05106F; Mon, 18 Mar 2024 10:17:58 -0700 (PDT) Received: from localhost.localdomain (unknown [10.57.16.1]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 2E4A33F67D; Mon, 18 Mar 2024 10:17:22 -0700 (PDT) From: Luca Vizzarro To: dev@dpdk.org Cc: =?utf-8?q?Juraj_Linke=C5=A1?= , Luca Vizzarro , Paul Szczepanek , Jack Bond-Preston Subject: [PATCH v2 3/3] dts: store stderr in RemoteCommandExecutionError Date: Mon, 18 Mar 2024 17:17:04 +0000 Message-Id: <20240318171704.798634-4-luca.vizzarro@arm.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20240318171704.798634-1-luca.vizzarro@arm.com> References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240318171704.798634-1-luca.vizzarro@arm.com> MIME-Version: 1.0 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 Store the stderr of an executed command in RemoteCommandExecutionError. Consequently, when the exception is logged the error message includes the stderr. Signed-off-by: Luca Vizzarro Reviewed-by: Paul Szczepanek Reviewed-by: Jack Bond-Preston --- dts/framework/exception.py | 13 ++++++++++--- dts/framework/remote_session/remote_session.py | 3 ++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dts/framework/exception.py b/dts/framework/exception.py index cce1e0231a..50724acdf2 100644 --- a/dts/framework/exception.py +++ b/dts/framework/exception.py @@ -2,6 +2,7 @@ # Copyright(c) 2010-2014 Intel Corporation # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. # Copyright(c) 2022-2023 University of New Hampshire +# Copyright(c) 2024 Arm Limited """DTS exceptions. @@ -129,21 +130,27 @@ class RemoteCommandExecutionError(DTSError): severity: ClassVar[ErrorSeverity] = ErrorSeverity.REMOTE_CMD_EXEC_ERR #: The executed command. command: str + _command_stderr: str _command_return_code: int - def __init__(self, command: str, command_return_code: int): + def __init__(self, command: str, command_return_code: int, command_stderr: str): """Define the meaning of the first two arguments. Args: command: The executed command. command_return_code: The return code of the executed command. + command_stderr: The stderr of the executed command. """ self.command = command self._command_return_code = command_return_code + self._command_stderr = command_stderr def __str__(self) -> str: - """Include both the command and return code in the string representation.""" - return f"Command {self.command} returned a non-zero exit code: {self._command_return_code}" + """Include the command, its return code and stderr in the string representation.""" + return ( + f"Command '{self.command}' returned a non-zero exit code: " + f"{self._command_return_code}\nStderr: {self._command_stderr}" + ) class InteractiveCommandExecutionError(DTSError): diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py index ad0f53720a..9aaa8c8a04 100644 --- a/dts/framework/remote_session/remote_session.py +++ b/dts/framework/remote_session/remote_session.py @@ -2,6 +2,7 @@ # Copyright(c) 2010-2014 Intel Corporation # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. # Copyright(c) 2022-2023 University of New Hampshire +# Copyright(c) 2024 Arm Limited """Base remote session. @@ -172,7 +173,7 @@ def send_command( ) self._logger.debug(f"stdout: '{result.stdout}'") self._logger.debug(f"stderr: '{result.stderr}'") - raise RemoteCommandExecutionError(command, result.return_code) + raise RemoteCommandExecutionError(command, result.return_code, result.stderr) self._logger.debug(f"Received from '{command}':\n{result}") self.history.append(result) return result