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