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

[Luca Vizzarro <luca.vizzarro@arm.com>]

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