[PATCH 0/4] dts: error and usage improvements

[PATCH 0/4] dts: error and usage improvements

[Luca Vizzarro]

As mentioned in my previous DTS docs improvement patch series, here are
some usage improvements to DTS. The main purpose is to give the
first-time user of DTS some more meaningful messages of its usage.

Secondly, report back stderr to the user when remote commands fail. For
example, if DTS tries to run any program which is not installed on the
target node, it will just say that it failed with its return code. The
only way to see the actual error message is through the DEBUG level of
verbosity. Rightfully though, errors should be logged as ERROR.

Best,
Luca

Luca Vizzarro (4):
  dts: constrain DPDK source flag
  dts: customise argparse error message
  dts: show help when DTS is ran without args
  dts: log stderr with failed remote commands

 doc/guides/tools/dts.rst                      |  8 +-
 dts/framework/exception.py                    | 10 ++-
 .../remote_session/remote_session.py          |  2 +-
 dts/framework/settings.py                     | 83 ++++++++++++++-----
 dts/framework/utils.py                        | 43 ++++++----
 5 files changed, 104 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH 1/4] dts: constrain DPDK source flag

[Luca Vizzarro]

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. It also aids the user understand
how to use the DTS in the scenario it is ran without any arguments set.

Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 doc/guides/tools/dts.rst  |  8 +++--
 dts/framework/settings.py | 64 ++++++++++++++++++++++++++++-----------
 dts/framework/utils.py    | 43 ++++++++++++++++----------
 3 files changed, 79 insertions(+), 36 deletions(-)

diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
index 846696e14e..6e2da317e8 100644
--- a/doc/guides/tools/dts.rst
+++ b/doc/guides/tools/dts.rst
@@ -215,12 +215,16 @@ 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-cases TEST_CASES] [--re-run RE_RUN]
+   usage: main.py [-h] (--tarball FILEPATH | --revision ID) [--config-file CONFIG_FILE] [--output-dir OUTPUT_DIR] [-t TIMEOUT] [-v] [-s] [--compile-timeout COMPILE_TIMEOUT] [--test-cases TEST_CASES] [--re-run RE_RUN]
 
    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 FILEPATH, --snapshot FILEPATH
+                         Path to DPDK source code tarball to test. (default: None)
+   --revision ID, --rev ID, --git-ref ID
+                         Git revision ID to test. Could be commit, tag, tree ID and vice versa. To test local changes, first commit them, then use their commit ID (default: None)
    --config-file CONFIG_FILE
                          [DTS_CFG_FILE] configuration file that describes the test cases, SUTs and targets. (default: ./conf.yaml)
    --output-dir OUTPUT_DIR, --output OUTPUT_DIR
@@ -229,8 +233,6 @@ DTS is run with ``main.py`` located in the ``dts`` directory after entering Poet
                          [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: None)
-   --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-cases TEST_CASES
diff --git a/dts/framework/settings.py b/dts/framework/settings.py
index 609c8d0e62..2d0365e763 100644
--- a/dts/framework/settings.py
+++ b/dts/framework/settings.py
@@ -76,7 +76,8 @@
 from pathlib import Path
 from typing import Any, TypeVar
 
-from .utils import DPDKGitTarball
+from .exception import ConfigurationError
+from .utils import DPDKGitTarball, get_commit_id
 
 _T = TypeVar("_T")
 
@@ -149,6 +150,26 @@ def __call__(
     return _EnvironmentArgument
 
 
+def _parse_tarball(filepath: str) -> Path:
+    """Validate if the filepath is valid and return a Path object."""
+    path = Path(filepath)
+    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:
+    """Retrieve effective commit ID from a revision ID. While validating it."""
+
+    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.
@@ -167,7 +188,7 @@ class Settings:
     #:
     skip_setup: bool = False
     #:
-    dpdk_tarball_path: Path | str = "dpdk.tar.xz"
+    dpdk_tarball_path: Path | str = ""
     #:
     compile_timeout: float = 1200
     #:
@@ -186,6 +207,28 @@ def _get_parser() -> argparse.ArgumentParser:
         formatter_class=argparse.ArgumentDefaultsHelpFormatter,
     )
 
+    dpdk_source = parser.add_mutually_exclusive_group(required=True)
+
+    dpdk_source.add_argument(
+        "--tarball",
+        "--snapshot",
+        action='store',
+        type=_parse_tarball,
+        help="Path to DPDK source code tarball to test.",
+        metavar="FILEPATH",
+    )
+
+    dpdk_source.add_argument(
+        "--revision",
+        "--rev",
+        "--git-ref",
+        type=_parse_revision_id,
+        metavar="ID",
+        help="Git revision ID to test. Could be commit, tag, tree ID and "
+        "vice versa. To test local changes, first commit them, then use their "
+        "commit ID",
+    )
+
     parser.add_argument(
         "--config-file",
         action=_env_arg("DTS_CFG_FILE"),
@@ -229,18 +272,6 @@ def _get_parser() -> argparse.ArgumentParser:
         help="[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes.",
     )
 
