|SUCCESS| [v1, 2/2] dts: add build arguments to test run configuration

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164298

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-22

I will review this patch for DPDK DTS code coverage and build arguments features.

git sha: 20260522154637.952588-1-knimoji at iol.unh.edu
Author: Koushik Bhargav Nimoji <knimoji at iol.unh.edu>
Subject: [PATCH v1 1/2] dts: add code coverage reporting to DTS

This patch adds command line argument support for building DPDK with code
coverage enabled, allowing users to create and view code coverage reports
during DTS runs.

> diff --git a/dts/framework/remote_session/dpdk.py b/dts/framework/remote_session/dpdk.py
> index c3575cfcaf..865f97f6ca 100644
> --- a/dts/framework/remote_session/dpdk.py
> +++ b/dts/framework/remote_session/dpdk.py
> @@ -107,7 +108,22 @@ def teardown(self) -> None:
>          """Teardown the DPDK build on the target node.
>  
>          Removes the DPDK tree and/or build directory/tarball depending on the configuration.
> +        If code coverage is enabled, the coverage report and .info file are generated and
> +        copied onto the local filesystem before teardown.
>          """
> +        if SETTINGS.code_coverage:
> +            report_folder = PurePath(self.remote_dpdk_build_dir / "meson-logs")
> +            output_dir = SETTINGS.output_dir
> +            Path(output_dir).mkdir(parents=True, exist_ok=True)
> +
> +            coverage_status = self._session.generate_coverage_report(self.remote_dpdk_build_dir)

Does this code properly handle errors if generate_coverage_report() fails
to create the report? The function returns False on failure, but the
subsequent copy_dir_from() call would still attempt to copy a potentially
non-existent directory.

> +            if coverage_status:
> +                self._session.copy_dir_from(report_folder, output_dir)
> +                self._logger.info(
> +                    "Coverage HTML report generated, "
> +                    f"available at {output_dir}/meson-logs/coveragereports/index.html"
> +                )

The log message shows "coveragereports" but the ninja target generates
"coveragereport" (without the 's'). Does the path match what ninja
coverage-html actually produces?

[ ... ]

> diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
> index 158325bb7f..d2440dc2d8 100644
> --- a/dts/framework/remote_session/remote_session.py
> +++ b/dts/framework/remote_session/remote_session.py
> @@ -252,7 +252,10 @@ def copy_from(self, source_file: str | PurePath, destination_dir: str | Path) ->
>              destination_dir: The directory path on the local filesystem where the `source_file`
>                  will be saved.
>          """
> -        self.session.get(str(source_file), str(destination_dir))
> +        source_file = PurePath(source_file)
> +        destination_dir = Path(destination_dir)
> +        local_path = destination_dir / source_file.name
> +        self.session.get(str(source_file), str(local_path))

This changes the copy_from() behavior - the original code copied to a
directory, but the new code copies to a specific file path. Does this code
break existing callers that expect directory behavior? The method is named
copy_from() but the parameter is named source_file - does this handle
copying directories as the code coverage feature requires?

[ ... ]

> diff --git a/dts/framework/testbed_model/posix_session.py b/dts/framework/testbed_model/posix_session.py
> index dec952685a..c021000a29 100644
> --- a/dts/framework/testbed_model/posix_session.py
> +++ b/dts/framework/testbed_model/posix_session.py
> @@ -295,6 +295,23 @@ def build_dpdk(
>          except RemoteCommandExecutionError as e:
>              raise DPDKBuildError(f"DPDK build failed when doing '{e.command}'.")
>  
> +    def generate_coverage_report(self, remote_build_dir: PurePath | None):
> +        """Overrides :meth:`~.os_session.OSSession.generate_coverage_report`."""
> +        lcov_version = float(self.send_command(r"lcov --version | grep -oP '\d+\.\d+'").stdout)
> +        gcov_version = float(
> +            self.send_command(
> +                r"gcov --version | head -n 1 | grep -oP '\d+\.\d+' | tail -n 1"
> +            ).stdout
> +        )

Does this code handle command failures if lcov or gcov are not installed?
The send_command() calls could raise exceptions, and there's no error
handling here. Also, does float() conversion work correctly for version
strings like "1.15" that could have more than one decimal point in the
full version string?

> +        if lcov_version == 1.15 and gcov_version >= 8.0:
> +            self.send_command(f"ninja -C {remote_build_dir} coverage-html", timeout=600)
> +            return True
> +        else:
> +            self._logger.info(
> +                "Unable to generate code coverage report, ensure lcov v1.5 and at least gcov v8.0"

The log message says "v1.5" but the check is for version 1.15. Does this
message have a typo?

> +            )
> +            return False

[ ... ]

> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index 9917ffbfaa..38da88cd9c 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -125,6 +125,14 @@ def __str__(self) -> str:
>          """The actual args."""
>          return " ".join(f"{self._default_library} {self._dpdk_args}".split())
>  
> +    def _add_arg(self, arg: str):

Does this method need a return type annotation? According to DPDK coding
standards, all methods should have complete type hints. The method appears
to return None implicitly.

> +        """Used to add a meson build argument to the DPDK build.
> +
> +        Args:
> +            arg: The meson build argument to be added.
> +        """
> +        self._dpdk_args = self._dpdk_args + " " + arg

---

git sha: 20260522154637.952588-2-knimoji at iol.unh.edu
Author: Koushik Bhargav Nimoji <knimoji at iol.unh.edu>
Subject: [PATCH v1 2/2] dts: add build arguments to test run configuration

This patch adds the ability to specify build arguments when building DPDK
through DTS, allowing for more configurable DTS runs.

> diff --git a/dts/framework/config/test_run.py b/dts/framework/config/test_run.py
> index 76e24d1785..eab12041fc 100644
> --- a/dts/framework/config/test_run.py
> +++ b/dts/framework/config/test_run.py
> @@ -191,6 +191,8 @@ class DPDKBuildOptionsConfiguration(FrozenModel):
>      #: This string will be put in front of the compiler when executing the build. Useful for adding
>      #: wrapper commands, such as ``ccache``.
>      compiler_wrapper: str = ""
> +    #: The build arguments to build dpdk with
> +    build_args: dict[str, list[str]] = {}

Does this mutable default value create issues? Python's mutable default
arguments are shared across all instances. Should this use a Pydantic
Field with default_factory instead?

[ ... ]

> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index 38da88cd9c..e0ed35066c 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -99,10 +99,16 @@ class MesonArgs:
>  
>      _default_library: str
>  
> -    def __init__(self, default_library: str | None = None, **dpdk_args: str | bool):
> +    def __init__(
> +        self,
> +        dpdk_build_args: dict[str, list[str]],
> +        default_library: str | None = None,
> +        **dpdk_args: str | bool,
> +    ):
>          """Initialize the meson arguments.
>  
>          Args:
> +            dpdk_build_args: The DPDK build arguments specified in the test run configuration file.
>              default_library: The default library type, Meson supports ``shared``, ``static`` and
>                  ``both``. Defaults to :data:`None`, in which case the argument won't be used.
>              dpdk_args: The arguments found in ``meson_options.txt`` in root DPDK directory.
> @@ -121,6 +127,19 @@ def __init__(self, default_library: str | None = None, **dpdk_args: str | bool):
>              )
>          )
>  
> +        arguments = []
> +        for option, value in dpdk_build_args.items():
> +            if option == "c_args":
> +                values = " ".join(f"-{val}" for val in value)
> +                arguments.append(f'-D{option}="{values}"')
> +            elif option == "flags":
> +                values = " ".join(f"--{val}" for val in value)
> +                arguments.append(values)
> +            else:
> +                arguments.append(f" -D{option}={value[0]}")

Does this code properly handle empty lists in the value? If value is an
empty list, then value[0] would raise an IndexError. Also, does the code
only use the first element with value[0] when multiple values are provided
for non-c_args/non-flags options?

> +
> +        self._dpdk_args = " ".join(arguments)

Does this overwrite the existing _dpdk_args that were constructed from the
**dpdk_args parameter? The code builds _dpdk_args from **dpdk_args above,
then immediately replaces it here. Does this break compatibility with
existing callers that rely on the **dpdk_args parameters?