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