-    parser.add_argument(
-        "--tarball",
-        "--snapshot",
-        "--git-ref",
-        action=_env_arg("DTS_DPDK_TARBALL"),
-        default=SETTINGS.dpdk_tarball_path,
-        type=Path,
-        help="[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.",
-    )
-
     parser.add_argument(
         "--compile-timeout",
         action=_env_arg("DTS_COMPILE_TIMEOUT"),
@@ -283,9 +314,8 @@ def get_settings() -> Settings:
         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)
+            parsed_args.tarball or
+            DPDKGitTarball(parsed_args.revision, parsed_args.output_dir)
         ),
         compile_timeout=parsed_args.compile_timeout,
         test_cases=(parsed_args.test_cases.split(",") if parsed_args.test_cases else []),
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index cc5e458cc8..dbbec8b00a 100644
--- a/dts/framework/utils.py
+++ b/dts/framework/utils.py
@@ -70,6 +70,32 @@ 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 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}"
+        )
+    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,21 +205,7 @@ 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)
-- 
2.34.1

[PATCH 2/4] dts: customise argparse error message

[Luca Vizzarro]

This commit customises the arguments parsing class' error message,
making it so the confusing usage is not displayed in these occurrences,
but the user is redirected to use the help argument instead.

Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 dts/framework/settings.py | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/dts/framework/settings.py b/dts/framework/settings.py
index 2d0365e763..acfe5cad44 100644
--- a/dts/framework/settings.py
+++ b/dts/framework/settings.py
@@ -170,6 +170,15 @@ def _parse_revision_id(rev_id: str) -> str:
         )
 
 
+class ArgumentParser(argparse.ArgumentParser):
+    """ArgumentParser with a custom error message."""
+    def error(self, message):
+        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")
+
+
 @dataclass(slots=True)
 class Settings:
     """Default framework-wide user settings.
@@ -200,8 +209,8 @@ class Settings:
 SETTINGS: Settings = Settings()
 
 
-def _get_parser() -> argparse.ArgumentParser:
-    parser = argparse.ArgumentParser(
+def _get_parser() -> ArgumentParser:
+    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,
-- 
2.34.1

[PATCH 3/4] dts: show help when DTS is ran without args

[Luca Vizzarro]

This commit changes the default behaviour of DTS, making it so that the
user automatically sees the help and usage page when running it without
any arguments set. Instead of being welcomed by an error message.

Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 dts/framework/settings.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/dts/framework/settings.py b/dts/framework/settings.py
index acfe5cad44..5809fd4e91 100644
--- a/dts/framework/settings.py
+++ b/dts/framework/settings.py
@@ -71,6 +71,7 @@
 
 import argparse
 import os
+import sys
 from collections.abc import Callable, Iterable, Sequence
 from dataclasses import dataclass, field
 from pathlib import Path
@@ -315,6 +316,11 @@ def get_settings() -> Settings:
 
     The inputs are taken from the command line and from environment variables.
     """
+
+    if len(sys.argv) == 1:
+        _get_parser().print_help()
+        sys.exit(1)
+
     parsed_args = _get_parser().parse_args()
     return Settings(
         config_file_path=parsed_args.config_file,
-- 
2.34.1

[PATCH 4/4] dts: log stderr with failed remote commands

[Luca Vizzarro]

Add the executed command stderr to RemoteCommandExecutionError. So that
it is logged as an error, instead of just as debug.

Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
 dts/framework/exception.py                     | 10 +++++++---
 dts/framework/remote_session/remote_session.py |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/dts/framework/exception.py b/dts/framework/exception.py
index 658eee2c38..9fc3fa096a 100644
--- a/dts/framework/exception.py
+++ b/dts/framework/exception.py
@@ -130,20 +130,24 @@ class RemoteCommandExecutionError(DTSError):
     #: The executed command.
     command: str
     _command_return_code: int
+    _command_stderr: str
 
-    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}\n{self._command_stderr}")
 
 
 class RemoteDirectoryExistsError(DTSError):
diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
index 2059f9a981..345439f2de 100644
--- a/dts/framework/remote_session/remote_session.py
+++ b/dts/framework/remote_session/remote_session.py
@@ -157,7 +157,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
-- 
2.34.1

Re: [PATCH 1/4] dts: constrain DPDK source flag

[Juraj Linkeš]

On Mon, Jan 22, 2024 at 7:26 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> 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.
>

I didn't see the mutual exclusion being enforced in the code. From
what I can tell, I could pass both --tarball FILEPATH and --revision
and the former would be used without checking the latter.

> 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.
>

I wanted to have just one argument for basically the same thing, but
the input is pretty different so a two argument solution actually
sounds better.

