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 61CAA4411C; Fri, 31 May 2024 11:04:34 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 11765402C9; Fri, 31 May 2024 11:04:34 +0200 (CEST) Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mails.dpdk.org (Postfix) with ESMTP id 2BEA8402AF for ; Fri, 31 May 2024 11:04:31 +0200 (CEST) Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-a653972487fso143699466b.1 for ; Fri, 31 May 2024 02:04:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1717146271; x=1717751071; 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=0fITMpjcARGyi+Vtbo04v3jZmj1FAbKHyOVEvSWnNJQ=; b=lgPmZL4Bc14trHhPjwWE9QLJQ52C6jngGuvLAYWb9aCEeBN4HJTDPwXZtqaOJZJePo tPF2bHFpk6eMZYTI1l7TfP9ioHLZ4UZDVWXpZ3tLduA73hddAOM/PmL+3CmXgonRZJNF 1WRQgusE+tsmhdIyvmsfEHuhr2gLnqQ+5WkC+kgQOZI88rTsG/l7jiyjHgXkD0Ux10kv AMFYcFbZEwUMkLTMQu+WgTm/MoYSt+d/AS4vkHnImhDoDKwlKgz50TWhCHmUyxgL93qL PWPdaAMK3ct0MqPdr6yHXt8OIPNEXa5WgBMM3NyC+hnpT5oQ8J61AR/5nRxEJQT1ezyu xX1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717146271; x=1717751071; 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=0fITMpjcARGyi+Vtbo04v3jZmj1FAbKHyOVEvSWnNJQ=; b=f76BmvOqP01JUtOeX8jWALezfIYGi2LDQBY3SBbpBTuSTGug3u0/I2voDjEXgr6A+E xfN09L0TTd70AXUyfDxYSbtOpAkzFY+NNKH9v7bzoR7h3JryyjTQxGeub0jQgKAkz0WR LzjzxpOf0wG8gMaGExqLKhM5mIhpzhefdl6SeTZtpMQ65I+qlS7JxHPAqv81QXPz6K/F EkkD9pLHwyBH85+AjDFSwwTts6f85V5XUDcsm0kFJ3YvMXXtMiU4IqYx0LRMHi2Op9Lu 6K8i0Haa1zLEsrDWY0Bk3GcY7TlIXi2d7KgMmBXQeGWciVchRLyeRPdSadfolfiIfi+U X4HA== X-Gm-Message-State: AOJu0Ywny/I26GLJ8sgPV08Hd3TPV8YF/E6DkIDynKdBQBTR3yDDrp2F 3kjQcqGHiL1JuW51WjHNNonWYgYK+xzrE7nWodQrL6z3Hn1CEHoon9E65yzXO4RS5FHva8PuPgW ZeuTf7unH7tVSPduEXW7EDya36aoyALVVWCeM3g== X-Google-Smtp-Source: AGHT+IEHXE42zkFPpUEL5f1wzjOTIIMRbe4dvSmJ7jP/wZrWdNTI9pniXrGAOdfVn6CsF1rzCtPzigm4SnA6sa0FmhU= X-Received: by 2002:a17:906:5fd4:b0:a66:2265:2bc7 with SMTP id a640c23a62f3a-a6821b71ebcmr95532266b.47.1717146270705; Fri, 31 May 2024 02:04:30 -0700 (PDT) MIME-Version: 1.0 References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240514121023.1957025-1-luca.vizzarro@arm.com> <20240514121023.1957025-2-luca.vizzarro@arm.com> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Fri, 31 May 2024 11:04:19 +0200 Message-ID: Subject: Re: [PATCH v5 1/3] dts: rework arguments framework To: Luca Vizzarro Cc: dev@dpdk.org, Jeremy Spewock , 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 Thu, May 30, 2024 at 8:43=E2=80=AFPM Luca Vizzarro wrote: > > On 30/05/2024 16:30, Juraj Linke=C5=A1 wrote: > > There is a difference in behavior when I pass no arguments and then I > > either have or don't have an env var set: > > ./main.py > > usage: main.py [-h] [--config-file FILE_PATH] ... > > ... > > > > ----------------------------- > > DTS_SKIP_SETUP=3DY ./main.py > > main.py: error: one of the arguments --tarball/--snapshot > > --revision/--rev/--git-ref is required > > > > For help and usage, run the command with the --help flag. > > > > I think this is because we're adding the env vars as actions (the > > second scenario thus has at least one) and the first scenario doesn't > > have any actions, which leads to the different output. I don't know > > whether we want to unify these, but it's a bit confusing, as the error > > applies to both cases. > > The behaviour is just as expected. You get the same exact results if you > were to run: > ./main.py -s > which is equivalent to: > DTS_SKIP_SETUP=3D ./main.py > > The true equivalent to your example would be: > ./main.py -s Y > Oh, so this is expected. I'm fine with this, but even though it's equivalent, it doesn't look that way (at first glance), especially when used with export: export DTS_SKIP_SETUP=3DY ... # other commands ./main.py -s But I don't think that's a problem, so let's leave it as is. > The only anomaly is that no matter the value passed (true, false, y, > n...) it's always going to be set because the action is `store_true`. > If we care to interpret a possible value then we need to implement a new > action to cater for this. > The only interpretation we want to do is whether the variable is set or not and it's documented as such (the docs say "set to any value"), so there's not much of a point to interpret it. And this way it mirrors the cmdline args (it either is there or isn't). > >> diff --git a/dts/framework/settings.py b/dts/framework/settings.py > >> index 688e8679a7..b19f274f9d 100644 > >> --- a/dts/framework/settings.py > >> +++ b/dts/framework/settings.py > > > >> @@ -109,104 +116,224 @@ class Settings: > >> > >> SETTINGS: Settings =3D Settings() > >> > >> +P =3D ParamSpec("P") > >> > >> -def _get_parser() -> argparse.ArgumentParser: > >> - """Create the argument parser for DTS. > >> +#: Attribute name representing the env variable name to augment :clas= s:`~argparse.Action` with. > >> +_ENV_VAR_NAME_ATTR =3D "env_var_name" > >> +#: Attribute name representing the value origin to augment :class:`~a= rgparse.Action` with. > >> +_IS_FROM_ENV_ATTR =3D "is_from_env" > >> > >> - Command line options take precedence over environment variables, = which in turn take precedence > >> - over default values. > >> +#: The prefix to be added to all of the environment variables. > >> +ENV_PREFIX =3D "DTS_" > >> + > >> + > >> +def is_action_in_args(action: Action) -> bool: > > > > I don't think there's any expectation that these functions will be > > used outside this module, so I'd make them all private, in which case > > the short docstring would be fine. > > > > The ParamSpec also maybe should be private. Same goes for ENV_PREFIX. > > Ack. > > > I'm also looking at the order of the various functions, classes and > > variables in the module and it looks all over the place. Maybe we can > > tidy it up a bit. > > Could you please elaborate on this? It is also important to define > what's the ordering expectation. At the moment they are mostly in order > of usage, except for the Settings which I purposefully left on top as > they are easier to find and probably the most "needed" for a user. As > for the env var helper functions, most of them are used rightaway, but I > left a couple that are used later there so that they are all grouped > together (and related). > I just wanted to make sure there's some logic to it and the way you described it sounds good. Also, I was looking at the file with all of the patches applied, so that made it look more jumbled than it is. Without the two functions added later (or with them in the right place), the order actually looks fine. > >> +class ArgumentParser(argparse.ArgumentParser): > > > > I'd rename this to DTSArgumentParser and maybe also make the classes > > (this one and the formatter) private. > > > > Ack. > >> + Instead of printing usage on every error, it prints instructions > >> + on how to do it. > > > > This sentence is confusing - how to do what? > > This could do without the sentence to be honest, I'll just remove it. > Ok. > >> - parser.add_argument( > >> + add_argument_to_parser_with_env =3D augment_add_argument_with_env= (parser.add_argument) > > > > I'm wondering whether we could modify the actual add_argument methods > > of ArgumentParser and _MutuallyExclusiveGroup, either the class > > methods or instance methods. Have you tried that? It could be easier > > to understand. > > I tried this, but it becomes overly complicated because add_argument is > implemented in _ActionsContainer which in turn is inherited by several > classes, which we ultimately use. The easiest and clearest approach is > just a wrapper. > Ok, let's go with the wrapper then. > > Or, alternatively, we could do: > > > > action =3D parser.add_argument( > > "--config-file", > > default=3DSETTINGS.config_file_path, > > type=3DPath, > > help=3D"The configuration file that describes the test cases, SUTs > > and targets.", > > metavar=3D"FILE_PATH", > > ) > > add_env_var_to_action(action, env_var_name=3D"CFG_FILE") > > > > This makes what we're trying to do a bit clearer, but requires two > > calls instead of one, so maybe it's not better. I'm not sure. > > I can drop the HOF and use it as `_add_env_var_to_action` as a clearer > replacement. > Let's try it.