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

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

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