> Moreover it ensures that these flags are explicitly required to be set
> by the user, dropping a default value. It also aids the user understand
> how to use the DTS in the scenario it is ran without any arguments set.
>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---
>  doc/guides/tools/dts.rst  |  8 +++--
>  dts/framework/settings.py | 64 ++++++++++++++++++++++++++++-----------
>  dts/framework/utils.py    | 43 ++++++++++++++++----------
>  3 files changed, 79 insertions(+), 36 deletions(-)
>
> diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
> index 846696e14e..6e2da317e8 100644
> --- a/doc/guides/tools/dts.rst
> +++ b/doc/guides/tools/dts.rst
> @@ -215,12 +215,16 @@ 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-cases TEST_CASES] [--re-run RE_RUN]
> +   usage: main.py [-h] (--tarball FILEPATH | --revision ID) [--config-file CONFIG_FILE] [--output-dir OUTPUT_DIR] [-t TIMEOUT] [-v] [-s] [--compile-timeout COMPILE_TIMEOUT] [--test-cases TEST_CASES] [--re-run RE_RUN]
>
>     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 FILEPATH, --snapshot FILEPATH
> +                         Path to DPDK source code tarball to test. (default: None)
> +   --revision ID, --rev ID, --git-ref ID
> +                         Git revision ID to test. Could be commit, tag, tree ID and vice versa. To test local changes, first commit them, then use their commit ID (default: None)
>     --config-file CONFIG_FILE
>                           [DTS_CFG_FILE] configuration file that describes the test cases, SUTs and targets. (default: ./conf.yaml)
>     --output-dir OUTPUT_DIR, --output OUTPUT_DIR
> @@ -229,8 +233,6 @@ DTS is run with ``main.py`` located in the ``dts`` directory after entering Poet
>                           [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: None)
> -   --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-cases TEST_CASES
> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index 609c8d0e62..2d0365e763 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py
> @@ -76,7 +76,8 @@
>  from pathlib import Path
>  from typing import Any, TypeVar
>
> -from .utils import DPDKGitTarball
> +from .exception import ConfigurationError
> +from .utils import DPDKGitTarball, get_commit_id
>
>  _T = TypeVar("_T")
>
> @@ -149,6 +150,26 @@ def __call__(
>      return _EnvironmentArgument
>
>
> +def _parse_tarball(filepath: str) -> Path:
> +    """Validate if the filepath is valid and return a Path object."""

whether `filepath` is valid

Even though private methods don't get included in the API docs, I like
to be consistent. In this case, it doesn't really detract (but maybe
some disability would prove this wrong) while adding a bit (the fact
that we're referencing the argument).

I think the name should either be _validate_tarball or
_parse_tarball_path. The argument name is two words, so let's put an
underscore between them.

> +    path = Path(filepath)
> +    if not path.exists() or not path.is_file():
> +        raise argparse.ArgumentTypeError(
> +            "the file path provided is not a valid file")

Typo: capital T

> +    return path
> +
> +
> +def _parse_revision_id(rev_id: str) -> str:
> +    """Retrieve effective commit ID from a revision ID. While validating it."""

I think this would read better as one sentence.

> +
> +    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.
> @@ -167,7 +188,7 @@ class Settings:
>      #:
>      skip_setup: bool = False
>      #:
> -    dpdk_tarball_path: Path | str = "dpdk.tar.xz"
> +    dpdk_tarball_path: Path | str = ""
>      #:
>      compile_timeout: float = 1200
>      #:
> @@ -186,6 +207,28 @@ def _get_parser() -> argparse.ArgumentParser:
>          formatter_class=argparse.ArgumentDefaultsHelpFormatter,
>      )
>
> +    dpdk_source = parser.add_mutually_exclusive_group(required=True)
> +
> +    dpdk_source.add_argument(
> +        "--tarball",
> +        "--snapshot",
> +        action='store',
> +        type=_parse_tarball,
> +        help="Path to DPDK source code tarball to test.",
> +        metavar="FILEPATH",
> +    )
> +
> +    dpdk_source.add_argument(
> +        "--revision",
> +        "--rev",
> +        "--git-ref",
> +        type=_parse_revision_id,
> +        metavar="ID",

Since this is a patch with improvements, maybe we could add metavars
to other arguments as well. It looks pretty good.

> +        help="Git revision ID to test. Could be commit, tag, tree ID and "
> +        "vice versa. To test local changes, first commit them, then use their "
> +        "commit ID",
> +    )
> +

This removes the support for environment variables. It's possible we
don't want the support for these two arguments. Maybe we don't need
the support for variables at all. I'm leaning towards supporting the
env variables, but we probably should refactor how they're done. The
easiest would be to not do them with action, but just modifying the
default value if set. That would be a worthwhile improvement.

