[RFC,v4,1/4] dts: code adjustments for sphinx

  sphinx-build only imports the Python modules when building the
documentation; it doesn't run DTS. This requires changes that make the
code importable without running it. This means:
* properly guarding argument parsing in the if __name__ == '__main__'
  block.
* the logger used by DTS runner underwent the same treatment so that it
  doesn't create unnecessary log files.
* however, DTS uses the arguments to construct an object holding global
  variables. The defaults for the global variables needed to be moved
  from argument parsing elsewhere.
* importing the remote_session module from framework resulted in
  circular imports because of one module trying to import another
  module. This is fixed by more granular imports.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/config/__init__.py              |  3 -
 dts/framework/dts.py                          | 34 ++++++-
 dts/framework/remote_session/__init__.py      | 41 ++++-----
 .../interactive_remote_session.py             |  0
 .../{remote => }/interactive_shell.py         |  0
 .../{remote => }/python_shell.py              |  0
 .../remote_session/remote/__init__.py         | 27 ------
 .../{remote => }/remote_session.py            |  0
 .../{remote => }/ssh_session.py               |  0
 .../{remote => }/testpmd_shell.py             |  0
 dts/framework/settings.py                     | 92 +++++++++++--------
 dts/framework/test_suite.py                   |  3 +-
 dts/framework/testbed_model/__init__.py       | 12 +--
 dts/framework/testbed_model/common.py         | 29 ++++++
 dts/framework/testbed_model/{hw => }/cpu.py   | 13 +++
 dts/framework/testbed_model/hw/__init__.py    | 27 ------
 .../linux_session.py                          |  4 +-
 dts/framework/testbed_model/node.py           | 22 ++++-
 .../os_session.py                             | 14 +--
 dts/framework/testbed_model/{hw => }/port.py  |  0
 .../posix_session.py                          |  2 +-
 dts/framework/testbed_model/sut_node.py       |  8 +-
 dts/framework/testbed_model/tg_node.py        | 30 +-----
 .../traffic_generator/__init__.py             | 24 +++++
 .../capturing_traffic_generator.py            |  2 +-
 .../{ => traffic_generator}/scapy.py          | 17 +---
 .../traffic_generator.py                      | 16 +++-
 .../testbed_model/{hw => }/virtual_device.py  |  0
 dts/framework/utils.py                        | 53 +----------
 dts/main.py                                   |  3 +-
 30 files changed, 229 insertions(+), 247 deletions(-)
 rename dts/framework/remote_session/{remote => }/interactive_remote_session.py (100%)
 rename dts/framework/remote_session/{remote => }/interactive_shell.py (100%)
 rename dts/framework/remote_session/{remote => }/python_shell.py (100%)
 delete mode 100644 dts/framework/remote_session/remote/__init__.py
 rename dts/framework/remote_session/{remote => }/remote_session.py (100%)
 rename dts/framework/remote_session/{remote => }/ssh_session.py (100%)
 rename dts/framework/remote_session/{remote => }/testpmd_shell.py (100%)
 create mode 100644 dts/framework/testbed_model/common.py
 rename dts/framework/testbed_model/{hw => }/cpu.py (95%)
 delete mode 100644 dts/framework/testbed_model/hw/__init__.py
 rename dts/framework/{remote_session => testbed_model}/linux_session.py (98%)
 rename dts/framework/{remote_session => testbed_model}/os_session.py (97%)
 rename dts/framework/testbed_model/{hw => }/port.py (100%)
 rename dts/framework/{remote_session => testbed_model}/posix_session.py (99%)
 create mode 100644 dts/framework/testbed_model/traffic_generator/__init__.py
 rename dts/framework/testbed_model/{ => traffic_generator}/capturing_traffic_generator.py (99%)
 rename dts/framework/testbed_model/{ => traffic_generator}/scapy.py (96%)
 rename dts/framework/testbed_model/{ => traffic_generator}/traffic_generator.py (80%)
 rename dts/framework/testbed_model/{hw => }/virtual_device.py (100%)
  

