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

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