>      parser.add_argument(
>          "--config-file",
>          action=_env_arg("DTS_CFG_FILE"),
> @@ -229,18 +272,6 @@ def _get_parser() -> argparse.ArgumentParser:
>          help="[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes.",
>      )
>
> -    parser.add_argument(
> -        "--tarball",
> -        "--snapshot",
> -        "--git-ref",
> -        action=_env_arg("DTS_DPDK_TARBALL"),
> -        default=SETTINGS.dpdk_tarball_path,
> -        type=Path,
> -        help="[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.",
> -    )
> -
>      parser.add_argument(
>          "--compile-timeout",
>          action=_env_arg("DTS_COMPILE_TIMEOUT"),
> @@ -283,9 +314,8 @@ def get_settings() -> Settings:
>          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)
> +            parsed_args.tarball or
> +            DPDKGitTarball(parsed_args.revision, parsed_args.output_dir)
>          ),
>          compile_timeout=parsed_args.compile_timeout,
>          test_cases=(parsed_args.test_cases.split(",") if parsed_args.test_cases else []),
> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index cc5e458cc8..dbbec8b00a 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -70,6 +70,32 @@ 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.

This would also probably read better as one sentence.

> +    """
> +    result = subprocess.run(
> +        ["git", "rev-parse", "--verify", rev_id],
> +        text=True,
> +        capture_output=True,
> +    )
> +    if result.returncode != 0:
> +        raise ConfigurationError(
> +            f"{rev_id} 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}"
> +        )

We shuffled the order of operations a bit and now the error message
doesn't correspond.

> +    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()

Makes sense to move this outside.

>          self._create_tarball_dir()
>
>          self._tarball_name = (
> @@ -180,21 +205,7 @@ 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)
> --
> 2.34.1
>

Re: [PATCH 2/4] dts: customise argparse error message

[Juraj Linkeš]

On Mon, Jan 22, 2024 at 7:26 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit customises the arguments parsing class' error message,
> making it so the confusing usage is not displayed in these occurrences,

I'm curious, what exactly is confusing about the message?

> but the user is redirected to use the help argument instead.
>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---
>  dts/framework/settings.py | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index 2d0365e763..acfe5cad44 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py
> @@ -170,6 +170,15 @@ def _parse_revision_id(rev_id: str) -> str:
>          )
>
>
> +class ArgumentParser(argparse.ArgumentParser):
> +    """ArgumentParser with a custom error message."""
> +    def error(self, message):
> +        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")
> +
> +
>  @dataclass(slots=True)
>  class Settings:
>      """Default framework-wide user settings.
> @@ -200,8 +209,8 @@ class Settings:
>  SETTINGS: Settings = Settings()
>
>
> -def _get_parser() -> argparse.ArgumentParser:
> -    parser = argparse.ArgumentParser(
> +def _get_parser() -> ArgumentParser:
> +    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,
> --
> 2.34.1
>

Re: [PATCH 4/4] dts: log stderr with failed remote commands

[Juraj Linkeš]

On Mon, Jan 22, 2024 at 7:26 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add the executed command stderr to RemoteCommandExecutionError. So that
> it is logged as an error, instead of just as debug.

Here's I'd add logged additionally as an error, as this sounds as if
we're changing debug to error.

>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---
>  dts/framework/exception.py                     | 10 +++++++---
>  dts/framework/remote_session/remote_session.py |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> index 658eee2c38..9fc3fa096a 100644
> --- a/dts/framework/exception.py
> +++ b/dts/framework/exception.py
> @@ -130,20 +130,24 @@ class RemoteCommandExecutionError(DTSError):
>      #: The executed command.
>      command: str
>      _command_return_code: int
> +    _command_stderr: str
>

I'd change the order here (and all other places) so that stderr is
before the return code.

> -    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}\n{self._command_stderr}")

We should mention that the last string is the stderr output. Maybe we
just add 'Stderr:' before {self._command_stderr}. And maybe we should
put quotes around {self._command_stderr}.

>
>
>  class RemoteDirectoryExistsError(DTSError):
> diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
> index 2059f9a981..345439f2de 100644
> --- a/dts/framework/remote_session/remote_session.py
> +++ b/dts/framework/remote_session/remote_session.py
> @@ -157,7 +157,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
> --
> 2.34.1
>

Re: [PATCH 1/4] dts: constrain DPDK source flag

[Luca Vizzarro]

Hi Juraj,

Thank you for your review!

On 29/01/2024 11:47, Juraj Linkeš wrote:
> I didn't see the mutual exclusion being enforced in the code. From
> what I can tell, I could pass both --tarball FILEPATH and --revision
> and the former would be used without checking the latter.

Yep, it is enforced in the code, you may have missed it. The two 
arguments are under the same mutual exclusion group in line 220:

dpdk_source = parser.add_mutually_exclusive_group(required=True)

When using both arguments `argparse` will automatically complain that 
you can only use one or the other.

