[RFC,v2,03/10] dts: add dpdk build on sut

  Add the ability to build DPDK and apps, using a configured target.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/exception.py                    |  17 +++
 dts/framework/remote_session/os/os_session.py |  90 +++++++++++-
 .../remote_session/os/posix_session.py        | 128 +++++++++++++++++
 .../remote_session/remote_session.py          |  34 ++++-
 dts/framework/remote_session/ssh_session.py   |  64 ++++++++-
 dts/framework/settings.py                     |  40 +++++-
 dts/framework/testbed_model/node/sut_node.py  | 131 ++++++++++++++++++
 dts/framework/utils.py                        |  15 ++
 8 files changed, 505 insertions(+), 14 deletions(-)
  

On Mon, Nov 14, 2022 at 11:54 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> Add the ability to build DPDK and apps, using a configured target.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>  dts/framework/exception.py                    |  17 +++
>  dts/framework/remote_session/os/os_session.py |  90 +++++++++++-
>  .../remote_session/os/posix_session.py        | 128 +++++++++++++++++
>  .../remote_session/remote_session.py          |  34 ++++-
>  dts/framework/remote_session/ssh_session.py   |  64 ++++++++-
>  dts/framework/settings.py                     |  40 +++++-
>  dts/framework/testbed_model/node/sut_node.py  | 131 ++++++++++++++++++
>  dts/framework/utils.py                        |  15 ++
>  8 files changed, 505 insertions(+), 14 deletions(-)
>
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> index b282e48198..93d99432ae 100644
> --- a/dts/framework/exception.py
> +++ b/dts/framework/exception.py
> @@ -26,6 +26,7 @@ class ReturnCode(IntEnum):
>      GENERIC_ERR = 1
>      SSH_ERR = 2
>      REMOTE_CMD_EXEC_ERR = 3
> +    DPDK_BUILD_ERR = 10
>      NODE_SETUP_ERR = 20
>      NODE_CLEANUP_ERR = 21
>
> @@ -110,6 +111,22 @@ def __str__(self) -> str:
>          )
>
>
> +class RemoteDirectoryExistsError(DTSError):
> +    """
> +    Raised when a remote directory to be created already exists.
> +    """
> +
> +    return_code: ClassVar[ReturnCode] = ReturnCode.REMOTE_CMD_EXEC_ERR
> +
> +
> +class DPDKBuildError(DTSError):
> +    """
> +    Raised when DPDK build fails for any reason.
> +    """
> +
> +    return_code: ClassVar[ReturnCode] = ReturnCode.DPDK_BUILD_ERR
> +
> +
>  class NodeSetupError(DTSError):
>      """
>      Raised when setting up a node.
> diff --git a/dts/framework/remote_session/os/os_session.py
> b/dts/framework/remote_session/os/os_session.py
> index 2a72082628..57e2865282 100644
> --- a/dts/framework/remote_session/os/os_session.py
> +++ b/dts/framework/remote_session/os/os_session.py
> @@ -2,12 +2,15 @@
>  # Copyright(c) 2022 PANTHEON.tech s.r.o.
>  # Copyright(c) 2022 University of New Hampshire
>
> -from abc import ABC
> +from abc import ABC, abstractmethod
> +from pathlib import PurePath
>
> -from framework.config import NodeConfiguration
> +from framework.config import Architecture, NodeConfiguration
>  from framework.logger import DTSLOG
>  from framework.remote_session.factory import create_remote_session
>  from framework.remote_session.remote_session import RemoteSession
> +from framework.settings import SETTINGS
> +from framework.utils import EnvVarsDict
>
>
>  class OSSession(ABC):
> @@ -44,3 +47,86 @@ def is_alive(self) -> bool:
>          Check whether the remote session is still responding.
>          """
>          return self.remote_session.is_alive()
> +
> +    @abstractmethod
> +    def guess_dpdk_remote_dir(self, remote_dir) -> PurePath:
> +        """
> +        Try to find DPDK remote dir in remote_dir.
> +        """
> +
> +    @abstractmethod
> +    def get_remote_tmp_dir(self) -> PurePath:
> +        """
> +        Get the path of the temporary directory of the remote OS.
> +        """
> +
> +    @abstractmethod
> +    def get_dpdk_build_env_vars(self, arch: Architecture) -> dict:
> +        """
> +        Create extra environment variables needed for the target
> architecture. Get
> +        information from the node if needed.
> +        """
> +
> +    @abstractmethod
> +    def join_remote_path(self, *args: str | PurePath) -> PurePath:
> +        """
> +        Join path parts using the path separator that fits the remote OS.
> +        """
> +
> +    @abstractmethod
> +    def copy_file(
> +        self,
> +        source_file: str | PurePath,
> +        destination_file: str | PurePath,
> +        source_remote: bool = False,
> +    ) -> None:
> +        """
> +        Copy source_file from local storage to destination_file on the
> remote Node
> +        associated with the remote session.
> +        If source_remote is True, reverse the direction - copy
> source_file from the
> +        associated remote Node to destination_file on local storage.
> +        """
> +
> +    @abstractmethod
> +    def remove_remote_dir(
> +        self,
> +        remote_dir_path: str | PurePath,
> +        recursive: bool = True,
> +        force: bool = True,
> +    ) -> None:
> +        """
> +        Remove remote directory, by default remove recursively and
> forcefully.
> +        """
> +
> +    @abstractmethod
> +    def extract_remote_tarball(
> +        self,
> +        remote_tarball_path: str | PurePath,
> +        expected_dir: str | PurePath | None = None,
> +    ) -> None:
> +        """
> +        Extract remote tarball in place. If expected_dir is a non-empty
> string, check
> +        whether the dir exists after extracting the archive.
> +        """
> +
> +    @abstractmethod
> +    def build_dpdk(
> +        self,
> +        env_vars: EnvVarsDict,
> +        meson_args: str,
> +        remote_dpdk_dir: str | PurePath,
> +        target_name: str,
> +        rebuild: bool = False,
> +        timeout: float = SETTINGS.compile_timeout,
> +    ) -> PurePath:
>

