Hi Quentin,
On Sat, 3 Sept 2022 at 11:45, Quentin Schulz <foss+u-b...@0leil.net> wrote:
>
> Hi Simon,
>
> On September 3, 2022 7:01:14 PM GMT+02:00, Simon Glass <s...@chromium.org>
> wrote:
> >Hi Quentin,
> >
> >On Sat, 3 Sept 2022 at 02:48, Quentin Schulz <f...@0leil.net> wrote:
> >>
> >> Hi Simon,
> >>
> >> On September 2, 2022 10:00:18 PM GMT+02:00, Simon Glass
> >> <s...@chromium.org> wrote:
> >> >Hi Quentin,
> >> >
> >> >On Thu, 1 Sept 2022 at 08:44, Quentin Schulz <foss+ub...@0leil.net> wrote:
> >> >>
> >> >> From: Quentin Schulz <quentin.sch...@theobroma-systems.com>
> >> >>
> >> >> The binary is looked on the system by the suffix of the packer class.
> >> >> This means binman was looking for btool_gzip on the system and not gzip.
> >> >>
> >> >> Since a btool can have its btool_ prefix missing but its module and
> >> >> binary presence on the system appropriately found, there's no need to
> >> >> actually keep this prefix after listing all possible btools, so let's
> >> >> remove it.
> >> >>
> >> >> This fixes gzip btool by letting Bintool.find_bintool_class handle the
> >> >> missing prefix and still return the correct class which is then init
> >> >> with gzip name instead of btool_gzip.
> >> >>
> >> >> Fixes: 0f369d79925a ("binman: Add gzip bintool")
> >> >> Cc: Quentin Schulz <foss+u-b...@0leil.net>
> >> >> Signed-off-by: Quentin Schulz <quentin.sch...@theobroma-systems.com>
> >> >> ---
> >> >> tools/binman/bintool.py | 2 ++
> >> >> 1 file changed, 2 insertions(+)
> >> >
> >> >Do we still need this patch? Please see u-boot-dm/testing
> >> >
> >>
> >> Since you took the V1, no. Either version is fine IMO though the second
> >> version would have been a cleaner approach when a second btool prefixed
> >> with btool_ will appear (if that ever happens).
> >
> >Hmm I cannot find v1. Can you please send the patchwork link?
> >
>
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/commit/daa2da754afe1bac777f6cb0f05233e0de7b325d
>
> Though, admittedly, not sure how one could have figured that one out since
> the title and content are entirely different from V2.
>
> Both patches are fixing the same bug, how should I have handled this new
> patch? As an entirely new patch and comment to the V1 that it is abandoned?
> Link to the V1 in the V2 and comment in the V1 where to find V2? Just
> wondering what's the process here if there's any?
Ah OK. I sent a PR today, so you could add a patch based on dm/master
if you lik.
>
> >Or perhaps just a new patch against dm/testing would sort this out?
> >
>
> Sure, can do. In essence it'll be this patch plus the one linked above
> reverted, all in one patch (or two?). But if there's no plan to have other
> btool_ prefixed btools, maybe it's just not necessary since the one is
> already fixed with the aforementioned commit.
Whatever suits.
>
> >>
> >> I might carve some time to rename all btools to have btool_ as prefix and
> >> remove the prefix as done in this patch before use, so that we simplify
> >> things a bit.
> >
> >I think tools with Python-module equivalents will be an uncommon case,
> >so it seems better to keep the special-casing code.
> >
>
> I like consistency in naming and was quite confused why only one had a
> different naming. But if there's no interest in such a patch, fine by me, I
> won't fight it :)
Yes I do understand that. We could fix it by preferring binman modules
to system ones, but at least for now I think the prefix things works.
We can always revisit it if it becomes a pain.
Regards,
SImon