> whether `filepath` is valid
> Even though private methods don't get included in the API docs, I like
> to be consistent. In this case, it doesn't really detract (but maybe
> some disability would prove this wrong) while adding a bit (the fact
> that we're referencing the argument).

Yes, it is a good idea. Especially since this will work within IDEs.

> I think the name should either be _validate_tarball or
> _parse_tarball_path. The argument name is two words, so let's put an
> underscore between them.

Ack.

> I think this would read better as one sentence.

Ack.

> Since this is a patch with improvements, maybe we could add metavars
> to other arguments as well. It looks pretty good.

Sure, no problem!

> This removes the support for environment variables. It's possible we
> don't want the support for these two arguments. Maybe we don't need
> the support for variables at all. I'm leaning towards supporting the
> env variables, but we probably should refactor how they're done. The
> easiest would be to not do them with action, but just modifying the
> default value if set. That would be a worthwhile improvement.

I've tried to find a way to still keep them. But with arguments done 
this way, it is somewhat hard to understand the provenance of the value, 
whether it's set in the arguments, an environment variable or just the 
default value. Therefore, give a useful error message to the user when 
using something invalid. I'll try to come up with something as you 
suggested, although I am not entirely convinced it'll be ideal.

> This would also probably read better as one sentence.

Ack.

> We shuffled the order of operations a bit and now the error message
> doesn't correspond.

Sorry, I don't think I am understanding what you are referring to 
exactly. What do you mean?

Best,
Luca

Re: [PATCH 2/4] dts: customise argparse error message

[Luca Vizzarro]

On 29/01/2024 13:04, Juraj Linkeš wrote:
> I'm curious, what exactly is confusing about the message?

Unfortunately a bit too much time has passed... but if I remember 
correctly I think that given the great amount of arguments, whenever the 
message is printed a bit too much information is given to the user. So 
bottomline, too crowded

Re: [PATCH 4/4] dts: log stderr with failed remote commands

[Luca Vizzarro]

On 29/01/2024 13:10, Juraj Linkeš wrote:
> Here's I'd add logged additionally as an error, as this sounds as if
> we're changing debug to error

That is also a way of doing this, but an error is an error. If we wanted 
to log the same thing in debug and error, then when we go read the debug 
we get duplicates... making it less readable. What do you say?

> I'd change the order here (and all other places) so that stderr is
> before the return code.
Ack.

> We should mention that the last string is the stderr output. Maybe we
> just add 'Stderr:' before {self._command_stderr}. And maybe we should
> put quotes around {self._command_stderr}.

Since you mentioned "quotes", I'd think that it'd be even better to 
indent it as if it's a quote. With logs as busy as the ones DTS prints, 
adding some quotes may not change much as it's all already very crowded. 
Can prefix with 'Stderr: ' though.

Re: [PATCH 4/4] dts: log stderr with failed remote commands

[Juraj Linkeš]

On Fri, Feb 23, 2024 at 8:19 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 29/01/2024 13:10, Juraj Linkeš wrote:
> > Here's I'd add logged additionally as an error, as this sounds as if
> > we're changing debug to error
>
> That is also a way of doing this, but an error is an error. If we wanted
> to log the same thing in debug and error, then when we go read the debug
> we get duplicates... making it less readable. What do you say?
>

I meant let's change the commit message wording to better reflect what
the patch does - it adds stderr to the exception, not doing something
instead of logging it as debug (it could be understood this way). But
it doesn't really matter much. Maybe a better wording of the second
sentence would be "So that, instead of logging it just as debug, it is
also stored in an error, ."

> > I'd change the order here (and all other places) so that stderr is
> > before the return code.
> Ack.
>
> > We should mention that the last string is the stderr output. Maybe we
> > just add 'Stderr:' before {self._command_stderr}. And maybe we should
> > put quotes around {self._command_stderr}.
>
> Since you mentioned "quotes", I'd think that it'd be even better to
> indent it as if it's a quote. With logs as busy as the ones DTS prints,
> adding some quotes may not change much as it's all already very crowded.
> Can prefix with 'Stderr: ' though.

The prefix is essential so that we know what the output actually is.
Indenting it sounds great.

Re: [PATCH 2/4] dts: customise argparse error message

[Juraj Linkeš]

On Fri, Feb 23, 2024 at 8:12 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 29/01/2024 13:04, Juraj Linkeš wrote:
> > I'm curious, what exactly is confusing about the message?
>
> Unfortunately a bit too much time has passed... but if I remember
> correctly I think that given the great amount of arguments, whenever the
> message is printed a bit too much information is given to the user. So
> bottomline, too crowded

The original message is:
./main.py -wdghf
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]
main.py: error: unrecognized arguments: -wdghf

So this is what's confusing. I guess it doesn't mention that the user
should use the help argument and that's where the confusion was? From
my point of view that's just standard (to run a command with -h in
case of an error such as the one above), but maybe it is better to
state it explicitly.

Re: [PATCH 1/4] dts: constrain DPDK source flag

[Juraj Linkeš]

On Fri, Feb 23, 2024 at 8:09 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Hi Juraj,
>
> Thank you for your review!
>
> On 29/01/2024 11:47, Juraj Linkeš wrote:
> > I didn't see the mutual exclusion being enforced in the code. From
> > what I can tell, I could pass both --tarball FILEPATH and --revision
> > and the former would be used without checking the latter.
>
> Yep, it is enforced in the code, you may have missed it. The two
> arguments are under the same mutual exclusion group in line 220:
>
> dpdk_source = parser.add_mutually_exclusive_group(required=True)
>
> When using both arguments `argparse` will automatically complain that
> you can only use one or the other.
>

