[PATCH v5 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>
---
 doc/guides/tools/dts.rst  | 39 ++++++++++++------------
 dts/framework/settings.py | 62 +++++++++++++++++++++++++++++----------
 dts/framework/utils.py    | 43 ++++++++++++++++-----------
 3 files changed, 90 insertions(+), 54 deletions(-)

diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
index 6993443389..f64ab7f732 100644
--- a/doc/guides/tools/dts.rst
+++ b/doc/guides/tools/dts.rst
@@ -215,41 +215,38 @@ 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]
+   usage: main.py [-h] [--config-file FILE_PATH] [--output-dir DIR_PATH] [-t SECONDS] [-v] [-s] (--tarball FILE_PATH | --revision ID)
                   [--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 FILE_PATH
-                           [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets.
-                           (default: conf.yaml)
+                           [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets. (default:
+                           /home/lucviz01/dpdk/dts/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)
+                           [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)
+     --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)
      --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: [])
+                           [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 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)
+                           [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 b19f274f9d..50d8929450 100644
--- a/dts/framework/settings.py
+++ b/dts/framework/settings.py
@@ -44,10 +44,16 @@
 
     Set to any value to skip building DPDK.
 
-.. option:: --tarball, --snapshot, --git-ref
+.. option:: --tarball, --snapshot
 .. envvar:: DTS_DPDK_TARBALL
 
-    The path to a DPDK tarball, git commit ID, tag ID or tree ID to test.
+    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:: --test-suite
 .. envvar:: DTS_TEST_SUITES
@@ -84,7 +90,24 @@
 from typing import Callable, ParamSpec
 
 from .config import TestSuiteConfig
-from .utils import DPDKGitTarball
+from .exception import ConfigurationError
+from .utils import DPDKGitTarball, get_commit_id
+
+
+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)
@@ -105,7 +128,7 @@ class Settings:
     #:
     skip_setup: bool = False
     #:
-    dpdk_tarball_path: Path | str = "dpdk.tar.xz"
+    dpdk_tarball_path: Path | str = ""
     #:
     compile_timeout: float = 1200
     #:
@@ -305,20 +328,30 @@ def _get_parser() -> ArgumentParser:
         help="Specify to skip all setup steps on SUT and TG nodes.",
     )
 
-    add_argument_to_parser_with_env(
+    dpdk_source = parser.add_mutually_exclusive_group(required=True)
+    add_argument_to_dpdk_source_with_env = augment_add_argument_with_env(dpdk_source.add_argument)
+
+    add_argument_to_dpdk_source_with_env(
         "--tarball",
         "--snapshot",
-        "--git-ref",
-        default=SETTINGS.dpdk_tarball_path,
-        type=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.",
+        type=_parse_tarball_path,
+        help="Path to DPDK source code tarball to test.",
         metavar="FILE_PATH",
         dest="dpdk_tarball_path",
         env_var_name="DPDK_TARBALL",
     )
 
+    add_argument_to_dpdk_source_with_env(
+        "--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",
+        dest="dpdk_revision_id",
+    ),
+
     add_argument_to_parser_with_env(
         "--compile-timeout",
         default=SETTINGS.compile_timeout,
@@ -393,11 +426,8 @@ def get_settings() -> Settings:
 
     args = parser.parse_args()
 
-    args.dpdk_tarball_path = Path(
-        Path(DPDKGitTarball(args.dpdk_tarball_path, args.output_dir))
-        if not os.path.exists(args.dpdk_tarball_path)
-        else Path(args.dpdk_tarball_path)
-    )
+    if args.dpdk_revision_id:
+        args.dpdk_tarball_path = DPDKGitTarball(args.dpdk_revision_id, args.output_dir)
 
     args.test_suites = _process_test_suites(parser, args.test_suites)
 
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index 74a11f1aaf..862bafb46c 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