On 8/31/23 11:04, Juraj Linkeš wrote:
> sphinx-build only imports the Python modules when building the
> documentation; it doesn't run DTS. This requires changes that make the
> code importable without running it. This means:
> * properly guarding argument parsing in the if __name__ == '__main__'
>    block.
> * the logger used by DTS runner underwent the same treatment so that it
>    doesn't create unnecessary log files.
> * however, DTS uses the arguments to construct an object holding global
>    variables. The defaults for the global variables needed to be moved
>    from argument parsing elsewhere.
> * importing the remote_session module from framework resulted in
>    circular imports because of one module trying to import another
>    module. This is fixed by more granular imports.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   dts/framework/config/__init__.py              |  3 -
>   dts/framework/dts.py                          | 34 ++++++-
>   dts/framework/remote_session/__init__.py      | 41 ++++-----
>   .../interactive_remote_session.py             |  0
>   .../{remote => }/interactive_shell.py         |  0
>   .../{remote => }/python_shell.py              |  0
>   .../remote_session/remote/__init__.py         | 27 ------
>   .../{remote => }/remote_session.py            |  0
>   .../{remote => }/ssh_session.py               |  0
>   .../{remote => }/testpmd_shell.py             |  0
>   dts/framework/settings.py                     | 92 +++++++++++--------
>   dts/framework/test_suite.py                   |  3 +-
>   dts/framework/testbed_model/__init__.py       | 12 +--
>   dts/framework/testbed_model/common.py         | 29 ++++++
>   dts/framework/testbed_model/{hw => }/cpu.py   | 13 +++
>   dts/framework/testbed_model/hw/__init__.py    | 27 ------
>   .../linux_session.py                          |  4 +-
>   dts/framework/testbed_model/node.py           | 22 ++++-
>   .../os_session.py                             | 14 +--
>   dts/framework/testbed_model/{hw => }/port.py  |  0
>   .../posix_session.py                          |  2 +-
>   dts/framework/testbed_model/sut_node.py       |  8 +-
>   dts/framework/testbed_model/tg_node.py        | 30 +-----
>   .../traffic_generator/__init__.py             | 24 +++++
>   .../capturing_traffic_generator.py            |  2 +-
>   .../{ => traffic_generator}/scapy.py          | 17 +---
>   .../traffic_generator.py                      | 16 +++-
>   .../testbed_model/{hw => }/virtual_device.py  |  0
>   dts/framework/utils.py                        | 53 +----------
>   dts/main.py                                   |  3 +-
>   30 files changed, 229 insertions(+), 247 deletions(-)
>   rename dts/framework/remote_session/{remote => }/interactive_remote_session.py (100%)
>   rename dts/framework/remote_session/{remote => }/interactive_shell.py (100%)
>   rename dts/framework/remote_session/{remote => }/python_shell.py (100%)
>   delete mode 100644 dts/framework/remote_session/remote/__init__.py
>   rename dts/framework/remote_session/{remote => }/remote_session.py (100%)
>   rename dts/framework/remote_session/{remote => }/ssh_session.py (100%)
>   rename dts/framework/remote_session/{remote => }/testpmd_shell.py (100%)
>   create mode 100644 dts/framework/testbed_model/common.py
>   rename dts/framework/testbed_model/{hw => }/cpu.py (95%)
>   delete mode 100644 dts/framework/testbed_model/hw/__init__.py
>   rename dts/framework/{remote_session => testbed_model}/linux_session.py (98%)
>   rename dts/framework/{remote_session => testbed_model}/os_session.py (97%)
>   rename dts/framework/testbed_model/{hw => }/port.py (100%)
>   rename dts/framework/{remote_session => testbed_model}/posix_session.py (99%)
>   create mode 100644 dts/framework/testbed_model/traffic_generator/__init__.py
>   rename dts/framework/testbed_model/{ => traffic_generator}/capturing_traffic_generator.py (99%)
>   rename dts/framework/testbed_model/{ => traffic_generator}/scapy.py (96%)
>   rename dts/framework/testbed_model/{ => traffic_generator}/traffic_generator.py (80%)
>   rename dts/framework/testbed_model/{hw => }/virtual_device.py (100%)
> 
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index cb7e00ba34..5de8b54bcf 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -324,6 +324,3 @@ def load_config() -> Configuration:
>       config: dict[str, Any] = warlock.model_factory(schema, name="_Config")(config_data)
>       config_obj: Configuration = Configuration.from_dict(dict(config))
>       return config_obj
> -
> -
> -CONFIGURATION = load_config()
> diff --git a/dts/framework/dts.py b/dts/framework/dts.py
> index f773f0c38d..925a212210 100644
> --- a/dts/framework/dts.py
> +++ b/dts/framework/dts.py
> @@ -3,22 +3,23 @@
>   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2022-2023 University of New Hampshire
>   
> +import logging
>   import sys
>   
>   from .config import (
> -    CONFIGURATION,
>       BuildTargetConfiguration,
>       ExecutionConfiguration,
>       TestSuiteConfig,
> +    load_config,
>   )
>   from .exception import BlockingTestSuiteError
>   from .logger import DTSLOG, getLogger
>   from .test_result import BuildTargetResult, DTSResult, ExecutionResult, Result
>   from .test_suite import get_test_suites
>   from .testbed_model import SutNode, TGNode
> -from .utils import check_dts_python_version
>   
> -dts_logger: DTSLOG = getLogger("DTSRunner")
> +# dummy defaults to satisfy linters
> +dts_logger: DTSLOG | logging.Logger = logging.getLogger("DTSRunner")
>   result: DTSResult = DTSResult(dts_logger)
>   
>   
> @@ -30,14 +31,18 @@ def run_all() -> None:
>       global dts_logger
>       global result
>   
> +    # create a regular DTS logger and create a new result with it
> +    dts_logger = getLogger("DTSRunner")
> +    result = DTSResult(dts_logger)
> +
>       # check the python version of the server that run dts
> -    check_dts_python_version()
> +    _check_dts_python_version()
>   
>       sut_nodes: dict[str, SutNode] = {}
>       tg_nodes: dict[str, TGNode] = {}
>       try:
>           # for all Execution sections
> -        for execution in CONFIGURATION.executions:
> +        for execution in load_config().executions:
>               sut_node = sut_nodes.get(execution.system_under_test_node.name)
>               tg_node = tg_nodes.get(execution.traffic_generator_node.name)
>   
> @@ -82,6 +87,25 @@ def run_all() -> None:
>       _exit_dts()
>   
>   
> +def _check_dts_python_version() -> None:
> +    def RED(text: str) -> str:
> +        return f"\u001B[31;1m{str(text)}\u001B[0m"
> +
> +    if sys.version_info.major < 3 or (
> +        sys.version_info.major == 3 and sys.version_info.minor < 10
> +    ):
> +        print(
> +            RED(
> +                (
> +                    "WARNING: DTS execution node's python version is lower than"
> +                    "python 3.10, is deprecated and will not work in future releases."
> +                )
> +            ),
> +            file=sys.stderr,
> +        )
> +        print(RED("Please use Python >= 3.10 instead"), file=sys.stderr)
> +
> +
>   def _run_execution(
>       sut_node: SutNode,
>       tg_node: TGNode,
> diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
> index 00b6d1f03a..5e7ddb2b05 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -12,29 +12,24 @@
>   
>   # pylama:ignore=W0611
>   
> -from framework.config import OS, NodeConfiguration
> -from framework.exception import ConfigurationError
> +from framework.config import NodeConfiguration
>   from framework.logger import DTSLOG
>   
> -from .linux_session import LinuxSession
> -from .os_session import InteractiveShellType, OSSession
> -from .remote import (
> -    CommandResult,
> -    InteractiveRemoteSession,
> -    InteractiveShell,
> -    PythonShell,
> -    RemoteSession,
> -    SSHSession,
> -    TestPmdDevice,
> -    TestPmdShell,
> -)
> -
> -
> -def create_session(
> +from .interactive_remote_session import InteractiveRemoteSession
> +from .interactive_shell import InteractiveShell
> +from .python_shell import PythonShell
> +from .remote_session import CommandResult, RemoteSession
> +from .ssh_session import SSHSession
> +from .testpmd_shell import TestPmdShell
> +
> +
> +def create_remote_session(
>       node_config: NodeConfiguration, name: str, logger: DTSLOG
> -) -> OSSession:
> -    match node_config.os:
> -        case OS.linux:
> -            return LinuxSession(node_config, name, logger)
> -        case _:
> -            raise ConfigurationError(f"Unsupported OS {node_config.os}")
> +) -> RemoteSession:
> +    return SSHSession(node_config, name, logger)
> +
> +
> +def create_interactive_session(
> +    node_config: NodeConfiguration, logger: DTSLOG
> +) -> InteractiveRemoteSession:
> +    return InteractiveRemoteSession(node_config, logger)
> diff --git a/dts/framework/remote_session/remote/interactive_remote_session.py b/dts/framework/remote_session/interactive_remote_session.py
> similarity index 100%
> rename from dts/framework/remote_session/remote/interactive_remote_session.py
> rename to dts/framework/remote_session/interactive_remote_session.py
> diff --git a/dts/framework/remote_session/remote/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> similarity index 100%
> rename from dts/framework/remote_session/remote/interactive_shell.py
> rename to dts/framework/remote_session/interactive_shell.py
> diff --git a/dts/framework/remote_session/remote/python_shell.py b/dts/framework/remote_session/python_shell.py
> similarity index 100%
> rename from dts/framework/remote_session/remote/python_shell.py
> rename to dts/framework/remote_session/python_shell.py
> diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py
> deleted file mode 100644
> index 06403691a5..0000000000
> --- a/dts/framework/remote_session/remote/__init__.py
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -# SPDX-License-Identifier: BSD-3-Clause
> -# Copyright(c) 2023 PANTHEON.tech s.r.o.
> -# Copyright(c) 2023 University of New Hampshire
> -
> -# pylama:ignore=W0611
> -
> -from framework.config import NodeConfiguration
> -from framework.logger import DTSLOG
> -
> -from .interactive_remote_session import InteractiveRemoteSession
> -from .interactive_shell import InteractiveShell
> -from .python_shell import PythonShell
> -from .remote_session import CommandResult, RemoteSession
> -from .ssh_session import SSHSession
> -from .testpmd_shell import TestPmdDevice, TestPmdShell
> -
> -
> -def create_remote_session(
> -    node_config: NodeConfiguration, name: str, logger: DTSLOG
> -) -> RemoteSession:
> -    return SSHSession(node_config, name, logger)
> -
> -
> -def create_interactive_session(
> -    node_config: NodeConfiguration, logger: DTSLOG
> -) -> InteractiveRemoteSession:
> -    return InteractiveRemoteSession(node_config, logger)
> diff --git a/dts/framework/remote_session/remote/remote_session.py b/dts/framework/remote_session/remote_session.py
> similarity index 100%
> rename from dts/framework/remote_session/remote/remote_session.py
> rename to dts/framework/remote_session/remote_session.py
> diff --git a/dts/framework/remote_session/remote/ssh_session.py b/dts/framework/remote_session/ssh_session.py
> similarity index 100%
> rename from dts/framework/remote_session/remote/ssh_session.py
> rename to dts/framework/remote_session/ssh_session.py
> diff --git a/dts/framework/remote_session/remote/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> similarity index 100%
> rename from dts/framework/remote_session/remote/testpmd_shell.py
> rename to dts/framework/remote_session/testpmd_shell.py
> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index cfa39d011b..bf86861efb 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py
> @@ -6,7 +6,7 @@
>   import argparse
>   import os
>   from collections.abc import Callable, Iterable, Sequence
> -from dataclasses import dataclass
> +from dataclasses import dataclass, field
>   from pathlib import Path
>   from typing import Any, TypeVar
>   
> @@ -22,7 +22,7 @@ def __init__(
>               option_strings: Sequence[str],
>               dest: str,
>               nargs: str | int | None = None,
> -            const: str | None = None,
> +            const: bool | None = None,
>               default: str = None,
>               type: Callable[[str], _T | argparse.FileType | None] = None,
>               choices: Iterable[_T] | None = None,
> @@ -32,6 +32,12 @@ def __init__(
>           ) -> None:
>               env_var_value = os.environ.get(env_var)
>               default = env_var_value or default
> +            if const is not None:
> +                nargs = 0
> +                default = const if env_var_value else default
> +                type = None
> +                choices = None
> +                metavar = None
>               super(_EnvironmentArgument, self).__init__(
>                   option_strings,
>                   dest,
> @@ -52,22 +58,28 @@ def __call__(
>               values: Any,
>               option_string: str = None,
>           ) -> None:
> -            setattr(namespace, self.dest, values)
> +            if self.const is not None:
> +                setattr(namespace, self.dest, self.const)
> +            else:
> +                setattr(namespace, self.dest, values)
>   
>       return _EnvironmentArgument
>   
>   
> -@dataclass(slots=True, frozen=True)
> +@dataclass(slots=True)
>   class _Settings:
> -    config_file_path: str
> -    output_dir: str
> -    timeout: float
> -    verbose: bool
> -    skip_setup: bool
> -    dpdk_tarball_path: Path
> -    compile_timeout: float
> -    test_cases: list
> -    re_run: int
> +    config_file_path: Path = Path(__file__).parent.parent.joinpath("conf.yaml")
> +    output_dir: str = "output"
> +    timeout: float = 15
> +    verbose: bool = False
> +    skip_setup: bool = False
> +    dpdk_tarball_path: Path | str = "dpdk.tar.xz"
> +    compile_timeout: float = 1200
> +    test_cases: list[str] = field(default_factory=list)
> +    re_run: int = 0
> +
> +
> +SETTINGS: _Settings = _Settings()
>   
>   
>   def _get_parser() -> argparse.ArgumentParser:
> @@ -81,7 +93,8 @@ def _get_parser() -> argparse.ArgumentParser:
>       parser.add_argument(
>           "--config-file",
>           action=_env_arg("DTS_CFG_FILE"),
> -        default="conf.yaml",
> +        default=SETTINGS.config_file_path,
> +        type=Path,
>           help="[DTS_CFG_FILE] configuration file that describes the test cases, SUTs "
>           "and targets.",
>       )
> @@ -90,7 +103,7 @@ def _get_parser() -> argparse.ArgumentParser:
>           "--output-dir",
>           "--output",
>           action=_env_arg("DTS_OUTPUT_DIR"),
> -        default="output",
> +        default=SETTINGS.output_dir,
>           help="[DTS_OUTPUT_DIR] Output directory where dts logs and results are saved.",
>       )
>   
> @@ -98,7 +111,7 @@ def _get_parser() -> argparse.ArgumentParser:
>           "-t",
>           "--timeout",
>           action=_env_arg("DTS_TIMEOUT"),
> -        default=15,
> +        default=SETTINGS.timeout,
>           type=float,
>           help="[DTS_TIMEOUT] The default timeout for all DTS operations except for "
>           "compiling DPDK.",
> @@ -108,8 +121,9 @@ def _get_parser() -> argparse.ArgumentParser:
>           "-v",
>           "--verbose",
>           action=_env_arg("DTS_VERBOSE"),
> -        default="N",
> -        help="[DTS_VERBOSE] Set to 'Y' to enable verbose output, logging all messages "
> +        default=SETTINGS.verbose,
> +        const=True,
> +        help="[DTS_VERBOSE] Specify to enable verbose output, logging all messages "
>           "to the console.",
>       )
>   
> @@ -117,8 +131,8 @@ def _get_parser() -> argparse.ArgumentParser:
>           "-s",
>           "--skip-setup",
>           action=_env_arg("DTS_SKIP_SETUP"),
> -        default="N",
> -        help="[DTS_SKIP_SETUP] Set to 'Y' to skip all setup steps on SUT and TG nodes.",
> +        const=True,
> +        help="[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes.",
>       )
>   
>       parser.add_argument(
> @@ -126,7 +140,7 @@ def _get_parser() -> argparse.ArgumentParser:
>           "--snapshot",
>           "--git-ref",
>           action=_env_arg("DTS_DPDK_TARBALL"),
> -        default="dpdk.tar.xz",
> +        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, "
> @@ -136,7 +150,7 @@ def _get_parser() -> argparse.ArgumentParser:
>       parser.add_argument(
>           "--compile-timeout",
>           action=_env_arg("DTS_COMPILE_TIMEOUT"),
> -        default=1200,
> +        default=SETTINGS.compile_timeout,
>           type=float,
>           help="[DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK.",
>       )
> @@ -153,7 +167,7 @@ def _get_parser() -> argparse.ArgumentParser:
>           "--re-run",
>           "--re_run",
>           action=_env_arg("DTS_RERUN"),
> -        default=0,
> +        default=SETTINGS.re_run,
>           type=int,
>           help="[DTS_RERUN] Re-run each test case the specified amount of times "
>           "if a test failure occurs",
> @@ -162,23 +176,21 @@ def _get_parser() -> argparse.ArgumentParser:
>       return parser
>   
>   
> -def _get_settings() -> _Settings:
> +def set_settings() -> None:
>       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 == "Y"),
> -        skip_setup=(parsed_args.skip_setup == "Y"),
> -        dpdk_tarball_path=Path(
> -            DPDKGitTarball(parsed_args.tarball, parsed_args.output_dir)
> -        )
> +    global SETTINGS
> +    SETTINGS.config_file_path = parsed_args.config_file
> +    SETTINGS.output_dir = parsed_args.output_dir
> +    SETTINGS.timeout = parsed_args.timeout
> +    SETTINGS.verbose = parsed_args.verbose
> +    SETTINGS.skip_setup = parsed_args.skip_setup
> +    SETTINGS.dpdk_tarball_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_cases=parsed_args.test_cases.split(",") if parsed_args.test_cases else [],
> -        re_run=parsed_args.re_run,
> +        else Path(parsed_args.tarball)
>       )
> -
> -
> -SETTINGS: _Settings = _get_settings()
> +    SETTINGS.compile_timeout = parsed_args.compile_timeout
> +    SETTINGS.test_cases = (
> +        parsed_args.test_cases.split(",") if parsed_args.test_cases else []
> +    )
> +    SETTINGS.re_run = parsed_args.re_run
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 3b890c0451..b381990d98 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -26,8 +26,7 @@
>   from .logger import DTSLOG, getLogger
>   from .settings import SETTINGS
>   from .test_result import BuildTargetResult, Result, TestCaseResult, TestSuiteResult
> -from .testbed_model import SutNode, TGNode
> -from .testbed_model.hw.port import Port, PortLink
> +from .testbed_model import Port, PortLink, SutNode, TGNode
>   from .utils import get_packet_summaries
>   
>   
> diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py
> index 5cbb859e47..8ced05653b 100644
> --- a/dts/framework/testbed_model/__init__.py
> +++ b/dts/framework/testbed_model/__init__.py
> @@ -9,15 +9,9 @@
>   
>   # pylama:ignore=W0611
>   
> -from .hw import (
> -    LogicalCore,
> -    LogicalCoreCount,
> -    LogicalCoreCountFilter,
> -    LogicalCoreList,
> -    LogicalCoreListFilter,
> -    VirtualDevice,
> -    lcore_filter,
> -)
> +from .cpu import LogicalCoreCount, LogicalCoreCountFilter, LogicalCoreList
>   from .node import Node
> +from .port import Port, PortLink
>   from .sut_node import SutNode
>   from .tg_node import TGNode
> +from .virtual_device import VirtualDevice
> diff --git a/dts/framework/testbed_model/common.py b/dts/framework/testbed_model/common.py
> new file mode 100644
> index 0000000000..9222f57847
> --- /dev/null
> +++ b/dts/framework/testbed_model/common.py
> @@ -0,0 +1,29 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 PANTHEON.tech s.r.o.
> +
> +
> +class MesonArgs(object):
> +    """
> +    Aggregate the arguments needed to build DPDK:
> +    default_library: Default library type, Meson allows "shared", "static" and "both".
> +               Defaults to None, in which case the argument won't be used.
> +    Keyword arguments: The arguments found in meson_options.txt in root DPDK directory.
> +               Do not use -D with them, for example:
> +               meson_args = MesonArgs(enable_kmods=True).
> +    """
> +
> +    _default_library: str
> +
> +    def __init__(self, default_library: str | None = None, **dpdk_args: str | bool):
> +        self._default_library = (
> +            f"--default-library={default_library}" if default_library else ""
> +        )
> +        self._dpdk_args = " ".join(
> +            (
> +                f"-D{dpdk_arg_name}={dpdk_arg_value}"
> +                for dpdk_arg_name, dpdk_arg_value in dpdk_args.items()
> +            )
> +        )
> +
> +    def __str__(self) -> str:
> +        return " ".join(f"{self._default_library} {self._dpdk_args}".split())
> diff --git a/dts/framework/testbed_model/hw/cpu.py b/dts/framework/testbed_model/cpu.py
> similarity index 95%
> rename from dts/framework/testbed_model/hw/cpu.py
> rename to dts/framework/testbed_model/cpu.py
> index d1918a12dc..8fe785dfe4 100644
> --- a/dts/framework/testbed_model/hw/cpu.py
> +++ b/dts/framework/testbed_model/cpu.py
> @@ -272,3 +272,16 @@ def filter(self) -> list[LogicalCore]:
>               )
>   
>           return filtered_lcores
> +
> +
> +def lcore_filter(
> +    core_list: list[LogicalCore],
> +    filter_specifier: LogicalCoreCount | LogicalCoreList,
> +    ascending: bool,
> +) -> LogicalCoreFilter:
> +    if isinstance(filter_specifier, LogicalCoreList):
> +        return LogicalCoreListFilter(core_list, filter_specifier, ascending)
> +    elif isinstance(filter_specifier, LogicalCoreCount):
> +        return LogicalCoreCountFilter(core_list, filter_specifier, ascending)
> +    else:
> +        raise ValueError(f"Unsupported filter r{filter_specifier}")
> diff --git a/dts/framework/testbed_model/hw/__init__.py b/dts/framework/testbed_model/hw/__init__.py
> deleted file mode 100644
> index 88ccac0b0e..0000000000
> --- a/dts/framework/testbed_model/hw/__init__.py
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -# SPDX-License-Identifier: BSD-3-Clause
> -# Copyright(c) 2023 PANTHEON.tech s.r.o.
> -
> -# pylama:ignore=W0611
> -
> -from .cpu import (
> -    LogicalCore,
> -    LogicalCoreCount,
> -    LogicalCoreCountFilter,
> -    LogicalCoreFilter,
> -    LogicalCoreList,
> -    LogicalCoreListFilter,
> -)
> -from .virtual_device import VirtualDevice
> -
> -
> -def lcore_filter(
> -    core_list: list[LogicalCore],
> -    filter_specifier: LogicalCoreCount | LogicalCoreList,
> -    ascending: bool,
> -) -> LogicalCoreFilter:
> -    if isinstance(filter_specifier, LogicalCoreList):
> -        return LogicalCoreListFilter(core_list, filter_specifier, ascending)
> -    elif isinstance(filter_specifier, LogicalCoreCount):
> -        return LogicalCoreCountFilter(core_list, filter_specifier, ascending)
> -    else:
> -        raise ValueError(f"Unsupported filter r{filter_specifier}")
> diff --git a/dts/framework/remote_session/linux_session.py b/dts/framework/testbed_model/linux_session.py
> similarity index 98%
> rename from dts/framework/remote_session/linux_session.py
> rename to dts/framework/testbed_model/linux_session.py
> index a3f1a6bf3b..7b60b5353f 100644
> --- a/dts/framework/remote_session/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -9,10 +9,10 @@
>   from typing_extensions import NotRequired
>   
>   from framework.exception import RemoteCommandExecutionError
> -from framework.testbed_model import LogicalCore
> -from framework.testbed_model.hw.port import Port
>   from framework.utils import expand_range
>   
> +from .cpu import LogicalCore
> +from .port import Port
>   from .posix_session import PosixSession
>   
>   
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index fc01e0bf8e..23efa79c50 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -12,23 +12,26 @@
>   from typing import Any, Callable, Type, Union
>   
>   from framework.config import (
> +    OS,
>       BuildTargetConfiguration,
>       ExecutionConfiguration,
>       NodeConfiguration,
>   )
> +from framework.exception import ConfigurationError
>   from framework.logger import DTSLOG, getLogger
> -from framework.remote_session import InteractiveShellType, OSSession, create_session
>   from framework.settings import SETTINGS
>   
> -from .hw import (
> +from .cpu import (
>       LogicalCore,
>       LogicalCoreCount,
>       LogicalCoreList,
>       LogicalCoreListFilter,
> -    VirtualDevice,
>       lcore_filter,
>   )
> -from .hw.port import Port
> +from .linux_session import LinuxSession
> +from .os_session import InteractiveShellType, OSSession
> +from .port import Port
> +from .virtual_device import VirtualDevice
>   
>   
>   class Node(ABC):
> @@ -69,6 +72,7 @@ def __init__(self, node_config: NodeConfiguration):
>       def _init_ports(self) -> None:
>           self.ports = [Port(self.name, port_config) for port_config in self.config.ports]
>           self.main_session.update_ports(self.ports)
> +
>           for port in self.ports:
>               self.configure_port_state(port)
>   
> @@ -249,3 +253,13 @@ def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]:
>               return lambda *args: None
>           else:
>               return func
> +
> +
> +def create_session(
> +    node_config: NodeConfiguration, name: str, logger: DTSLOG
> +) -> OSSession:
> +    match node_config.os:
> +        case OS.linux:
> +            return LinuxSession(node_config, name, logger)
> +        case _:
> +            raise ConfigurationError(f"Unsupported OS {node_config.os}")
> diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/testbed_model/os_session.py
> similarity index 97%
> rename from dts/framework/remote_session/os_session.py
> rename to dts/framework/testbed_model/os_session.py
> index 8a709eac1c..19ba9a69d5 100644
> --- a/dts/framework/remote_session/os_session.py
> +++ b/dts/framework/testbed_model/os_session.py
> @@ -10,19 +10,19 @@
>   
>   from framework.config import Architecture, NodeConfiguration, NodeInfo
>   from framework.logger import DTSLOG
> -from framework.remote_session.remote import InteractiveShell
> -from framework.settings import SETTINGS
> -from framework.testbed_model import LogicalCore
> -from framework.testbed_model.hw.port import Port
> -from framework.utils import MesonArgs
> -
> -from .remote import (
> +from framework.remote_session import (
>       CommandResult,
>       InteractiveRemoteSession,
> +    InteractiveShell,
>       RemoteSession,
>       create_interactive_session,
>       create_remote_session,
>   )
> +from framework.settings import SETTINGS
> +
> +from .common import MesonArgs
> +from .cpu import LogicalCore
> +from .port import Port
>   
>   InteractiveShellType = TypeVar("InteractiveShellType", bound=InteractiveShell)
>   
> diff --git a/dts/framework/testbed_model/hw/port.py b/dts/framework/testbed_model/port.py
> similarity index 100%
> rename from dts/framework/testbed_model/hw/port.py
> rename to dts/framework/testbed_model/port.py
> diff --git a/dts/framework/remote_session/posix_session.py b/dts/framework/testbed_model/posix_session.py
> similarity index 99%
> rename from dts/framework/remote_session/posix_session.py
> rename to dts/framework/testbed_model/posix_session.py
> index 5da0516e05..9a95baa353 100644
> --- a/dts/framework/remote_session/posix_session.py
> +++ b/dts/framework/testbed_model/posix_session.py
> @@ -9,8 +9,8 @@
>   from framework.config import Architecture, NodeInfo
>   from framework.exception import DPDKBuildError, RemoteCommandExecutionError
>   from framework.settings import SETTINGS
> -from framework.utils import MesonArgs
>   
> +from .common import MesonArgs
>   from .os_session import OSSession
>   
>   
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> index 202aebfd06..2b7e104dcd 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -15,12 +15,14 @@
>       NodeInfo,
>       SutNodeConfiguration,
>   )
> -from framework.remote_session import CommandResult, InteractiveShellType, OSSession
> +from framework.remote_session import CommandResult
>   from framework.settings import SETTINGS
> -from framework.utils import MesonArgs
>   
> -from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice
> +from .common import MesonArgs
> +from .cpu import LogicalCoreCount, LogicalCoreList
>   from .node import Node
> +from .os_session import InteractiveShellType, OSSession
> +from .virtual_device import VirtualDevice
>   
>   
>   class EalParameters(object):
> diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
> index 27025cfa31..166eb8430e 100644
> --- a/dts/framework/testbed_model/tg_node.py
> +++ b/dts/framework/testbed_model/tg_node.py
> @@ -16,16 +16,11 @@
>   
>   from scapy.packet import Packet  # type: ignore[import]
>   
> -from framework.config import (
> -    ScapyTrafficGeneratorConfig,
> -    TGNodeConfiguration,
> -    TrafficGeneratorType,
> -)
> -from framework.exception import ConfigurationError
> -
> -from .capturing_traffic_generator import CapturingTrafficGenerator
> -from .hw.port import Port
> +from framework.config import TGNodeConfiguration
> +
>   from .node import Node
> +from .port import Port
> +from .traffic_generator import CapturingTrafficGenerator, create_traffic_generator
>   
>   
>   class TGNode(Node):
> @@ -80,20 +75,3 @@ def close(self) -> None:
>           """Free all resources used by the node"""
>           self.traffic_generator.close()
>           super(TGNode, self).close()
> -
> -
> -def create_traffic_generator(
> -    tg_node: TGNode, traffic_generator_config: ScapyTrafficGeneratorConfig
> -) -> CapturingTrafficGenerator:
> -    """A factory function for creating traffic generator object from user config."""
> -
> -    from .scapy import ScapyTrafficGenerator
> -
> -    match traffic_generator_config.traffic_generator_type:
> -        case TrafficGeneratorType.SCAPY:
> -            return ScapyTrafficGenerator(tg_node, traffic_generator_config)
> -        case _:
> -            raise ConfigurationError(
> -                "Unknown traffic generator: "
> -                f"{traffic_generator_config.traffic_generator_type}"
> -            )
> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
> new file mode 100644
> index 0000000000..11bfa1ee0f
> --- /dev/null
> +++ b/dts/framework/testbed_model/traffic_generator/__init__.py
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 PANTHEON.tech s.r.o.
> +
> +from framework.config import ScapyTrafficGeneratorConfig, TrafficGeneratorType
> +from framework.exception import ConfigurationError
> +from framework.testbed_model.node import Node
> +
> +from .capturing_traffic_generator import CapturingTrafficGenerator
> +from .scapy import ScapyTrafficGenerator
> +
> +
> +def create_traffic_generator(
> +    tg_node: Node, traffic_generator_config: ScapyTrafficGeneratorConfig
> +) -> CapturingTrafficGenerator:
> +    """A factory function for creating traffic generator object from user config."""
> +
> +    match traffic_generator_config.traffic_generator_type:
> +        case TrafficGeneratorType.SCAPY:
> +            return ScapyTrafficGenerator(tg_node, traffic_generator_config)
> +        case _:
> +            raise ConfigurationError(
> +                "Unknown traffic generator: "
> +                f"{traffic_generator_config.traffic_generator_type}"
> +            )
> diff --git a/dts/framework/testbed_model/capturing_traffic_generator.py b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> similarity index 99%
> rename from dts/framework/testbed_model/capturing_traffic_generator.py
> rename to dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> index ab98987f8e..765378fb4a 100644
> --- a/dts/framework/testbed_model/capturing_traffic_generator.py
> +++ b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> @@ -16,9 +16,9 @@
>   from scapy.packet import Packet  # type: ignore[import]
>   
>   from framework.settings import SETTINGS
> +from framework.testbed_model.port import Port
>   from framework.utils import get_packet_summaries
>   
> -from .hw.port import Port
>   from .traffic_generator import TrafficGenerator
>   
>   
> diff --git a/dts/framework/testbed_model/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py
> similarity index 96%
> rename from dts/framework/testbed_model/scapy.py
> rename to dts/framework/testbed_model/traffic_generator/scapy.py
> index af0d4dbb25..395d7e9bc0 100644
> --- a/dts/framework/testbed_model/scapy.py
> +++ b/dts/framework/testbed_model/traffic_generator/scapy.py
> @@ -24,16 +24,15 @@
>   from scapy.packet import Packet  # type: ignore[import]
>   
>   from framework.config import OS, ScapyTrafficGeneratorConfig
> -from framework.logger import DTSLOG, getLogger
>   from framework.remote_session import PythonShell
>   from framework.settings import SETTINGS
> +from framework.testbed_model.node import Node
> +from framework.testbed_model.port import Port
>   
>   from .capturing_traffic_generator import (
>       CapturingTrafficGenerator,
>       _get_default_capture_name,
>   )
> -from .hw.port import Port
> -from .tg_node import TGNode
>   
>   """
>   ========= BEGIN RPC FUNCTIONS =========
> @@ -191,15 +190,9 @@ class ScapyTrafficGenerator(CapturingTrafficGenerator):
>       session: PythonShell
>       rpc_server_proxy: xmlrpc.client.ServerProxy
>       _config: ScapyTrafficGeneratorConfig
> -    _tg_node: TGNode
> -    _logger: DTSLOG
> -
> -    def __init__(self, tg_node: TGNode, config: ScapyTrafficGeneratorConfig):
> -        self._config = config
> -        self._tg_node = tg_node
> -        self._logger = getLogger(
> -            f"{self._tg_node.name} {self._config.traffic_generator_type}"
> -        )
> +
> +    def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
> +        super().__init__(tg_node, config)
>   
>           assert (
>               self._tg_node.config.os == OS.linux
> diff --git a/dts/framework/testbed_model/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> similarity index 80%
> rename from dts/framework/testbed_model/traffic_generator.py
> rename to dts/framework/testbed_model/traffic_generator/traffic_generator.py
> index 28c35d3ce4..ea7c3963da 100644
> --- a/dts/framework/testbed_model/traffic_generator.py
> +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> @@ -12,11 +12,12 @@
>   
>   from scapy.packet import Packet  # type: ignore[import]
>   
> -from framework.logger import DTSLOG
> +from framework.config import TrafficGeneratorConfig
> +from framework.logger import DTSLOG, getLogger
> +from framework.testbed_model.node import Node
> +from framework.testbed_model.port import Port
>   from framework.utils import get_packet_summaries
>   
> -from .hw.port import Port
> -
>   
>   class TrafficGenerator(ABC):
>       """The base traffic generator.
> @@ -24,8 +25,17 @@ class TrafficGenerator(ABC):
>       Defines the few basic methods that each traffic generator must implement.
>       """
>   
> +    _config: TrafficGeneratorConfig
> +    _tg_node: Node
>       _logger: DTSLOG
>   
> +    def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> +        self._config = config
> +        self._tg_node = tg_node
> +        self._logger = getLogger(
> +            f"{self._tg_node.name} {self._config.traffic_generator_type}"
> +        )
> +
>       def send_packet(self, packet: Packet, port: Port) -> None:
>           """Send a packet and block until it is fully sent.
>   
> diff --git a/dts/framework/testbed_model/hw/virtual_device.py b/dts/framework/testbed_model/virtual_device.py
> similarity index 100%
> rename from dts/framework/testbed_model/hw/virtual_device.py
> rename to dts/framework/testbed_model/virtual_device.py
> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index d27c2c5b5f..07e2d1c076 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -7,7 +7,6 @@
>   import json
>   import os
>   import subprocess
> -import sys
>   from enum import Enum
>   from pathlib import Path
>   from subprocess import SubprocessError
> @@ -16,6 +15,8 @@
>   
>   from .exception import ConfigurationError
>   
> +REGEX_FOR_PCI_ADDRESS = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> +
>   
>   class StrEnum(Enum):
>       @staticmethod
> @@ -28,25 +29,6 @@ def __str__(self) -> str:
>           return self.name
>   
>   
> -REGEX_FOR_PCI_ADDRESS = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> -
> -
> -def check_dts_python_version() -> None:
> -    if sys.version_info.major < 3 or (
> -        sys.version_info.major == 3 and sys.version_info.minor < 10
> -    ):
> -        print(
> -            RED(
> -                (
> -                    "WARNING: DTS execution node's python version is lower than"
> -                    "python 3.10, is deprecated and will not work in future releases."
> -                )
> -            ),
> -            file=sys.stderr,
> -        )
> -        print(RED("Please use Python >= 3.10 instead"), file=sys.stderr)
> -
> -
>   def expand_range(range_str: str) -> list[int]:
>       """
>       Process range string into a list of integers. There are two possible formats:
> @@ -77,37 +59,6 @@ def get_packet_summaries(packets: list[Packet]):
>       return f"Packet contents: \n{packet_summaries}"
>   
>   
> -def RED(text: str) -> str:
> -    return f"\u001B[31;1m{str(text)}\u001B[0m"
> -
> -
> -class MesonArgs(object):
> -    """
> -    Aggregate the arguments needed to build DPDK:
> -    default_library: Default library type, Meson allows "shared", "static" and "both".
> -               Defaults to None, in which case the argument won't be used.
> -    Keyword arguments: The arguments found in meson_options.txt in root DPDK directory.
> -               Do not use -D with them, for example:
> -               meson_args = MesonArgs(enable_kmods=True).
> -    """
> -
> -    _default_library: str
> -
> -    def __init__(self, default_library: str | None = None, **dpdk_args: str | bool):
> -        self._default_library = (
> -            f"--default-library={default_library}" if default_library else ""
> -        )
> -        self._dpdk_args = " ".join(
> -            (
> -                f"-D{dpdk_arg_name}={dpdk_arg_value}"
> -                for dpdk_arg_name, dpdk_arg_value in dpdk_args.items()
> -            )
> -        )
> -
> -    def __str__(self) -> str:
> -        return " ".join(f"{self._default_library} {self._dpdk_args}".split())
> -
> -
>   class _TarCompressionFormat(StrEnum):
>       """Compression formats that tar can use.
>   
> diff --git a/dts/main.py b/dts/main.py
> index 43311fa847..060ff1b19a 100755
> --- a/dts/main.py
> +++ b/dts/main.py
> @@ -10,10 +10,11 @@
>   
>   import logging
>   
> -from framework import dts
> +from framework import dts, settings
>   
>   
>   def main() -> None:
> +    settings.set_settings()
>       dts.run_all()
>   
>   