Yes, I missed this. This looks great.

> > whether `filepath` is valid
> > Even though private methods don't get included in the API docs, I like
> > to be consistent. In this case, it doesn't really detract (but maybe
> > some disability would prove this wrong) while adding a bit (the fact
> > that we're referencing the argument).
>
> Yes, it is a good idea. Especially since this will work within IDEs.
>
> > I think the name should either be _validate_tarball or
> > _parse_tarball_path. The argument name is two words, so let's put an
> > underscore between them.
>
> Ack.
>
> > I think this would read better as one sentence.
>
> Ack.
>
> > Since this is a patch with improvements, maybe we could add metavars
> > to other arguments as well. It looks pretty good.
>
> Sure, no problem!
>
> > This removes the support for environment variables. It's possible we
> > don't want the support for these two arguments. Maybe we don't need
> > the support for variables at all. I'm leaning towards supporting the
> > env variables, but we probably should refactor how they're done. The
> > easiest would be to not do them with action, but just modifying the
> > default value if set. That would be a worthwhile improvement.
>
> I've tried to find a way to still keep them. But with arguments done
> this way, it is somewhat hard to understand the provenance of the value,
> whether it's set in the arguments, an environment variable or just the
> default value. Therefore, give a useful error message to the user when
> using something invalid. I'll try to come up with something as you
> suggested, although I am not entirely convinced it'll be ideal.
>

For reference, my test case blocking patch implements an alternative
approach: https://patches.dpdk.org/project/dpdk/patch/20240223075502.60485-8-juraj.linkes@pantheon.tech/

It's the same thing, significantly simplified. Looks like it could work here.

> > This would also probably read better as one sentence.
>
> Ack.
>
> > We shuffled the order of operations a bit and now the error message
> > doesn't correspond.
>
> Sorry, I don't think I am understanding what you are referring to
> exactly. What do you mean?
>

You removed what I commented on (in the future, please keep that so
what we know what the original comment relates to), so here's what I
commented on:

+    if result.returncode != 0:
+        raise ConfigurationError(
+            f"{rev_id} 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}"
+        )

Previously, this was inside DPDKGitTarball which assumed it was passed
git ref because the command line arg is not a path. In this patch, the
git ref arg is decoupled from the tarball path, so the function
shouldn't reference the path.

> Best,
> Luca

[PATCH v2 0/3] dts: error and usage improvements

[Luca Vizzarro]

Hello!

Sending in v2 for my patch series, which changes a lot compared to v1.
The main and big change was the reworking of the arguments handling,
this can potentially be seen as a controversial change but I tried to
explain it as much as I could in the commit body message.

v2:
- complete rework of the arguments handling, to retain
  the environment variables and gain control over them
- prefixing 'Stderr: ' to RemoteCommandExecutionError
- rebased

Luca Vizzarro (3):
  dts: rework arguments framework
  dts: constrain DPDK source argument
  dts: store stderr in RemoteCommandExecutionError

 doc/guides/tools/dts.rst                      |  55 ++-
 dts/framework/exception.py                    |  13 +-
 .../remote_session/remote_session.py          |   3 +-
 dts/framework/settings.py                     | 459 +++++++++++++-----
 dts/framework/utils.py                        |  44 +-
 5 files changed, 417 insertions(+), 157 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] dts: rework arguments framework

[Luca Vizzarro]

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 <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
 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)
-- 
2.34.1

[PATCH v2 2/3] dts: constrain DPDK source argument

[Luca Vizzarro]

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 <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
 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)
 
-- 
2.34.1

[PATCH v2 3/3] dts: store stderr in RemoteCommandExecutionError

[Luca Vizzarro]

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 <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
 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
-- 
2.34.1

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

[Juraj Linkeš]

On Mon, Mar 18, 2024 at 6:17 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> 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.
>

Judging from the code, this patch seems like a convoluted way to implement:
1. An association between an Argument and the corresponding
environment variable,
2. A better way to add the env vars names to the help message of each
argument as well as adding the current value if set,
3. A better error message where we replace argument names with env var
names for args where env vars are used.

But maybe there's more.

In any case, isn't there a simpler way to implement the above? I
originally thought extending something in the argparse module (like
the add_argument methods in ArgumentParser and
_MutuallyExclusiveGroup), but that doesn't seem as simple as maybe
just augmenting the actions that are returned with the add_argument
methods (maybe we could subclass ArgumentParser and add some method
for this, such as add_dts_argument?), where we could just add the
env_var_name (and maybe arg_name if needed) and modify the help
message the same way we do now (modifying the self._action.help
attribute). This should also be enough for 3, but even if not, we
could store what we need in the subclass.

Also, what seems to be missing is the modification of actual values of
SETTING with the env var values (this could be done somewhere in the
add_dts_argument method).

But overall I like this approach as it gives us the knowledge of
whether an env var was used or not. I have some comments that
illustrate why I think the patch is a bit convoluted.

> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> ---
>  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)

