[PATCH] binman: btool: gzip: fix packer name so that binary can be found

[PATCH] binman: btool: gzip: fix packer name so that binary can be found

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

Therefore, let's pass "gzip" as the name so that it can be found and
used.

Fixes: 0f369d79925a ("binman: Add gzip bintool")
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 tools/binman/btool/btool_gzip.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/btool/btool_gzip.py b/tools/binman/btool/btool_gzip.py
index 7bea300b5d..70cbc19f04 100644
--- a/tools/binman/btool/btool_gzip.py
+++ b/tools/binman/btool/btool_gzip.py
@@ -27,5 +27,5 @@ class Bintoolbtool_gzip(bintool.BintoolPacker):
         man gzip
     """
     def __init__(self, name):
-        super().__init__(name, compress_args=[],
+        super().__init__("gzip", compress_args=[],
                          version_regex=r'gzip ([0-9.]+)')
-- 
2.37.2

Re: [PATCH] binman: btool: gzip: fix packer name so that binary can be found

[Simon Glass]

On Wed, 31 Aug 2022 at 09:55, 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.
>
> Therefore, let's pass "gzip" as the name so that it can be found and
> used.
>
> Fixes: 0f369d79925a ("binman: Add gzip bintool")
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  tools/binman/btool/btool_gzip.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Oops! I wonder how we could test this? One way would be to require
those tools to be present and write a test that reads the version, I
suppose.

Regards,
Simon

Re: [PATCH] binman: btool: gzip: fix packer name so that binary can be found

[Stefan Herbrechtsmeier]

Hi Quentin,

Am 31.08.2022 um 19:44 schrieb Simon Glass:
> On Wed, 31 Aug 2022 at 09:55, 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.

Are you sure? I test it and the name is already gzip because the bintool 
is requested as gzip. The find_bintool_class function only change the 
class name.

>> Therefore, let's pass "gzip" as the name so that it can be found and
>> used.
>>
>> Fixes: 0f369d79925a ("binman: Add gzip bintool")
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>   tools/binman/btool/btool_gzip.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Oops! I wonder how we could test this? One way would be to require
> those tools to be present and write a test that reads the version, I
> suppose.

We already have a test for the compressions:
testCompUtilVersions

Regards
   Stefan

Re: [PATCH] binman: btool: gzip: fix packer name so that binary can be found

[Quentin Schulz]

Hi Stefan,

On 9/1/22 08:12, Stefan Herbrechtsmeier wrote:
> Hi Quentin,
> 
> Am 31.08.2022 um 19:44 schrieb Simon Glass:
>> On Wed, 31 Aug 2022 at 09:55, 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.
> 
> Are you sure? I test it and the name is already gzip because the bintool 
> is requested as gzip. The find_bintool_class function only change the 
> class name.
> 

 From current master:
tools/binman/binman tool --list
Name             Version      Description                Path
---------------  -----------  ------------------------- 
------------------------------
btool_gzip       -            btool_gzip compression     (not found)

With my patch:
tools/binman/binman tool --list
Name             Version      Description                Path
---------------  -----------  ------------------------- 
------------------------------
gzip             1.11         gzip compression           /usr/bin/gzip

Bintool.get_tool_list will return btool_gzip. Bintool.list_all will then 
iterate over all tools and call Bintool.create(name) for each.

Bintool.create will call Bintool.find_bintool_class with btool_gzip and 
it'll return the Bintoolbtool_gzip class. Then its constructor will be 
called, with btool_gzip passed as argument, here: 
https://source.denx.de/u-boot/u-boot/-/blob/master/tools/binman/bintool.py#L111

This is because Bintool.create has no knowledge of btool_gzip actually 
being gzip unlike Bintool.find_bintool_class.

Another way to handle this, and without user intervention would be to 
remove btool_ prefix when listing the supported tools since 
Bintool.find_bintool_class will actually handle the case where the 
prefix is missing.

It'd be something like:
diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index ec30ceff74..433ee87c46 100644
--- a/tools/binman/bintool.py
+++ b/tools/binman/bintool.py
@@ -135,6 +135,7 @@ 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)

Which also makes sure that the tools are actually alphabetically ordered 
(it is currently ordered with the "btool_" prefix).

Now I have to ask... Why not simplify all this and force all bintools to 
be prefixed with btool_ so we do not have to care about different scenarii?

>>> Therefore, let's pass "gzip" as the name so that it can be found and
>>> used.
>>>
>>> Fixes: 0f369d79925a ("binman: Add gzip bintool")
>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>> ---
>>>   tools/binman/btool/btool_gzip.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Oops! I wonder how we could test this? One way would be to require
>> those tools to be present and write a test that reads the version, I
>> suppose.

We'd need each btool class to provide the name of the binary expected to 
exist on a given system. Then we could mock calls to os.path.isfile and 
os.access in patman.tool_find and check that the correct string is 
searched for. If we don't have a hardcoded value that the developer had 
to put there, automated tests won't help anyways since here we'd have 
looked for btool_gzip in one of the mocked calls and that would have 
succeeded unfortunately.

> 
> We already have a test for the compressions:
> testCompUtilVersions
> 

If the tests are skipped because gzip is not found but is actually 
present, that is not great either.

I have nothing more interesting to offer though at the moment, I'm sorry.

Cheers,
Quentin

Re: [PATCH] binman: btool: gzip: fix packer name so that binary can be found

[Simon Glass]

Hi Stefan,

On Thu, 1 Sept 2022 at 00:12, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> Hi Quentin,
>
> Am 31.08.2022 um 19:44 schrieb Simon Glass:
> > On Wed, 31 Aug 2022 at 09:55, 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.
>
> Are you sure? I test it and the name is already gzip because the bintool
> is requested as gzip. The find_bintool_class function only change the
> class name.

When I tested it, it was not picking up the correct version without this patch.

>
> >> Therefore, let's pass "gzip" as the name so that it can be found and
> >> used.
> >>
> >> Fixes: 0f369d79925a ("binman: Add gzip bintool")
> >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >> ---
> >>   tools/binman/btool/btool_gzip.py | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Oops! I wonder how we could test this? One way would be to require
> > those tools to be present and write a test that reads the version, I
> > suppose.
>
> We already have a test for the compressions:
> testCompUtilVersions
>
> Regards
>    Stefan

Regards,
Simon

Re: [PATCH] binman: btool: gzip: fix packer name so that binary can be found

[Stefan Herbrechtsmeier]

Hi Quentin,

Am 01.09.2022 um 16:34 schrieb Quentin Schulz:
> Hi Stefan,
> 
> On 9/1/22 08:12, Stefan Herbrechtsmeier wrote:
>> Hi Quentin,
>>
>> Am 31.08.2022 um 19:44 schrieb Simon Glass:
>>> On Wed, 31 Aug 2022 at 09:55, 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.
>>
>> Are you sure? I test it and the name is already gzip because the 
>> bintool is requested as gzip. The find_bintool_class function only 
>> change the class name.
>>
> 
>  From current master:
> tools/binman/binman tool --list
> Name             Version      Description                Path
> ---------------  -----------  ------------------------- 
> ------------------------------
> btool_gzip       -            btool_gzip compression     (not found)
> 
> With my patch:
> tools/binman/binman tool --list
> Name             Version      Description                Path
> ---------------  -----------  ------------------------- 
> ------------------------------
> gzip             1.11         gzip compression           /usr/bin/gzip
> 
> Bintool.get_tool_list will return btool_gzip. Bintool.list_all will then 
> iterate over all tools and call Bintool.create(name) for each.
> 
> Bintool.create will call Bintool.find_bintool_class with btool_gzip and 
> it'll return the Bintoolbtool_gzip class. Then its constructor will be 
> called, with btool_gzip passed as argument, here: 
> https://source.denx.de/u-boot/u-boot/-/blob/master/tools/binman/bintool.py#L111 

Ok, we use different ways to test it. I use the version test and this 
use a fixed gzip name as input.

> This is because Bintool.create has no knowledge of btool_gzip actually 
> being gzip unlike Bintool.find_bintool_class.
> 
> Another way to handle this, and without user intervention would be to 
> remove btool_ prefix when listing the supported tools since 
> Bintool.find_bintool_class will actually handle the case where the 
> prefix is missing.

I think this is a better solution.

> It'd be something like:
> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> index ec30ceff74..433ee87c46 100644
> --- a/tools/binman/bintool.py
> +++ b/tools/binman/bintool.py
> @@ -135,6 +135,7 @@ 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)
> 
> Which also makes sure that the tools are actually alphabetically ordered 
> (it is currently ordered with the "btool_" prefix).
> 
> Now I have to ask... Why not simplify all this and force all bintools to 
> be prefixed with btool_ so we do not have to care about different scenarii?

This would make things easier.

>>>> Therefore, let's pass "gzip" as the name so that it can be found and
>>>> used.
>>>>
>>>> Fixes: 0f369d79925a ("binman: Add gzip bintool")
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>> ---
>>>>   tools/binman/btool/btool_gzip.py | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> Oops! I wonder how we could test this? One way would be to require
>>> those tools to be present and write a test that reads the version, I
>>> suppose.
> 
> We'd need each btool class to provide the name of the binary expected to 
> exist on a given system. Then we could mock calls to os.path.isfile and 
> os.access in patman.tool_find and check that the correct string is 
> searched for. If we don't have a hardcoded value that the developer had 
> to put there, automated tests won't help anyways since here we'd have 
> looked for btool_gzip in one of the mocked calls and that would have 
> succeeded unfortunately.
> 
>>
>> We already have a test for the compressions:
>> testCompUtilVersions
>>
> 
> If the tests are skipped because gzip is not found but is actually 
> present, that is not great either.

The test isn't skipped because it use a fixed list of required tools 
(ex. gzip) and therefore work. The problem is the tools option which 
doesn't remove the prefix.

Regards
   Stefan

Re: [PATCH] binman: btool: gzip: fix packer name so that binary can be found

[Simon Glass]

Hi Quentin,

Am 01.09.2022 um 16:34 schrieb Quentin Schulz:
> Hi Stefan,
>
> On 9/1/22 08:12, Stefan Herbrechtsmeier wrote:
>> Hi Quentin,
>>
>> Am 31.08.2022 um 19:44 schrieb Simon Glass:
>>> On Wed, 31 Aug 2022 at 09:55, 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.
>>
>> Are you sure? I test it and the name is already gzip because the
>> bintool is requested as gzip. The find_bintool_class function only
>> change the class name.
>>
>
>  From current master:
> tools/binman/binman tool --list
> Name             Version      Description                Path
> ---------------  -----------  -------------------------
> ------------------------------
> btool_gzip       -            btool_gzip compression     (not found)
>
> With my patch:
> tools/binman/binman tool --list
> Name             Version      Description                Path
> ---------------  -----------  -------------------------
> ------------------------------
> gzip             1.11         gzip compression           /usr/bin/gzip
>
> Bintool.get_tool_list will return btool_gzip. Bintool.list_all will then
> iterate over all tools and call Bintool.create(name) for each.
>
> Bintool.create will call Bintool.find_bintool_class with btool_gzip and
> it'll return the Bintoolbtool_gzip class. Then its constructor will be
> called, with btool_gzip passed as argument, here:
> https://source.denx.de/u-boot/u-boot/-/blob/master/tools/binman/bintool.py#L111

Ok, we use different ways to test it. I use the version test and this
use a fixed gzip name as input.

> This is because Bintool.create has no knowledge of btool_gzip actually
> being gzip unlike Bintool.find_bintool_class.
>
> Another way to handle this, and without user intervention would be to
> remove btool_ prefix when listing the supported tools since
> Bintool.find_bintool_class will actually handle the case where the
> prefix is missing.

I think this is a better solution.

> It'd be something like:
Applied to u-boot-dm, thanks!