My only nitpick comment would be on the name of the file common.py that 
only contain the MesonArgs class. Looks good otherwise
  

<snip>

>
> My only nitpick comment would be on the name of the file common.py that
> only contain the MesonArgs class. Looks good otherwise

Could you elaborate a bit more, Yoan? The common.py module is supposed
to be extended with code common to all other modules in the
testbed_model package. Right now we only have MesonArgs which fits in
common.py, but we could also move something else into common.py. We
also could rename common.py to something else, but then the above
purpose would not be clear.

I'm finishing the docstrings soon so expect a new version where things
like these will be clearer. :-)
  

On 10/23/23 07:44, Juraj Linkeš wrote:
> <snip>
> 
>>
>> My only nitpick comment would be on the name of the file common.py that
>> only contain the MesonArgs class. Looks good otherwise
> 
> Could you elaborate a bit more, Yoan? The common.py module is supposed
> to be extended with code common to all other modules in the
> testbed_model package. Right now we only have MesonArgs which fits in
> common.py, but we could also move something else into common.py. We
> also could rename common.py to something else, but then the above
> purpose would not be clear.
> 
> I'm finishing the docstrings soon so expect a new version where things
> like these will be clearer. :-)

My issue with the name is that it isn't clear what is the purpose of 
this file. It only tell to some extend how it is used.
If we start adding more things in this file, then I see two options:
	- Either this is related to the current class, and thus the file could 