Looking at this, this class could've been just a subclassed dict. We
could set the attributes with setattr in __init__(). But at that
point, it looks to be the same as the namespace returned by
parser.parse_args(), apart from the environment_fed_arguments property
(more below), which we could do without.

>
> -    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)
> +

We're already storing the arguments in the class, so we could just add
whatever is in ArgumentEnvPair to the argument and we have the
correspondence (looking at the Argument class again, we already have
that). The pair class seems redundant.

> +    @property
> +    def environment_fed_arguments(self) -> list[ArgumentEnvPair]:
> +        """Returns the list of all the successfully injected environment variables."""
> +        return self._env_vars
> +

And then we could get all of this from the stored arguments. Could be
just a tuple of (var_name, arg_name) of args with from_env == True.

> +
> +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}"
> +            )

I think this should also contain the env var value to be consistent
with the help message.

> +
> +        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)

We're going through all of the args here so we could just do this when
creating the argument. I guess we'd need to modify the top-level error
message afterwards with the current class structure, but I'm sure even
that could be done when creating the argument with some refactoring.

> +
> +    if len(sys.argv) == 1:
> +        parser.print_help()
> +        sys.exit(1)
> +
> +    ARGS.values = parser.parse_args()
> +

The values attribute seems superfluous as we're only using it locally.

> +    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)}

This is here so that we don't use both arguments from the mutual
group, right? We could add a short comment explaining this.
Also, I think we don't need to use vars() here, the result should be the same.

> +    return Settings(**kwargs)
> --
> 2.34.1
>

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

[Luca Vizzarro]

On 04/04/2024 10:25, Juraj Linkeš wrote:
> Judging from the code, this patch seems like a convoluted way to implement:
> 1. An association between an Argument and the corresponding
> environment variable,
> 2. A better way to add the env vars names to the help message of each
> argument as well as adding the current value if set,
> 3. A better error message where we replace argument names with env var
> names for args where env vars are used.
> 
> But maybe there's more.
> 
> In any case, isn't there a simpler way to implement the above? I
> originally thought extending something in the argparse module (like
> the add_argument methods in ArgumentParser and
> _MutuallyExclusiveGroup), but that doesn't seem as simple as maybe
> just augmenting the actions that are returned with the add_argument
> methods (maybe we could subclass ArgumentParser and add some method
> for this, such as add_dts_argument?), where we could just add the
> env_var_name (and maybe arg_name if needed) and modify the help
> message the same way we do now (modifying the self._action.help
> attribute). This should also be enough for 3, but even if not, we
> could store what we need in the subclass.

I agree that the solutions appears somewhat convoluted, and I am indeed 
open to ideas on how to simplify the approach. I initially considered 
extending the argparse module, but as you said it's no particularly 
simple, and it wasn't written to be particularly extendable either. If
we want to go down this route, it would be somewhat hacky. I am not 
against it though. But, yeah, in retrospective some things can be easily 
integrated.

> Also, what seems to be missing is the modification of actual values of
> SETTING with the env var values (this could be done somewhere in the
> add_dts_argument method).

I don't think I am following what you mean by this. If you refer to 
updating the values of `SETTING` with the environment ones, this is done 
using `inject_env_variable` before the arguments are parsed. In a few 
words, that method just injects the environment arguments in sys.argv. 
Therefore to the ArgumentParser it just looks like they were supplied on 
the command line.

> But overall I like this approach as it gives us the knowledge of
> whether an env var was used or not. I have some comments that
> illustrate why I think the patch is a bit convoluted.
> 
<snip>
> 
> Looking at this, this class could've been just a subclassed dict. We
> could set the attributes with setattr in __init__(). But at that
> point, it looks to be the same as the namespace returned by
> parser.parse_args(), apart from the environment_fed_arguments property
> (more below), which we could do without.
> 

Yes, definitely. The main reason to store the namespaced values was in 
case this could turn out to be useful when debugging in the future. But 
if we think it's not worthwhile, it can reduce complexity.

> 
> We're already storing the arguments in the class, so we could just add
> whatever is in ArgumentEnvPair to the argument and we have the
> correspondence (looking at the Argument class again, we already have
> that). The pair class seems redundant.
>
> 
> And then we could get all of this from the stored arguments. Could be
> just a tuple of (var_name, arg_name) of args with from_env == True.
> 

And storing a pre-made list of environment-fed arguments. Yes, we could 
definitely walk through every argument as needed. Given the context and 
usage of this, I guess yeah, you are right in saying it's redundant. 
Happy to remove it.

> 
> I think this should also contain the env var value to be consistent
> with the help message.
> 

Ack.

> 
> We're going through all of the args here so we could just do this when
> creating the argument. I guess we'd need to modify the top-level error
> message afterwards with the current class structure, but I'm sure even
> that could be done when creating the argument with some refactoring.
> 

Yeah. I decided to do this separately to avoid duplicating the code for 
the mutual exclusion group (as per the next commit).

> 
> The values attribute seems superfluous as we're only using it locally.
> 

Ack.