I think that we should consider having a MesonArgs type which implements
the builder pattern. That way common things like static vs dynamic linking,
enabling lto, setting the optimization level, et can be handled via
dedicated methods, and then we can add a method on that which is "add this
string onto the end". This would also allow defining additional methods for
DPDK-specific meson arguments, like only enabling
certain drivers/applications/tests or forcing certain vector widths. I
would also like to see an option to make use of ccache, because currently
the only way I see to do that is via environment variables, which will make
creating a test matrix that includes multiple compilers difficult.


> +        """
> +        Build DPDK in the input dir with specified environment variables
> and meson
> +        arguments.
> +        Return the directory path where DPDK was built.
> +        """
> +
> +    @abstractmethod
> +    def get_dpdk_version(self, version_path: str | PurePath) -> str:
> +        """
> +        Inspect DPDK version on the remote node from version_path.
> +        """
> diff --git a/dts/framework/remote_session/os/posix_session.py
> b/dts/framework/remote_session/os/posix_session.py
> index 9622a4ea30..a36b8e8c1a 100644
> --- a/dts/framework/remote_session/os/posix_session.py
> +++ b/dts/framework/remote_session/os/posix_session.py
> @@ -2,6 +2,13 @@
>  # Copyright(c) 2022 PANTHEON.tech s.r.o.
>  # Copyright(c) 2022 University of New Hampshire
>
> +from pathlib import PurePath, PurePosixPath
> +
> +from framework.config import Architecture
> +from framework.exception import DPDKBuildError,
> RemoteCommandExecutionError
> +from framework.settings import SETTINGS
> +from framework.utils import EnvVarsDict
> +
>  from .os_session import OSSession
>
>
> @@ -10,3 +17,124 @@ class PosixSession(OSSession):
>      An intermediary class implementing the Posix compliant parts of
>      Linux and other OS remote sessions.
>      """
> +
> +    @staticmethod
> +    def combine_short_options(**opts: [str, bool]) -> str:
> +        ret_opts = ""
> +        for opt, include in opts.items():
> +            if include:
> +                ret_opts = f"{ret_opts}{opt}"
> +
> +        if ret_opts:
> +            ret_opts = f" -{ret_opts}"
> +
> +        return ret_opts
> +
> +    def guess_dpdk_remote_dir(self, remote_dir) -> PurePosixPath:
> +        remote_guess = self.join_remote_path(remote_dir, "dpdk-*")
> +        result = self.remote_session.send_command(f"ls -d {remote_guess}
> | tail -1")
> +        return PurePosixPath(result.stdout)
> +
> +    def get_remote_tmp_dir(self) -> PurePosixPath:
> +        return PurePosixPath("/tmp")
> +
> +    def get_dpdk_build_env_vars(self, arch: Architecture) -> dict:
> +        """
> +        Create extra environment variables needed for i686 arch build.
> Get information
> +        from the node if needed.
> +        """
> +        env_vars = {}
> +        if arch == Architecture.i686:
> +            # find the pkg-config path and store it in PKG_CONFIG_LIBDIR
> +            out = self.remote_session.send_command("find /usr -type d
> -name pkgconfig")
> +            pkg_path = ""
> +            res_path = out.stdout.split("\r\n")
> +            for cur_path in res_path:
> +                if "i386" in cur_path:
> +                    pkg_path = cur_path
> +                    break
> +            assert pkg_path != "", "i386 pkg-config path not found"
> +
> +            env_vars["CFLAGS"] = "-m32"
> +            env_vars["PKG_CONFIG_LIBDIR"] = pkg_path
> +
> +        return env_vars
> +
> +    def join_remote_path(self, *args: str | PurePath) -> PurePosixPath:
> +        return PurePosixPath(*args)
> +
> +    def copy_file(
> +        self,
> +        source_file: str | PurePath,
> +        destination_file: str | PurePath,
> +        source_remote: bool = False,
> +    ) -> None:
> +        self.remote_session.copy_file(source_file, destination_file,
> source_remote)
> +
> +    def remove_remote_dir(
> +        self,
> +        remote_dir_path: str | PurePath,
> +        recursive: bool = True,
> +        force: bool = True,
> +    ) -> None:
> +        opts = PosixSession.combine_short_options(r=recursive, f=force)
> +        self.remote_session.send_command(f"rm{opts} {remote_dir_path}")
> +
> +    def extract_remote_tarball(
> +        self,
> +        remote_tarball_path: str | PurePath,
> +        expected_dir: str | PurePath | None = None,
> +    ) -> None:
> +        self.remote_session.send_command(
> +            f"tar xfm {remote_tarball_path} "
> +            f"-C {PurePosixPath(remote_tarball_path).parent}",
> +            60,
> +        )
> +        if expected_dir:
> +            self.remote_session.send_command(f"ls {expected_dir}",
> verify=True)
> +
> +    def build_dpdk(
> +        self,
> +        env_vars: EnvVarsDict,
> +        meson_args: str,
> +        remote_dpdk_dir: str | PurePath,
> +        target_name: str,
> +        rebuild: bool = False,
> +        timeout: float = SETTINGS.compile_timeout,
> +    ) -> PurePosixPath:
> +        build_dir = self.join_remote_path(remote_dpdk_dir, target_name)
> +        try:
> +            if rebuild:
> +                # reconfigure, then build
> +                self.logger.info("Reconfiguring DPDK build.")
> +                self.remote_session.send_command(
> +                    f"meson configure {meson_args} {build_dir}",
> +                    timeout,
> +                    verify=True,
> +                    env=env_vars,
> +                )
> +            else:
> +                # fresh build - remove target dir first, then build from
> scratch
> +                self.logger.info("Configuring DPDK build from scratch.")
> +                self.remove_remote_dir(build_dir)
> +                self.remote_session.send_command(
> +                    f"meson {meson_args} {remote_dpdk_dir} {build_dir}",
> +                    timeout,
> +                    verify=True,
> +                    env=env_vars,
> +                )
> +
> +            self.logger.info("Building DPDK.")
> +            self.remote_session.send_command(
> +                f"ninja -C {build_dir}", timeout, verify=True,
> env=env_vars
> +            )
> +        except RemoteCommandExecutionError as e:
> +            raise DPDKBuildError(f"DPDK build failed when doing
> '{e.command}'.")
> +
> +        return build_dir
> +
> +    def get_dpdk_version(self, build_dir: str | PurePath) -> str:
> +        out = self.remote_session.send_command(
> +            f"cat {self.join_remote_path(build_dir, 'VERSION')}",
> verify=True
> +        )
> +        return out.stdout
> diff --git a/dts/framework/remote_session/remote_session.py
> b/dts/framework/remote_session/remote_session.py
> index fccd80a529..f10b1023f8 100644
> --- a/dts/framework/remote_session/remote_session.py
> +++ b/dts/framework/remote_session/remote_session.py
> @@ -10,6 +10,7 @@
>  from framework.exception import RemoteCommandExecutionError
>  from framework.logger import DTSLOG
>  from framework.settings import SETTINGS
> +from framework.utils import EnvVarsDict
>
>
>  @dataclasses.dataclass(slots=True, frozen=True)
> @@ -83,15 +84,22 @@ def _connect(self) -> None:
>          """
>
>      def send_command(
> -        self, command: str, timeout: float = SETTINGS.timeout, verify:
> bool = False
> +        self,
> +        command: str,
> +        timeout: float = SETTINGS.timeout,
> +        verify: bool = False,
> +        env: EnvVarsDict | None = None,

     ) -> CommandResult:
