Message ID | 20220816084210.14972-13-stefan.herbrechtsmeier-oss@weidmueller.com |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
Series | [v4,01/13] binman: Skip elf tests if python elftools is not available | expand |
Hi Stefan, On Tue, 16 Aug 2022 at 02:42, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@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@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
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 + 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 + """ + data = self.data = self._DoReadFileRealDtb('236_compress_dtb_missing_bintool.dts') + 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"; + }; + }; +};