Re: [PATCH 1/4] dts: constrain DPDK source flag

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Fri, Feb 23, 2024 at 8:09 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Hi Juraj,
>
> Thank you for your review!
>
> On 29/01/2024 11:47, Juraj Linkeš 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 = parser.add_mutually_exclusive_group(required=True)
>
> 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 here.

> > 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 != 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