On Fri, Feb 23, 2024 at 8:09 PM Luca Vizzarro <luca.vizza...@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.lin...@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