be named meson_arg or something along those lines.
	- Or it is unrelated to the current class, and we end up with a file 
coalescing random bits of code, which is usually a bit dirty in OOP.

Like I said, it's a bit of a nitpick, but given it's an RFC I hope 
you'll give it a thought in the next version.
  

On Mon, Oct 23, 2023 at 1:52 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> On 10/23/23 07:44, Juraj Linkeš wrote:
> > <snip>
> >
> >>
> >> My only nitpick comment would be on the name of the file common.py that
> >> only contain the MesonArgs class. Looks good otherwise
> >
> > Could you elaborate a bit more, Yoan? The common.py module is supposed
> > to be extended with code common to all other modules in the
> > testbed_model package. Right now we only have MesonArgs which fits in
> > common.py, but we could also move something else into common.py. We
> > also could rename common.py to something else, but then the above
> > purpose would not be clear.
> >
> > I'm finishing the docstrings soon so expect a new version where things
> > like these will be clearer. :-)
>
> My issue with the name is that it isn't clear what is the purpose of
> this file. It only tell to some extend how it is used.

Well, the name suggests it's code that's common to other modules, as
in code that other modules use, just like the MesonArgs class, which
is used in three different modules. I've chosen common.py as that's
what some of the DPDK libs (such as EAL) seem to be using for this
purpose. Maybe there's a better name though or we could move the class
elsewhere.

