[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 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.
> 
> 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 at theobroma-systems.com>
>>> ---
>>>   tools/binman/btool/btool_gzip.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Simon Glass <sjg at 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