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

[Simon Glass]

Hi Quentin,

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