> If we start adding more things in this file, then I see two options:
>         - Either this is related to the current class, and thus the file could
> be named meson_arg or something along those lines.
>         - Or it is unrelated to the current class, and we end up with a file
> coalescing random bits of code, which is usually a bit dirty in OOP.
>

It's code that's reused in multiple places, I'm not sure whether that
qualifies as random bits of code. It could be in os_session.py (as
that would work import-wise), but that's not a good place to put it,
as the logic is actually utilized in sut_node.py. But putting it into
sut_node.py doesn't work because of imports. Maybe we could just put
it into utils.py in the framework dir, which is a very similar file,
if not the same. My original thoughts were to have a file with common
code in each package (if needed), depending on where the code is used
(package level-wise), but it looks like we can just have this sort of
common utilities on the top level.

> Like I said, it's a bit of a nitpick, but given it's an RFC I hope
> you'll give it a thought in the next version.

I thought a lot about this before submitting this RFC, but I wanted
someone to have a look at this exact thing - whether the common.py
file makes sense and what is the better name, common.py or utils.py
(which is why I have both in this patch). I'll move the MesonArgs
class to the top level utils.py and remove the common.py file as that
makes the most sense to me now.

If you have any recommendations we may be able to make this even better.
  

On 10/24/23 07:39, Juraj Linkeš wrote:
> On Mon, Oct 23, 2023 at 1:52 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>>
>> On 10/23/23 07:44, Juraj Linkeš wrote:
>>> <snip>
>>>
>>>>
>>>> My only nitpick comment would be on the name of the file common.py that
>>>> only contain the MesonArgs class. Looks good otherwise
>>>
>>> Could you elaborate a bit more, Yoan? The common.py module is supposed
>>> to be extended with code common to all other modules in the
>>> testbed_model package. Right now we only have MesonArgs which fits in
>>> common.py, but we could also move something else into common.py. We
>>> also could rename common.py to something else, but then the above
>>> purpose would not be clear.
>>>
>>> I'm finishing the docstrings soon so expect a new version where things
>>> like these will be clearer. :-)
>>
>> My issue with the name is that it isn't clear what is the purpose of
>> this file. It only tell to some extend how it is used.
> 
> Well, the name suggests it's code that's common to other modules, as
> in code that other modules use, just like the MesonArgs class, which
> is used in three different modules. I've chosen common.py as that's
> what some of the DPDK libs (such as EAL) seem to be using for this
> purpose. Maybe there's a better name though or we could move the class
> elsewhere.
> 
>> If we start adding more things in this file, then I see two options:
>>          - Either this is related to the current class, and thus the file could
>> be named meson_arg or something along those lines.
>>          - Or it is unrelated to the current class, and we end up with a file
>> coalescing random bits of code, which is usually a bit dirty in OOP.
>>
> 
> It's code that's reused in multiple places, I'm not sure whether that
> qualifies as random bits of code. It could be in os_session.py (as
> that would work import-wise), but that's not a good place to put it,
> as the logic is actually utilized in sut_node.py. But putting it into
> sut_node.py doesn't work because of imports. Maybe we could just put
> it into utils.py in the framework dir, which is a very similar file,
> if not the same. My original thoughts were to have a file with common
> code in each package (if needed), depending on where the code is used
> (package level-wise), but it looks like we can just have this sort of
> common utilities on the top level.
> 
>> Like I said, it's a bit of a nitpick, but given it's an RFC I hope
>> you'll give it a thought in the next version.
> 
> I thought a lot about this before submitting this RFC, but I wanted
> someone to have a look at this exact thing - whether the common.py
> file makes sense and what is the better name, common.py or utils.py
> (which is why I have both in this patch). I'll move the MesonArgs
> class to the top level utils.py and remove the common.py file as that
> makes the most sense to me now.
> 
> If you have any recommendations we may be able to make this even better.

I didn't meant to imply you did not think a lot about it, sorry if it 
came that way.
I prefer the idea of utils.py to a common.py, be it at package level or 
above. There might also be the option of __init__.py but I'm not sure 
about it.
That being said, I'm relatively new to dpdk and didn't know common.py 
was a common thing in EAL so I'll leave it up to you.