All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] binman: bintool: remove btool_ prefix from btool names
@ 2022-09-01 14:43 Quentin Schulz
  2022-09-02 20:00 ` Simon Glass
  0 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2022-09-01 14:43 UTC (permalink / raw)
  Cc: sjg, alpernebiyasak, stefan.herbrechtsmeier, u-boot,
	Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@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-boot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 tools/binman/bintool.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index ec30ceff74..81a314f6f1 100644
--- a/tools/binman/bintool.py
+++ b/tools/binman/bintool.py
@@ -135,6 +135,8 @@ class Bintool:
         names = [os.path.splitext(os.path.basename(fname))[0]
                  for fname in files]
         names = [name for name in names if name[0] != '_']
+        names = [name[6:] if name.startswith('btool_') else name
+                 for name in names]
         if include_testing:
             names.append('_testing')
         return sorted(names)
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] binman: bintool: remove btool_ prefix from btool names
  2022-09-01 14:43 [PATCH v2] binman: bintool: remove btool_ prefix from btool names Quentin Schulz
@ 2022-09-02 20:00 ` Simon Glass
  2022-09-03  8:48   ` Quentin Schulz
  0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2022-09-02 20:00 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Alper Nebi Yasak, Stefan Herbrechtsmeier, U-Boot Mailing List,
	Quentin Schulz, Quentin Schulz

Hi Quentin,

On Thu, 1 Sept 2022 at 08:44, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@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-boot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@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

Regards,
Simon

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] binman: bintool: remove btool_ prefix from btool names
  2022-09-02 20:00 ` Simon Glass
@ 2022-09-03  8:48   ` Quentin Schulz
  2022-09-03 17:01     ` Simon Glass
  0 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2022-09-03  8:48 UTC (permalink / raw)
  To: Simon Glass, Quentin Schulz
  Cc: Alper Nebi Yasak, Stefan Herbrechtsmeier, U-Boot Mailing List,
	Quentin Schulz, Quentin Schulz

Hi Simon,

On September 2, 2022 10:00:18 PM GMT+02:00, Simon Glass <sjg@chromium.org> wrote:
>Hi Quentin,
>
>On Thu, 1 Sept 2022 at 08:44, Quentin Schulz <foss+uboot@0leil.net> wrote:
>>
>> From: Quentin Schulz <quentin.schulz@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-boot@0leil.net>
>> Signed-off-by: Quentin Schulz <quentin.schulz@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).

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.

Cheers,
Quentin

>Regards,
>Simon

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] binman: bintool: remove btool_ prefix from btool names
  2022-09-03  8:48   ` Quentin Schulz
@ 2022-09-03 17:01     ` Simon Glass
  2022-09-03 17:45       ` Quentin Schulz
  0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2022-09-03 17:01 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, Alper Nebi Yasak, Stefan Herbrechtsmeier,
	U-Boot Mailing List, Quentin Schulz, Quentin Schulz

Hi Quentin,

On Sat, 3 Sept 2022 at 02:48, Quentin Schulz <foss@0leil.net> wrote:
>
> Hi Simon,
>
> On September 2, 2022 10:00:18 PM GMT+02:00, Simon Glass <sjg@chromium.org> wrote:
> >Hi Quentin,
> >
> >On Thu, 1 Sept 2022 at 08:44, Quentin Schulz <foss+uboot@0leil.net> wrote:
> >>
> >> From: Quentin Schulz <quentin.schulz@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-boot@0leil.net>
> >> Signed-off-by: Quentin Schulz <quentin.schulz@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?

Or perhaps just a new patch against dm/testing would sort this out?

>
> 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.

Regards,
Simon

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] binman: bintool: remove btool_ prefix from btool names
  2022-09-03 17:01     ` Simon Glass
@ 2022-09-03 17:45       ` Quentin Schulz
  2022-09-07 21:11         ` Simon Glass
  0 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2022-09-03 17:45 UTC (permalink / raw)
  To: Simon Glass
  Cc: Quentin Schulz, Alper Nebi Yasak, Stefan Herbrechtsmeier,
	U-Boot Mailing List, Quentin Schulz

Hi Simon,

On September 3, 2022 7:01:14 PM GMT+02:00, Simon Glass <sjg@chromium.org> wrote:
>Hi Quentin,
>
>On Sat, 3 Sept 2022 at 02:48, Quentin Schulz <foss@0leil.net> wrote:
>>
>> Hi Simon,
>>
>> On September 2, 2022 10:00:18 PM GMT+02:00, Simon Glass <sjg@chromium.org> wrote:
>> >Hi Quentin,
>> >
>> >On Thu, 1 Sept 2022 at 08:44, Quentin Schulz <foss+uboot@0leil.net> wrote:
>> >>
>> >> From: Quentin Schulz <quentin.schulz@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-boot@0leil.net>
>> >> Signed-off-by: Quentin Schulz <quentin.schulz@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?

>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.

>>
>> 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 :)

Cheers,
Quentin

>Regards,
>Simon

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] binman: bintool: remove btool_ prefix from btool names
  2022-09-03 17:45       ` Quentin Schulz
@ 2022-09-07 21:11         ` Simon Glass
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2022-09-07 21:11 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, Alper Nebi Yasak, Stefan Herbrechtsmeier,
	U-Boot Mailing List, Quentin Schulz

Hi Quentin,

On Sat, 3 Sept 2022 at 11:45, Quentin Schulz <foss+u-boot@0leil.net> wrote:
>
> Hi Simon,
>
> On September 3, 2022 7:01:14 PM GMT+02:00, Simon Glass <sjg@chromium.org> wrote:
> >Hi Quentin,
> >
> >On Sat, 3 Sept 2022 at 02:48, Quentin Schulz <foss@0leil.net> wrote:
> >>
> >> Hi Simon,
> >>
> >> On September 2, 2022 10:00:18 PM GMT+02:00, Simon Glass <sjg@chromium.org> wrote:
> >> >Hi Quentin,
> >> >
> >> >On Thu, 1 Sept 2022 at 08:44, Quentin Schulz <foss+uboot@0leil.net> wrote:
> >> >>
> >> >> From: Quentin Schulz <quentin.schulz@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-boot@0leil.net>
> >> >> Signed-off-by: Quentin Schulz <quentin.schulz@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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-09-07 21:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-01 14:43 [PATCH v2] binman: bintool: remove btool_ prefix from btool names Quentin Schulz
2022-09-02 20:00 ` Simon Glass
2022-09-03  8:48   ` Quentin Schulz
2022-09-03 17:01     ` Simon Glass
2022-09-03 17:45       ` Quentin Schulz
2022-09-07 21:11         ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.