From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B4BC6439FD; Mon, 29 Jan 2024 12:47:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3038B402C2; Mon, 29 Jan 2024 12:47:15 +0100 (CET) Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by mails.dpdk.org (Postfix) with ESMTP id 6AD2D4029A for ; Mon, 29 Jan 2024 12:47:13 +0100 (CET) Received: by mail-ej1-f41.google.com with SMTP id a640c23a62f3a-a354408e6bfso275979166b.1 for ; Mon, 29 Jan 2024 03:47:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1706528833; x=1707133633; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=i/zRmJeXFnRR/ZxGVKuBPRNq3lNNpeZfto99o6QG5nk=; b=Y0J3W7X/yo8sfGixvmHnyamVy5KIcd4wHJ5LiTsaOM/kVgr586i4slbHfcW7u2APC4 77PWihMr1nYHcjx7jexgNiJ/GE8vFIK/gIZsnyOYEfmGfDf7JWGSvQbDH52Q08ErZtxz s84l7MSrxf2VVfbGs2U94oEwT2+A+Ux3BAzAq5S+t+8sZ5Zev+5kqhL3sIHcDtej9RKY zPdzy+fHV02wrea/P7/0fKAr96jFGHOQ8vmqSc2suDnOzQ7SzHVQ0jcZJVgFHCnM+Igj MhPMCxLGANw7jxx6iBV1VpvDoBM+wCNpnkdgwjhcqc2Em685YOn98PxNzIUEa5uZQw8n cv2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706528833; x=1707133633; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=i/zRmJeXFnRR/ZxGVKuBPRNq3lNNpeZfto99o6QG5nk=; b=Fljz3It7RQ57HU6NzbkD/Cp/+yzXpC7xRCMeUTvKwmctpkDNFro+MOz1CwI1qDcZe4 sj6SxZpmebgDqBCfHyw6uEk/wqu132mdRlwhsfKXgHrxb+mETV3OmTFFnUXCn/Px1AVC Eok8QvZaGEdsyILs7WMCbFSdil+vUxncdqKtHz7TuQNzra0dOl2DNTku1zs00AQdEi+5 frDhgVMROVRbt/DcZY6U/FtZ3ymgiClxN2pOqMoi9iU9B0rOhbEk9Z/Xr+UD7t72fuwp kkeIiJn4sbkYYhQlsoRFQDsGukplhfD67DS78AwodDSUk7ykvYkk6Pp5MfqSYRh9u2GH A8Fw== X-Gm-Message-State: AOJu0Yxa7N9szFw8glv9QzGU4AlvI1uGVNkiY4I6gtBt3BgliqbD3V2r /FySwuS6WRN40QeIE7eMmSxZxji8F++NLCPRrbzG7aF/0DPI65Tm6z9PbwZNEgzvYPNWeyNGDla eq12rUqeKK4kQ/IPhSM10rfH1fcJsGG0H4XjQTA== X-Google-Smtp-Source: AGHT+IEaou9TqKBinzBr2HqOzspMVsNoQPKMI1oWd1bsmKzChei9FIngOov+U0twOLURTWWTOExzTUdiLuuYG5sbaaY= X-Received: by 2002:a17:907:1759:b0:a35:b59c:fc04 with SMTP id lf25-20020a170907175900b00a35b59cfc04mr2066513ejc.25.1706528832902; Mon, 29 Jan 2024 03:47:12 -0800 (PST) MIME-Version: 1.0 References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240122182611.1904974-2-luca.vizzarro@arm.com> In-Reply-To: <20240122182611.1904974-2-luca.vizzarro@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 29 Jan 2024 12:47:01 +0100 Message-ID: Subject: Re: [PATCH 1/4] dts: constrain DPDK source flag To: Luca Vizzarro Cc: dev@dpdk.org, Paul Szczepanek Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Jan 22, 2024 at 7:26=E2=80=AFPM Luca Vizzarro wrote: > > DTS needs an input to gather the DPDK source code from. This is then > built on the remote target. This commit makes sure that this input is > more constrained, separating the Git revision ID =E2=80=93 used to create= a > tarball using Git =E2=80=93 and providing tarballed source code directly,= while > retaining mutual exclusion. > I didn't see the mutual exclusion being enforced in the code. From what I can tell, I could pass both --tarball FILEPATH and --revision and the former would be used without checking the latter. > This makes the code more readable and easier to handle for input > validation, of which this commit introduces a basic one based on the > pre-existing code. > I wanted to have just one argument for basically the same thing, but the input is pretty different so a two argument solution actually sounds better. > Moreover it ensures that these flags are explicitly required to be set > by the user, dropping a default value. It also aids the user understand > how to use the DTS in the scenario it is ran without any arguments set. > > Reviewed-by: Paul Szczepanek > Signed-off-by: Luca Vizzarro > --- > doc/guides/tools/dts.rst | 8 +++-- > dts/framework/settings.py | 64 ++++++++++++++++++++++++++++----------- > dts/framework/utils.py | 43 ++++++++++++++++---------- > 3 files changed, 79 insertions(+), 36 deletions(-) > > diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst > index 846696e14e..6e2da317e8 100644 > --- a/doc/guides/tools/dts.rst > +++ b/doc/guides/tools/dts.rst > @@ -215,12 +215,16 @@ DTS is run with ``main.py`` located in the ``dts`` = directory after entering Poet > .. code-block:: console > > (dts-py3.10) $ ./main.py --help > - usage: main.py [-h] [--config-file CONFIG_FILE] [--output-dir OUTPUT_= DIR] [-t TIMEOUT] [-v] [-s] [--tarball TARBALL] [--compile-timeout COMPILE_= TIMEOUT] [--test-cases TEST_CASES] [--re-run RE_RUN] > + usage: main.py [-h] (--tarball FILEPATH | --revision ID) [--config-fi= le CONFIG_FILE] [--output-dir OUTPUT_DIR] [-t TIMEOUT] [-v] [-s] [--compile= -timeout COMPILE_TIMEOUT] [--test-cases TEST_CASES] [--re-run RE_RUN] > > Run DPDK test suites. All options may be specified with the environme= nt variables provided in brackets. Command line arguments have higher prior= ity. > > options: > -h, --help show this help message and exit > + --tarball FILEPATH, --snapshot FILEPATH > + Path to DPDK source code tarball to test. (defa= ult: None) > + --revision ID, --rev ID, --git-ref ID > + Git revision ID to test. Could be commit, tag, = tree ID and vice versa. To test local changes, first commit them, then use = their commit ID (default: None) > --config-file CONFIG_FILE > [DTS_CFG_FILE] configuration file that describe= s the test cases, SUTs and targets. (default: ./conf.yaml) > --output-dir OUTPUT_DIR, --output OUTPUT_DIR > @@ -229,8 +233,6 @@ DTS is run with ``main.py`` located in the ``dts`` di= rectory after entering Poet > [DTS_TIMEOUT] The default timeout for all DTS o= perations except for compiling DPDK. (default: 15) > -v, --verbose [DTS_VERBOSE] Specify to enable verbose output,= logging all messages to the console. (default: False) > -s, --skip-setup [DTS_SKIP_SETUP] Specify to skip all setup step= s on SUT and TG nodes. (default: None) > - --tarball TARBALL, --snapshot TARBALL, --git-ref TARBALL > - [DTS_DPDK_TARBALL] Path to DPDK source code tar= ball or a git commit ID, tag ID or tree ID to test. To test local changes, = first commit them, then use the commit ID with this option. (default: dpdk.= tar.xz) > --compile-timeout COMPILE_TIMEOUT > [DTS_COMPILE_TIMEOUT] The timeout for compiling= DPDK. (default: 1200) > --test-cases TEST_CASES > diff --git a/dts/framework/settings.py b/dts/framework/settings.py > index 609c8d0e62..2d0365e763 100644 > --- a/dts/framework/settings.py > +++ b/dts/framework/settings.py > @@ -76,7 +76,8 @@ > from pathlib import Path > from typing import Any, TypeVar > > -from .utils import DPDKGitTarball > +from .exception import ConfigurationError > +from .utils import DPDKGitTarball, get_commit_id > > _T =3D TypeVar("_T") > > @@ -149,6 +150,26 @@ def __call__( > return _EnvironmentArgument > > > +def _parse_tarball(filepath: str) -> Path: > + """Validate if the filepath is valid and return a Path object.""" whether `filepath` is valid Even though private methods don't get included in the API docs, I like to be consistent. In this case, it doesn't really detract (but maybe some disability would prove this wrong) while adding a bit (the fact that we're referencing the argument). I think the name should either be _validate_tarball or _parse_tarball_path. The argument name is two words, so let's put an underscore between them. > + path =3D Path(filepath) > + if not path.exists() or not path.is_file(): > + raise argparse.ArgumentTypeError( > + "the file path provided is not a valid file") Typo: capital T > + return path > + > + > +def _parse_revision_id(rev_id: str) -> str: > + """Retrieve effective commit ID from a revision ID. While validating= it.""" I think this would read better as one sentence. > + > + try: > + return get_commit_id(rev_id) > + except ConfigurationError: > + raise argparse.ArgumentTypeError( > + "the Git revision ID supplied is invalid or ambiguous" > + ) > + > + > @dataclass(slots=3DTrue) > class Settings: > """Default framework-wide user settings. > @@ -167,7 +188,7 @@ class Settings: > #: > skip_setup: bool =3D False > #: > - dpdk_tarball_path: Path | str =3D "dpdk.tar.xz" > + dpdk_tarball_path: Path | str =3D "" > #: > compile_timeout: float =3D 1200 > #: > @@ -186,6 +207,28 @@ def _get_parser() -> argparse.ArgumentParser: > formatter_class=3Dargparse.ArgumentDefaultsHelpFormatter, > ) > > + dpdk_source =3D parser.add_mutually_exclusive_group(required=3DTrue) > + > + dpdk_source.add_argument( > + "--tarball", > + "--snapshot", > + action=3D'store', > + type=3D_parse_tarball, > + help=3D"Path to DPDK source code tarball to test.", > + metavar=3D"FILEPATH", > + ) > + > + dpdk_source.add_argument( > + "--revision", > + "--rev", > + "--git-ref", > + type=3D_parse_revision_id, > + metavar=3D"ID", Since this is a patch with improvements, maybe we could add metavars to other arguments as well. It looks pretty good. > + help=3D"Git revision ID to test. Could be commit, tag, tree ID a= nd " > + "vice versa. To test local changes, first commit them, then use = their " > + "commit ID", > + ) > + This removes the support for environment variables. It's possible we don't want the support for these two arguments. Maybe we don't need the support for variables at all. I'm leaning towards supporting the env variables, but we probably should refactor how they're done. The easiest would be to not do them with action, but just modifying the default value if set. That would be a worthwhile improvement. > parser.add_argument( > "--config-file", > action=3D_env_arg("DTS_CFG_FILE"), > @@ -229,18 +272,6 @@ def _get_parser() -> argparse.ArgumentParser: > help=3D"[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT = and TG nodes.", > ) > > - parser.add_argument( > - "--tarball", > - "--snapshot", > - "--git-ref", > - action=3D_env_arg("DTS_DPDK_TARBALL"), > - default=3DSETTINGS.dpdk_tarball_path, > - type=3DPath, > - help=3D"[DTS_DPDK_TARBALL] Path to DPDK source code tarball or a= git commit ID, " > - "tag ID or tree ID to test. To test local changes, first commit = them, " > - "then use the commit ID with this option.", > - ) > - > parser.add_argument( > "--compile-timeout", > action=3D_env_arg("DTS_COMPILE_TIMEOUT"), > @@ -283,9 +314,8 @@ def get_settings() -> Settings: > verbose=3Dparsed_args.verbose, > skip_setup=3Dparsed_args.skip_setup, > dpdk_tarball_path=3DPath( > - Path(DPDKGitTarball(parsed_args.tarball, parsed_args.output_= dir)) > - if not os.path.exists(parsed_args.tarball) > - else Path(parsed_args.tarball) > + parsed_args.tarball or > + DPDKGitTarball(parsed_args.revision, parsed_args.output_dir) > ), > compile_timeout=3Dparsed_args.compile_timeout, > test_cases=3D(parsed_args.test_cases.split(",") if parsed_args.t= est_cases else []), > diff --git a/dts/framework/utils.py b/dts/framework/utils.py > index cc5e458cc8..dbbec8b00a 100644 > --- a/dts/framework/utils.py > +++ b/dts/framework/utils.py > @@ -70,6 +70,32 @@ def get_packet_summaries(packets: list[Packet]) -> str= : > return f"Packet contents: \n{packet_summaries}" > > > +def get_commit_id(rev_id: str) -> str: > + """Given a Git revision ID, return the corresponding commit ID. > + > + Args: > + rev_id: The Git revision ID. > + > + Raises: > + ConfigurationError: The ``git rev-parse`` command failed. Sugges= ting > + an invalid or ambiguous revision ID was supplied. This would also probably read better as one sentence. > + """ > + result =3D subprocess.run( > + ["git", "rev-parse", "--verify", rev_id], > + text=3DTrue, > + capture_output=3DTrue, > + ) > + if result.returncode !=3D 0: > + raise ConfigurationError( > + f"{rev_id} is neither a path to an existing DPDK " > + "archive nor a valid git reference.\n" > + f"Command: {result.args}\n" > + f"Stdout: {result.stdout}\n" > + f"Stderr: {result.stderr}" > + ) We shuffled the order of operations a bit and now the error message doesn't correspond. > + return result.stdout.strip() > + > + > class StrEnum(Enum): > """Enum with members stored as strings.""" > > @@ -170,7 +196,6 @@ def __init__( > > self._tarball_dir =3D Path(output_dir, "tarball") > > - self._get_commit_id() Makes sense to move this outside. > self._create_tarball_dir() > > self._tarball_name =3D ( > @@ -180,21 +205,7 @@ def __init__( > if not self._tarball_path: > self._create_tarball() > > - def _get_commit_id(self) -> None: > - result =3D subprocess.run( > - ["git", "rev-parse", "--verify", self._git_ref], > - text=3DTrue, > - capture_output=3DTrue, > - ) > - if result.returncode !=3D 0: > - raise ConfigurationError( > - f"{self._git_ref} is neither a path to an existing DPDK = " > - "archive nor a valid git reference.\n" > - f"Command: {result.args}\n" > - f"Stdout: {result.stdout}\n" > - f"Stderr: {result.stderr}" > - ) > - self._git_ref =3D result.stdout.strip() > + > > def _create_tarball_dir(self) -> None: > os.makedirs(self._tarball_dir, exist_ok=3DTrue) > -- > 2.34.1 >