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 75AA643BCD; Fri, 1 Mar 2024 11:22:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6399E43391; Fri, 1 Mar 2024 11:22:20 +0100 (CET) Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by mails.dpdk.org (Postfix) with ESMTP id 62BA543390 for ; Fri, 1 Mar 2024 11:22:19 +0100 (CET) Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-56647babfe6so2956715a12.3 for ; Fri, 01 Mar 2024 02:22:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1709288539; x=1709893339; 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=AFkeiNDUaLU+NDDrhD6Vmpsscn4ncxiZ/SOqY2QgB50=; b=ZhhGnPVNNb7b/wOiNOZ3ax6n6ylQ4XkvVoEXCjBkvHznJN/FuANf0MchVMdg3hg5fr arFHIzM5C3msJF7ZvpPOx81SEYkKjYXsCXhQp5QBADCdLBLITsD5mUU0+aF43V/wr0bG Dn6lkQNz+r7YuTcNOaHvWidOfSNTEFvzfVV3YuaLGM1+9+P7vgyo7JDbzst05+JKmI9G B0zxaJms3gvVe6IUYxwZyaOuEi0XPwX7sfXnxPb99p4BVDsiS1dYj8bvM6v2cJsi7IIA 0WGHZf+6YuZ7sq3MdROongjBIBvvtsEwgUz8kahcRI+QugbNGbyF6zUSWvessndUYsnj 7/QQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709288539; x=1709893339; 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=AFkeiNDUaLU+NDDrhD6Vmpsscn4ncxiZ/SOqY2QgB50=; b=kAW8a8DDyiptzKSKffdrjZqOpEa+bEhuV7hAHeBmQ6tFgt7drPT2YfvXYQn0YZ4KQD o0VMiQ3gRd5fLUqtJ/n6Wlr+dydBpIeJZFGRAovXr6taKHVom2FzR//pB0mUxh2zu8vz i2PonC7bVVQxQFTNhEsbkqRp5u3RXHWihcJaKParG3pWUv5SWeiYeQYTdxnJAytR05S0 xVR1hV8Eji9xtycGitya1LssQEXMsO1EJn2k6Rhesq3hX8ZXVZkAPeic4aZlRuX5oxP+ cjKjRA8IFohC0ZTKwnXvAsZBxkkzY/ZlJAOG6GkIhGF68q0DymOKRGPhay9+ehaZLuhC Wo1w== X-Gm-Message-State: AOJu0YyikQCPWgZY9OHJXFOUbYD4tlvoiGILuvWuj35r3tFephzR7PG2 sQMQYUGy/1uVn7N1X8uCV7BX2Ng36yoYcmKNeWt6HfSCMsAIvI+0YTyOaaeNXeVtqlNsyyspJlP GlNXhkf8yQIHk+xP+g4IQgzhfphFrZOm3Nubu1Q== X-Google-Smtp-Source: AGHT+IES/QmLZcsKKXZ6Q2/7bxbWIGs3x6UHXN/TSBM741sLj9ZKKizzzKFwLwxYGLXyjIshUM538bKYT6+3vpSQ29A= X-Received: by 2002:a17:906:a409:b0:a44:4e66:e5ce with SMTP id l9-20020a170906a40900b00a444e66e5cemr1042877ejz.12.1709288538941; Fri, 01 Mar 2024 02:22:18 -0800 (PST) MIME-Version: 1.0 References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240122182611.1904974-2-luca.vizzarro@arm.com> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Fri, 1 Mar 2024 11:22:07 +0100 Message-ID: Subject: Re: [PATCH 1/4] dts: constrain DPDK source flag To: Luca Vizzarro Cc: "dev@dpdk.org" 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 Fri, Feb 23, 2024 at 8:09=E2=80=AFPM Luca Vizzarro wrote: > > Hi Juraj, > > Thank you for your review! > > On 29/01/2024 11:47, Juraj Linke=C5=A1 wrote: > > 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. > > Yep, it is enforced in the code, you may have missed it. The two > arguments are under the same mutual exclusion group in line 220: > > dpdk_source =3D parser.add_mutually_exclusive_group(required=3DTrue) > > When using both arguments `argparse` will automatically complain that > you can only use one or the other. > Yes, I missed this. This looks great. > > 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). > > Yes, it is a good idea. Especially since this will work within IDEs. > > > 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. > > Ack. > > > I think this would read better as one sentence. > > Ack. > > > Since this is a patch with improvements, maybe we could add metavars > > to other arguments as well. It looks pretty good. > > Sure, no problem! > > > 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. > > I've tried to find a way to still keep them. But with arguments done > this way, it is somewhat hard to understand the provenance of the value, > whether it's set in the arguments, an environment variable or just the > default value. Therefore, give a useful error message to the user when > using something invalid. I'll try to come up with something as you > suggested, although I am not entirely convinced it'll be ideal. > For reference, my test case blocking patch implements an alternative approach: https://patches.dpdk.org/project/dpdk/patch/20240223075502.60485-= 8-juraj.linkes@pantheon.tech/ It's the same thing, significantly simplified. Looks like it could work her= e. > > This would also probably read better as one sentence. > > Ack. > > > We shuffled the order of operations a bit and now the error message > > doesn't correspond. > > Sorry, I don't think I am understanding what you are referring to > exactly. What do you mean? > You removed what I commented on (in the future, please keep that so what we know what the original comment relates to), so here's what I commented on: + 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}" + ) Previously, this was inside DPDKGitTarball which assumed it was passed git ref because the command line arg is not a path. In this patch, the git ref arg is decoupled from the tarball path, so the function shouldn't reference the path. > Best, > Luca