Message ID | 20220831155514.110128-1-foss+uboot@0leil.net |
---|---|
State | Accepted |
Commit | daa2da754afe1bac777f6cb0f05233e0de7b325d |
Delegated to: | Simon Glass |
Headers | show |
Series | binman: btool: gzip: fix packer name so that binary can be found | expand |
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. > > 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. Regards, Simon
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. >> 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 already have a test for the compressions: testCompUtilVersions Regards Stefan
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#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@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. I have nothing more interesting to offer though at the moment, I'm sorry. Cheers, Quentin
Hi Stefan, On Thu, 1 Sept 2022 at 00:12, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> 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. When I tested it, it was not picking up the correct version without this patch. > > >> 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 already have a test for the compressions: > testCompUtilVersions > > Regards > Stefan Regards, Simon
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#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@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
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#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: Applied to u-boot-dm, thanks!
diff --git a/tools/binman/btool/btool_gzip.py b/tools/binman/btool/btool_gzip.py index 7bea300b5d..70cbc19f04 100644 --- a/tools/binman/btool/btool_gzip.py +++ b/tools/binman/btool/btool_gzip.py @@ -27,5 +27,5 @@ class Bintoolbtool_gzip(bintool.BintoolPacker): man gzip """ def __init__(self, name): - super().__init__(name, compress_args=[], + super().__init__("gzip", compress_args=[], version_regex=r'gzip ([0-9.]+)')