>          """
> -        Send a command to the connected node and return CommandResult.
> +        Send a command to the connected node using optional env vars
> +        and return CommandResult.
>          If verify is True, check the return code of the executed command
>          and raise a RemoteCommandExecutionError if the command failed.
>          """
> -        self.logger.info(f"Sending: '{command}'")
> -        result = self._send_command(command, timeout)
> +        self.logger.info(
> +            f"Sending: '{command}'" + (f" with env vars: '{env}'" if env
> else "")
> +        )
> +        result = self._send_command(command, timeout, env)
>          if verify and result.return_code:
>              self.logger.debug(
>                  f"Command '{command}' failed with return code
> '{result.return_code}'"
> @@ -104,9 +112,12 @@ def send_command(
>          return result
>
>      @abstractmethod
> -    def _send_command(self, command: str, timeout: float) ->
> CommandResult:
> +    def _send_command(
> +        self, command: str, timeout: float, env: EnvVarsDict | None
> +    ) -> CommandResult:
>          """
> -        Use the underlying protocol to execute the command and return
> CommandResult.
> +        Use the underlying protocol to execute the command using optional
> env vars
> +        and return CommandResult.
>          """
>
>      def close(self, force: bool = False) -> None:
> @@ -127,3 +138,14 @@ def is_alive(self) -> bool:
>          """
>          Check whether the remote session is still responding.
>          """
> +
> +    @abstractmethod
> +    def copy_file(
> +        self, source_file: str, destination_file: str, source_remote:
> bool = False
> +    ) -> None:
> +        """
> +        Copy source_file from local storage to destination_file on the
> remote Node
>

This should clarify that local storage means inside of the DTS container,
not the system it is running on.


> +        associated with the remote session.
> +        If source_remote is True, reverse the direction - copy
> source_file from the
> +        associated Node to destination_file on local storage.
> +        """
> diff --git a/dts/framework/remote_session/ssh_session.py
> b/dts/framework/remote_session/ssh_session.py
> index fb2f01dbc1..d4a6714e6b 100644
> --- a/dts/framework/remote_session/ssh_session.py
> +++ b/dts/framework/remote_session/ssh_session.py
> @@ -5,12 +5,13 @@
>
>  import time
>
> +import pexpect  # type: ignore
>  from pexpect import pxssh  # type: ignore
>
>  from framework.config import NodeConfiguration
>  from framework.exception import SSHConnectionError, SSHSessionDeadError,
> SSHTimeoutError
>  from framework.logger import DTSLOG
> -from framework.utils import GREEN, RED
> +from framework.utils import GREEN, RED, EnvVarsDict
>
>  from .remote_session import CommandResult, RemoteSession
>
> @@ -163,16 +164,22 @@ def _flush(self) -> None:
>      def is_alive(self) -> bool:
>          return self.session.isalive()
>
> -    def _send_command(self, command: str, timeout: float) ->
> CommandResult:
> -        output = self._send_command_get_output(command, timeout)
> -        return_code = int(self._send_command_get_output("echo $?",
> timeout))
> +    def _send_command(
> +        self, command: str, timeout: float, env: EnvVarsDict | None
> +    ) -> CommandResult:
> +        output = self._send_command_get_output(command, timeout, env)
> +        return_code = int(self._send_command_get_output("echo $?",
> timeout, None))
>
>          # we're capturing only stdout
>          return CommandResult(self.name, command, output, "", return_code)
>
> -    def _send_command_get_output(self, command: str, timeout: float) ->
> str:
> +    def _send_command_get_output(
> +        self, command: str, timeout: float, env: EnvVarsDict | None
> +    ) -> str:
>          try:
>              self._clean_session()
> +            if env:
> +                command = f"{env} {command}"
>              self._send_line(command)
>          except Exception as e:
>              raise e
> @@ -189,3 +196,50 @@ def _close(self, force: bool = False) -> None:
>          else:
>              if self.is_alive():
>                  self.session.logout()
> +
> +    def copy_file(
> +        self, source_file: str, destination_file: str, source_remote:
> bool = False
> +    ) -> None:
> +        """
> +        Send a local file to a remote host.
> +        """
> +        if source_remote:
> +            source_file = f"{self.username}@{self.ip}:{source_file}"
> +        else:
> +            destination_file = f"{self.username}@
> {self.ip}:{destination_file}"
> +
> +        port = ""
> +        if self.port:
> +            port = f" -P {self.port}"
> +
> +        # this is not OS agnostic, find a Pythonic (and thus OS agnostic)
> way
> +        # TODO Fabric should handle this
> +        command = (
> +            f"scp -v{port} -o NoHostAuthenticationForLocalhost=yes"
> +            f" {source_file} {destination_file}"
> +        )
> +
> +        self._spawn_scp(command)
> +
> +    def _spawn_scp(self, scp_cmd: str) -> None:
> +        """
> +        Transfer a file with SCP
> +        """
> +        self.logger.info(scp_cmd)
> +        p: pexpect.spawn = pexpect.spawn(scp_cmd)
> +        time.sleep(0.5)
> +        ssh_newkey: str = "Are you sure you want to continue connecting"
> +        i: int = p.expect(
> +            [ssh_newkey, "[pP]assword", "# ", pexpect.EOF,
> pexpect.TIMEOUT], 120
> +        )
> +        if i == 0:  # add once in trust list
> +            p.sendline("yes")
> +            i = p.expect([ssh_newkey, "[pP]assword", pexpect.EOF], 2)
> +
> +        if i == 1:
> +            time.sleep(0.5)
> +            p.sendline(self.password)
> +            p.expect("Exit status 0", 60)
> +        if i == 4:
> +            self.logger.error("SCP TIMEOUT error %d" % i)
> +        p.close()
> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index 800f2c7b7f..e2bf3d2ce4 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py
> @@ -7,6 +7,7 @@
>  import os
>  from collections.abc import Callable, Iterable, Sequence
>  from dataclasses import dataclass
> +from pathlib import Path
>  from typing import Any, TypeVar
>
>  _T = TypeVar("_T")
> @@ -60,6 +61,9 @@ class _Settings:
>      output_dir: str
>      timeout: float
>      verbose: bool
> +    skip_setup: bool
> +    dpdk_ref: Path
> +    compile_timeout: float
>
>
>  def _get_parser() -> argparse.ArgumentParser:
> @@ -88,6 +92,7 @@ def _get_parser() -> argparse.ArgumentParser:
>          "--timeout",
>          action=_env_arg("DTS_TIMEOUT"),
>          default=15,
> +        type=float,
>          required=False,
>          help="[DTS_TIMEOUT] The default timeout for all DTS operations
> except for "
>          "compiling DPDK.",
> @@ -103,6 +108,36 @@ def _get_parser() -> argparse.ArgumentParser:
>          "to the console.",
>      )
>
> +    parser.add_argument(
> +        "-s",
> +        "--skip-setup",
> +        action=_env_arg("DTS_SKIP_SETUP"),
> +        required=False,
> +        help="[DTS_SKIP_SETUP] Set to 'Y' to skip all setup steps on SUT
> and TG nodes.",
> +    )
> +
> +    parser.add_argument(
> +        "--dpdk-ref",
> +        "--git",
> +        "--snapshot",
> +        action=_env_arg("DTS_DPDK_REF"),
> +        default="dpdk.tar.xz",
> +        type=Path,
> +        required=False,
> +        help="[DTS_DPDK_REF] Reference to DPDK source code, "
> +        "can be either a path to a tarball or a git refspec. "
> +        "In case of a tarball, it will be extracted in the same
> directory.",
> +    )
> +
> +    parser.add_argument(
> +        "--compile-timeout",
> +        action=_env_arg("DTS_COMPILE_TIMEOUT"),
> +        default=1200,
> +        type=float,
> +        required=False,
> +        help="[DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK.",
> +    )
> +
>      return parser
>
>
> @@ -111,8 +146,11 @@ def _get_settings() -> _Settings:
>      return _Settings(
>          config_file_path=parsed_args.config_file,
>          output_dir=parsed_args.output_dir,
> -        timeout=float(parsed_args.timeout),
> +        timeout=parsed_args.timeout,
>          verbose=(parsed_args.verbose == "Y"),
> +        skip_setup=(parsed_args.skip_setup == "Y"),
> +        dpdk_ref=parsed_args.dpdk_ref,
> +        compile_timeout=parsed_args.compile_timeout,
>      )
>
>
> diff --git a/dts/framework/testbed_model/node/sut_node.py
> b/dts/framework/testbed_model/node/sut_node.py
> index 79d54585c9..53268a7565 100644
> --- a/dts/framework/testbed_model/node/sut_node.py
> +++ b/dts/framework/testbed_model/node/sut_node.py
> @@ -2,6 +2,14 @@
>  # Copyright(c) 2010-2014 Intel Corporation
>  # Copyright(c) 2022 PANTHEON.tech s.r.o.
>
> +import os
> +import tarfile
> +from pathlib import PurePath
> +
> +from framework.config import BuildTargetConfiguration, NodeConfiguration
> +from framework.settings import SETTINGS
> +from framework.utils import EnvVarsDict, skip_setup
> +
>  from .node import Node
>
>
> @@ -10,4 +18,127 @@ class SutNode(Node):
>      A class for managing connections to the System under Test, providing
>      methods that retrieve the necessary information about the node (such
> as
>      cpu, memory and NIC details) and configuration capabilities.
> +    Another key capability is building DPDK according to given build
> target.
>      """
> +
> +    _build_target_config: BuildTargetConfiguration | None
> +    _env_vars: EnvVarsDict
> +    _remote_tmp_dir: PurePath
> +    __remote_dpdk_dir: PurePath | None
> +    _app_compile_timeout: float
> +
> +    def __init__(self, node_config: NodeConfiguration):
> +        super(SutNode, self).__init__(node_config)
> +        self._build_target_config = None
> +        self._env_vars = EnvVarsDict()
> +        self._remote_tmp_dir = self.main_session.get_remote_tmp_dir()
> +        self.__remote_dpdk_dir = None
> +        self._app_compile_timeout = 90
> +
> +    @property
> +    def _remote_dpdk_dir(self) -> PurePath:
> +        if self.__remote_dpdk_dir is None:
> +            self.__remote_dpdk_dir = self._guess_dpdk_remote_dir()
> +        return self.__remote_dpdk_dir
> +
> +    @_remote_dpdk_dir.setter
> +    def _remote_dpdk_dir(self, value: PurePath) -> None:
> +        self.__remote_dpdk_dir = value
> +
> +    def _guess_dpdk_remote_dir(self) -> PurePath:
> +        return
> self.main_session.guess_dpdk_remote_dir(self._remote_tmp_dir)
> +
> +    def _setup_build_target(
> +        self, build_target_config: BuildTargetConfiguration
> +    ) -> None:
> +        """
> +        Setup DPDK on the SUT node.
> +        """
> +        self._configure_build_target(build_target_config)
> +        self._copy_dpdk_tarball()
> +        self._build_dpdk()
> +
> +    def _configure_build_target(
> +        self, build_target_config: BuildTargetConfiguration
> +    ) -> None:
> +        """
> +        Populate common environment variables and set build target config.
> +        """
> +        self._build_target_config = build_target_config
> +        self._env_vars.update(
> +
> self.main_session.get_dpdk_build_env_vars(build_target_config.arch)
> +        )
> +        self._env_vars["CC"] = build_target_config.compiler.name
> +
> +    @skip_setup
> +    def _copy_dpdk_tarball(self) -> None:
> +        """
> +        Copy to and extract DPDK tarball on the SUT node.
> +        """
> +        # check local path
> +        assert SETTINGS.dpdk_ref.exists(), f"Package {SETTINGS.dpdk_ref}
> doesn't exist."
> +
> +        self.logger.info("Copying DPDK tarball to SUT.")
> +        self.main_session.copy_file(SETTINGS.dpdk_ref,
> self._remote_tmp_dir)
> +
> +        # construct remote tarball path
> +        # the basename is the same on local host and on remote Node
> +        remote_tarball_path = self.main_session.join_remote_path(
> +            self._remote_tmp_dir, os.path.basename(SETTINGS.dpdk_ref)
> +        )
> +
> +        # construct remote path after extracting
> +        with tarfile.open(SETTINGS.dpdk_ref) as dpdk_tar:
> +            dpdk_top_dir = dpdk_tar.getnames()[0]
> +        self._remote_dpdk_dir = self.main_session.join_remote_path(
> +            self._remote_tmp_dir, dpdk_top_dir
> +        )
> +
> +        self.logger.info("Extracting DPDK tarball on SUT.")
>

Can we add a path to this log message?


> +        # clean remote path where we're extracting
> +        self.main_session.remove_remote_dir(self._remote_dpdk_dir)
> +
> +        # then extract to remote path
> +        self.main_session.extract_remote_tarball(
> +            remote_tarball_path, self._remote_dpdk_dir
> +        )
> +
> +    @skip_setup
> +    def _build_dpdk(self) -> None:
> +        """
> +        Build DPDK. Uses the already configured target. Assumes that the
> tarball has
> +        already been copied to and extracted on the SUT node.
> +        """
> +        meson_args = "-Denable_kmods=True -Dlibdir=lib
> --default-library=static"
> +        self.main_session.build_dpdk(
> +            self._env_vars,
> +            meson_args,
> +            self._remote_dpdk_dir,
> +            self._build_target_config.name if self._build_target_config
> else "build",
> +        )
> +        self.logger.info(
> +            f"DPDK version:
> {self.main_session.get_dpdk_version(self._remote_dpdk_dir)}"
> +        )
> +
> +    def build_dpdk_app(self, app_name: str) -> PurePath:
> +        """
> +        Build one or all DPDK apps. Requires DPDK to be already built on
> the SUT node.
> +        When app_name is 'all', build all example apps.
> +        When app_name is any other string, tries to build that example
> app.
> +        Return the directory path of the built app. If building all apps,
> return
> +        the path to the examples directory (where all apps reside).
> +        """
> +        meson_args = f"-Dexamples={app_name}"
> +        build_dir = self.main_session.build_dpdk(
> +            self._env_vars,
> +            meson_args,
> +            self._remote_dpdk_dir,
> +            self._build_target_config.name if self._build_target_config
> else "build",
> +            rebuild=True,
> +            timeout=self._app_compile_timeout,
> +        )
> +        if app_name == "all":
> +            return self.main_session.join_remote_path(build_dir,
> "examples")
> +        return self.main_session.join_remote_path(
> +            build_dir, "examples", f"dpdk-{app_name}"
> +        )
> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index c28c8f1082..91e58f3218 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -4,6 +4,9 @@
>  # Copyright(c) 2022 University of New Hampshire
>
>  import sys
> +from typing import Callable
> +
> +from framework.settings import SETTINGS
>
>
>  def check_dts_python_version() -> None:
> @@ -22,9 +25,21 @@ def check_dts_python_version() -> None:
>          print(RED("Please use Python >= 3.10 instead"), file=sys.stderr)
>
>
> +def skip_setup(func) -> Callable[..., None]:
> +    if SETTINGS.skip_setup:
> +        return lambda *args: None
> +    else:
> +        return func
> +
> +
>  def GREEN(text: str) -> str:
>      return f"\u001B[32;1m{str(text)}\u001B[0m"
>
>
>  def RED(text: str) -> str:
>      return f"\u001B[31;1m{str(text)}\u001B[0m"
> +
> +
> +class EnvVarsDict(dict):
> +    def __str__(self) -> str:
> +        return " ".join(["=".join(item) for item in self.items()])
>

This needs to make sure it doesn't silently run over the line length
limitations in posix sh/bash (4096 chars) or cmd (8191 chars). That would
be a VERY frustrating bug to track down and it can easily be stopped by
checking that this is a reasonable length (< 2k characters) and emitting a
warning if something goes over that.


> --
> 2.30.2
>
>
  

Apologies for removing recipients in my previous reply.

From: Owen Hilyard <ohilyard@iol.unh.edu>
Sent: Monday, November 21, 2022 1:35 PM
To: Juraj Linkeš <juraj.linkes@pantheon.tech>
Subject: Re: [RFC PATCH v2 03/10] dts: add dpdk build on sut

On Fri, Nov 18, 2022 at 7:24 AM Juraj Linkeš <juraj.linkes@pantheon.tech<mailto:juraj.linkes@pantheon.tech>> wrote:
A note: If I'm not mistaken, review should be done in plain text. I've formatted this as plain text and prefixed my replies with [Juraj].

+    @abstractmethod
+    def build_dpdk(
+        self,
+        env_vars: EnvVarsDict,
+        meson_args: str,
+        remote_dpdk_dir: str | PurePath,
+        target_name: str,
+        rebuild: bool = False,
+        timeout: float = SETTINGS.compile_timeout,
+    ) -> PurePath:

I think that we should consider having a MesonArgs type which implements the builder pattern. That way common things like static vs dynamic linking, enabling lto, setting the optimization level, et can be handled via dedicated methods, and then we can add a method on that which is "add this string onto the end". This would also allow defining additional methods for DPDK-specific meson arguments, like only enabling certain drivers/applications/tests or forcing certain vector widths. I would also like to see an option to make use of ccache, because currently the only way I see to do that is via environment variables, which will make creating a test matrix that includes multiple compilers difficult.

[Juraj] The MesonArgs type is a good suggestion, I'll do that.
[Juraj] We don't necessarily need ccache at this point, but it is very useful and it doesn't look like that big of an addition. How exactly should the implementation look like? Do we want configure something in the conf.yaml file? What to I need to add to meson invocation?

[Owen] I think that we probably want to have a setting in the conf.yaml file that creates a "compiler wrapper". You can either declare one for all compilers or declare one for some subset of compilers. I think putting it into the conf.yaml file makes sense.

executions:
    - build_targets:
        - arch: x86_64
          os: linux
          cpu: native
          compiler: gcc
          compiler_wrapper: ccache
        - arch: x86_64
          os: linux
          cpu: native
          compiler: icc
          compiler_wrapper: /usr/local/bin/my_super_special_compiler_wrapper
        - arch: x86_64
          os: linux
          cpu: native
          compiler: clang # clang doesn't need a wrapper for some reason



The only way that I know of to easily set the compiler in Meson is to set CC="<compiler_wrapper> <compiler>" for "meson setup". Also, you will need to completely wipe out the build directory between build targets due to meson not actually reconfiguring properly.

Ok, I'll modify the CC variable when compiler_wrapper is defined. It seems to be working, but may not be the cleanest implementation.
The current DPDK build works this way: The first DPDK build per build target is done from scratch and subsequent builds (currently application building) is done on top of that, so we should be fine on this front.

<snip>

+    @abstractmethod
+    def copy_file(
+        self, source_file: str, destination_file: str, source_remote: bool = False
+    ) -> None:
+        """
+        Copy source_file from local storage to destination_file on the remote Node

This should clarify that local storage means inside of the DTS container, not the system it is running on.

[Juraj] Ack. The local storage (I really should've said filesystem) could be any place where DTS is running, be it a container, a VM or a baremetal host. I think just changing local storage to local filesystem should be enough. If not, please propose an alternative wording.

[Juraj] And a related note - should we split copy_file into copy_file_to and copy_file_from?

<snip>

+    @skip_setup
+    def _copy_dpdk_tarball(self) -> None:
+        """
+        Copy to and extract DPDK tarball on the SUT node.
+        """
+        # check local path
+        assert SETTINGS.dpdk_ref.exists(), f"Package {SETTINGS.dpdk_ref} doesn't exist."
+
+        http://self.logger.info("Copying DPDK tarball to SUT.")
+        self.main_session.copy_file(SETTINGS.dpdk_ref, self._remote_tmp_dir)
+
+        # construct remote tarball path
+        # the basename is the same on local host and on remote Node
+        remote_tarball_path = self.main_session.join_remote_path(
+            self._remote_tmp_dir, os.path.basename(SETTINGS.dpdk_ref)
+        )
+
+        # construct remote path after extracting
+        with tarfile.open(SETTINGS.dpdk_ref) as dpdk_tar:
+            dpdk_top_dir = dpdk_tar.getnames()[0]
+        self._remote_dpdk_dir = self.main_session.join_remote_path(
+            self._remote_tmp_dir, dpdk_top_dir
+        )
+
+        http://self.logger.info("Extracting DPDK tarball on SUT.")

Can we add a path to this log message?

[Juraj] Absolutely, I'll add it. If there are more logs that would be useful to you, I'll add those as well (maybe as debugs).

<snip>

+class EnvVarsDict(dict):
+    def __str__(self) -> str:
+        return " ".join(["=".join(item) for item in self.items()])

This needs to make sure it doesn't silently run over the line length limitations in posix sh/bash (4096 chars) or cmd (8191 chars). That would be a VERY frustrating bug to track down and it can easily be stopped by checking that this is a reasonable length (< 2k characters) and emitting a warning if something goes over that.

[Juraj] Interesting, I didn't know about this. Would a warning be enough?
Also, Allowing less than 2k characters leaves us with at least 2k characters for the rest of the command and that should be plenty, but do we want to check that as well? If so, we may want to do the check when sending a commad. Another thing to consider is that we're going to switch to Fabric and we won't need to worry about this - it would be up to the underlying RemoteSession implementations to check this.

[Owen] A warning would probably be enough. "Another thing to consider is that we're going to switch to Fabric and we won't need to worry about this" We will need to worry about this if we are still exposing a bash shell in any way to the user.

Ok, I think we should note this and consider it when implementing Farbric. I don't think we'll be exposing shell at this point, but maybe that'll change when we need to handle DPDK applications - we should address this then I think.