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 <[email protected]>
wrote:
From: Quentin Schulz <[email protected]>
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 <[email protected]>
---
tools/binman/btool/btool_gzip.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass <[email protected]>
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
