[PATCH v4 13/13] binman: Support missing compression tools

[Simon Glass]

Hi Stefan,

On Tue, 16 Aug 2022 at 02:42, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>
> Handle missing compression tools by returning empty data and marking the
> entry as 'missing'.

Great to see this.

This is actually a bit more subtle, see below.

>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>
> ---
>
> Changes in v4:
> - Add missing 236_compress_dtb_missing_bintool.dts file
>
> Changes in v3:
> - Added
>
>  tools/binman/entry.py                            |  4 ++++
>  tools/binman/ftest.py                            |  8 ++++++++
>  .../test/236_compress_dtb_missing_bintool.dts    | 16 ++++++++++++++++
>  3 files changed, 28 insertions(+)
>  create mode 100644 tools/binman/test/236_compress_dtb_missing_bintool.dts
>
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 9ec5811b46..c86b757a4e 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -1078,7 +1078,11 @@ features to produce new behaviours.
>          """
>          self.uncomp_data = indata
>          if self.compress != 'none':
> +            if not comp_util.is_present(self.compress):
> +                self.missing = True

We must check self.GetAllowMissing(). If that is True then we can do
this. If False then we cannot. Hmm binman needs to fail if a bintool
is missing, unless --allow-missing is given.

Also you should call self.record_missing_bintool() here, instead of
setting self.missing (which refers to a missing blob).

BTW you also need to record the binbool somewhere with
self.AddBintools() in Entry:

def AddBintools(self, btools):
   self.comp_bintool = self.AddBintool(btools, self.compress)

That way you will get the right message, which is 'has missing bintools'

You then need to check for that stdout in your test, e.g. as is done
in testGbbMissing()

Finally, be careful to keep code coverage at 100%.

> +                return b''
>              self.uncomp_size = len(indata)
> +
>          data = comp_util.compress(indata, self.compress)
>          return data
>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index a360ebeef5..eac7ccb087 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -2557,6 +2557,14 @@ class TestFunctional(unittest.TestCase):
>              }
>          self.assertEqual(expected, props)
>
> +    def testCompressMissingBintool(self):
> +        """Test that compress of device-tree files with missing bintool is
> +        supported

Please add new tests at the end (so things are roughly in order of
test-file number). Also the test desc should fit on one like (e.g.
drop the 'Test that' text.

> +        """
> +        data = self.data = self._DoReadFileRealDtb('236_compress_dtb_missing_bintool.dts')

Can you drop self.data ?

> +        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
> +        dtb_data = data[len(U_BOOT_DATA):]
> +        self.assertEqual(0, len(dtb_data))


>
>      def testCbfsUpdateFdt(self):
>          """Test that we can update the device tree with CBFS offset/size info"""
> diff --git a/tools/binman/test/236_compress_dtb_missing_bintool.dts b/tools/binman/test/236_compress_dtb_missing_bintool.dts
> new file mode 100644
> index 0000000000..e7ce1b893d
> --- /dev/null
> +++ b/tools/binman/test/236_compress_dtb_missing_bintool.dts
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               u-boot {
> +               };
> +               u-boot-dtb {
> +                       compress = "_testing";
> +               };
> +       };
> +};
> --
> 2.30.2
>

Regards,
Simon