diff mbox series

[v3,1/2] binman: bintool: remove btool_ prefix from btool names

Message ID 20220930-upstream-gzip-binman-v3-v3-1-17c99d6d87ac@theobroma-systems.com
State Changes Requested
Delegated to: Simon Glass
Headers show
Series automatically stip btool_ from btool names | expand

Commit Message

Quentin Schulz Sept. 30, 2022, 2:36 p.m. UTC 
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.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 tools/binman/bintool.py | 2 ++
 1 file changed, 2 insertions(+)

Comments

Simon Glass Sept. 30, 2022, 11:49 p.m. UTC | #1 
On Fri, 30 Sept 2022 at 08:37, 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.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  tools/binman/bintool.py | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>
Simon Glass Oct. 22, 2022, 1:06 a.m. UTC | #2 
Hi Quentin,

On Fri, 30 Sept 2022 at 17:49, Simon Glass <sjg@chromium.org> wrote:
>
> On Fri, 30 Sept 2022 at 08:37, 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.
> >
> > Cc: Quentin Schulz <foss+uboot@0leil.net>
> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > ---
> >  tools/binman/bintool.py | 2 ++
> >  1 file changed, 2 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Unfortunately this breaks 'binman test'.

Regards,
Simon
Quentin Schulz Oct. 24, 2022, 9:26 a.m. UTC | #3 
Hi Simon,

On 10/22/22 03:06, Simon Glass wrote:
> Hi Quentin,
> 
> On Fri, 30 Sept 2022 at 17:49, Simon Glass <sjg@chromium.org> wrote:
>>
>> On Fri, 30 Sept 2022 at 08:37, 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.
>>>
>>> Cc: Quentin Schulz <foss+uboot@0leil.net>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>> ---
>>>   tools/binman/bintool.py | 2 ++
>>>   1 file changed, 2 insertions(+)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Unfortunately this breaks 'binman test'.
> 

Indeed.

So this is an issue with the modules global variable in 
tools/binman/bintool.py which only stores the module and not the 
associated class name when calling find_bintool_class.

This means that when caching the module on the first call to 
find_bintool_class, class_name will be set to Bintoolbtool_gzip but the 
module_name is gzip only, adding the module in the gzip key in the 
module dictionary. When hitting the cache on next calls, the gzip key is 
found, so its value (the module) is used. However the default class_name 
(Bintoolgzip) is used, failing the getattr call.

What I can offer is to only have the module (filename) changed for when 
there are system conflicts like we had for gzip.

E.g.:

diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index a582d9d344..8fda13ff01 100644
--- a/tools/binman/bintool.py
+++ b/tools/binman/bintool.py
@@ -85,7 +85,6 @@ class Bintool:
                  try:
                      # Deal with classes which must be renamed due to 
conflicts
                      # with Python libraries
-                    class_name = f'Bintoolbtool_{module_name}'
                      module = 
importlib.import_module('binman.btool.btool_' +
                                                       module_name)
                  except ImportError:
@@ -137,6 +136,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)
diff --git a/tools/binman/btool/btool_gzip.py 
b/tools/binman/btool/btool_gzip.py
index 70cbc19f04..a7ce6411cd 100644
--- a/tools/binman/btool/btool_gzip.py
+++ b/tools/binman/btool/btool_gzip.py
@@ -14,7 +14,7 @@ Documentation is available via::
  from binman import bintool

  # pylint: disable=C0103
-class Bintoolbtool_gzip(bintool.BintoolPacker):
+class Bintoolgzip(bintool.BintoolPacker):
      """Compression/decompression using the gzip algorithm

      This bintool supports running `gzip` to compress and decompress 
data, as

This would at least limit the number of differences between a btool_ 
prefixed module and one that isn't to the filename and the module name, 
the rest would be identical.

What do you think? I can send this as a v4 if you prefer discussing it 
this way.

Cheers,
Quentin
Simon Glass Oct. 30, 2022, 1:44 a.m. UTC | #4 
Hi Quentin,

On Mon, 24 Oct 2022 at 03:26, Quentin Schulz
<quentin.schulz@theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 10/22/22 03:06, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Fri, 30 Sept 2022 at 17:49, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> On Fri, 30 Sept 2022 at 08:37, 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.
> >>>
> >>> Cc: Quentin Schulz <foss+uboot@0leil.net>
> >>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>> ---
> >>>   tools/binman/bintool.py | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Unfortunately this breaks 'binman test'.
> >
>
> Indeed.
>
> So this is an issue with the modules global variable in
> tools/binman/bintool.py which only stores the module and not the
> associated class name when calling find_bintool_class.
>
> This means that when caching the module on the first call to
> find_bintool_class, class_name will be set to Bintoolbtool_gzip but the
> module_name is gzip only, adding the module in the gzip key in the
> module dictionary. When hitting the cache on next calls, the gzip key is
> found, so its value (the module) is used. However the default class_name
> (Bintoolgzip) is used, failing the getattr call.
>
> What I can offer is to only have the module (filename) changed for when
> there are system conflicts like we had for gzip.
>
> E.g.:
>
> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> index a582d9d344..8fda13ff01 100644
> --- a/tools/binman/bintool.py
> +++ b/tools/binman/bintool.py
> @@ -85,7 +85,6 @@ class Bintool:
>                   try:
>                       # Deal with classes which must be renamed due to
> conflicts
>                       # with Python libraries
> -                    class_name = f'Bintoolbtool_{module_name}'
>                       module =
> importlib.import_module('binman.btool.btool_' +
>                                                        module_name)
>                   except ImportError:
> @@ -137,6 +136,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)
> diff --git a/tools/binman/btool/btool_gzip.py
> b/tools/binman/btool/btool_gzip.py
> index 70cbc19f04..a7ce6411cd 100644
> --- a/tools/binman/btool/btool_gzip.py
> +++ b/tools/binman/btool/btool_gzip.py
> @@ -14,7 +14,7 @@ Documentation is available via::
>   from binman import bintool
>
>   # pylint: disable=C0103
> -class Bintoolbtool_gzip(bintool.BintoolPacker):
> +class Bintoolgzip(bintool.BintoolPacker):
>       """Compression/decompression using the gzip algorithm
>
>       This bintool supports running `gzip` to compress and decompress
> data, as
>
> This would at least limit the number of differences between a btool_
> prefixed module and one that isn't to the filename and the module name,
> the rest would be identical.
>
> What do you think? I can send this as a v4 if you prefer discussing it
> this way.

That seems good to me. Let's see the patch.

Regards,
Simon
diff mbox series

Patch

diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index a582d9d344..fcc6aab36f 100644
--- a/tools/binman/bintool.py
+++ b/tools/binman/bintool.py
@@ -137,6 +137,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)