> 
> This is here so that we don't use both arguments from the mutual
> group, right? We could add a short comment explaining this.
> Also, I think we don't need to use vars() here, the result should be the same.
> 

Yes, and any other argument we may want to add in the future that don't 
belong in SETTINGS. It's only selecting the arguments that already exist 
in SETTING and write values for that. Can add a comment.

Namespace doesn't appear to implement iteration. As per the doc page[1], 
the suggested way is to use vars to extract a dict, which we can iterate 
over.

[1] https://docs.python.org/3/library/argparse.html#argparse.Namespace

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

[Juraj Linkeš]

On Tue, Apr 9, 2024 at 5:14 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 04/04/2024 10:25, Juraj Linkeš wrote:
> > Judging from the code, this patch seems like a convoluted way to implement:
> > 1. An association between an Argument and the corresponding
> > environment variable,
> > 2. A better way to add the env vars names to the help message of each
> > argument as well as adding the current value if set,
> > 3. A better error message where we replace argument names with env var
> > names for args where env vars are used.
> >
> > But maybe there's more.
> >
> > In any case, isn't there a simpler way to implement the above? I
> > originally thought extending something in the argparse module (like
> > the add_argument methods in ArgumentParser and
> > _MutuallyExclusiveGroup), but that doesn't seem as simple as maybe
> > just augmenting the actions that are returned with the add_argument
> > methods (maybe we could subclass ArgumentParser and add some method
> > for this, such as add_dts_argument?), where we could just add the
> > env_var_name (and maybe arg_name if needed) and modify the help
> > message the same way we do now (modifying the self._action.help
> > attribute). This should also be enough for 3, but even if not, we
> > could store what we need in the subclass.
>
> I agree that the solutions appears somewhat convoluted, and I am indeed
> open to ideas on how to simplify the approach. I initially considered
> extending the argparse module, but as you said it's no particularly
> simple, and it wasn't written to be particularly extendable either. If
> we want to go down this route, it would be somewhat hacky. I am not
> against it though. But, yeah, in retrospective some things can be easily
> integrated.
>

Great, let's get a v2 and see where we end up.

> > Also, what seems to be missing is the modification of actual values of
> > SETTING with the env var values (this could be done somewhere in the
> > add_dts_argument method).
>
> I don't think I am following what you mean by this. If you refer to
> updating the values of `SETTING` with the environment ones, this is done
> using `inject_env_variable` before the arguments are parsed. In a few
> words, that method just injects the environment arguments in sys.argv.
> Therefore to the ArgumentParser it just looks like they were supplied on
> the command line.
>

My bad, I must've made a mistake when verifying this. It is working fine.

> > But overall I like this approach as it gives us the knowledge of
> > whether an env var was used or not. I have some comments that
> > illustrate why I think the patch is a bit convoluted.
> >
> <snip>
> >
> > Looking at this, this class could've been just a subclassed dict. We
> > could set the attributes with setattr in __init__(). But at that
> > point, it looks to be the same as the namespace returned by
> > parser.parse_args(), apart from the environment_fed_arguments property
> > (more below), which we could do without.
> >
>
> Yes, definitely. The main reason to store the namespaced values was in
> case this could turn out to be useful when debugging in the future. But
> if we think it's not worthwhile, it can reduce complexity.
>
> >
> > We're already storing the arguments in the class, so we could just add
> > whatever is in ArgumentEnvPair to the argument and we have the
> > correspondence (looking at the Argument class again, we already have
> > that). The pair class seems redundant.
> >
> >
> > And then we could get all of this from the stored arguments. Could be
> > just a tuple of (var_name, arg_name) of args with from_env == True.
> >
>
> And storing a pre-made list of environment-fed arguments. Yes, we could
> definitely walk through every argument as needed. Given the context and
> usage of this, I guess yeah, you are right in saying it's redundant.
> Happy to remove it.
>
> >
> > I think this should also contain the env var value to be consistent
> > with the help message.
> >
>
> Ack.
>
> >
> > We're going through all of the args here so we could just do this when
> > creating the argument. I guess we'd need to modify the top-level error
> > message afterwards with the current class structure, but I'm sure even
> > that could be done when creating the argument with some refactoring.
> >
>
> Yeah. I decided to do this separately to avoid duplicating the code for
> the mutual exclusion group (as per the next commit).
>
> >
> > The values attribute seems superfluous as we're only using it locally.
> >
>
> Ack.
>
> >
> > This is here so that we don't use both arguments from the mutual
> > group, right? We could add a short comment explaining this.
> > Also, I think we don't need to use vars() here, the result should be the same.
> >
>
> Yes, and any other argument we may want to add in the future that don't
> belong in SETTINGS. It's only selecting the arguments that already exist
> in SETTING and write values for that. Can add a comment.
>
> Namespace doesn't appear to implement iteration. As per the doc page[1],
> the suggested way is to use vars to extract a dict, which we can iterate
> over.
>
> [1] https://docs.python.org/3/library/argparse.html#argparse.Namespace

Ah, I get it now. Thanks for pointing this out.