On Fri, Jun 13, 2025 at 5:04 AM Yannic Moog <[email protected]> wrote: > > When blobs are absent and are marked as optional, they can be safely > dropped from the binman tree. Use the drop_absent function for that. > Rename drop_absent to drop_absent_optional as we do not want to drop any > entries that are absent; they should be reported by binman as errors > when they are missing. > We also reorder the processing of the image the following: > - We call the CheckForProblems function before the image is built. > - We drop entries after we checked for problems with the image. > This is okay because CheckForProblems does not look at the file we have > written but rather queries the data structure (image) built with binman. > This also allows us to get all error and warning messages that we want > to report while avoiding putting missing optional entries in the final > image. > As only the blobs are dropped, the sections still remain in the > assembled image. Thus add them to the expected test case checks where > necessary. > > In addition, a rework of testPackTeeOsOptional test case is necessary. > > The test did not really do what it was supposed to. The description said > that optional binary is tested, but the binary is not marked as > optional. Further, the tee.elf file, when included in the image > properly, also shows up in the image data. This must be added as well. > > As there is no global variable for the elf data, set the pathname to the > elf file that was created when setting up the test suite. > For the test case get the filename and read the contents, comparing them > to the contents of the created binman image. > > Signed-off-by: Yannic Moog <[email protected]> > --- > tools/binman/control.py | 5 ++--- > tools/binman/entry.py | 6 +++++- > tools/binman/etype/cbfs.py | 3 ++- > tools/binman/etype/mkimage.py | 2 +- > tools/binman/etype/section.py | 16 ++++++++++++---- > tools/binman/ftest.py | 14 ++++++++------ > tools/binman/image.py | 2 ++ > 7 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/tools/binman/control.py b/tools/binman/control.py > index > 1946656f7d368209df3299a0e7c833b93edf2120..5a66c06d346cc74dc3e0e037eb263b6ced64a9b4 > 100644 > --- a/tools/binman/control.py > +++ b/tools/binman/control.py > @@ -697,7 +697,6 @@ def ProcessImage(image, update_fdt, write_map, > get_contents=True, > image.SetAllowMissing(allow_missing) > image.SetAllowFakeBlob(allow_fake_blobs) > image.GetEntryContents() > - image.drop_absent() > image.GetEntryOffsets() > > # We need to pack the entries to figure out where everything > @@ -736,12 +735,12 @@ def ProcessImage(image, update_fdt, write_map, > get_contents=True, > image.Raise('Entries changed size after packing (tried %s passes)' % > passes) > > + has_problems = CheckForProblems(image) > + > image.BuildImage() > if write_map: > image.WriteMap() > > - has_problems = CheckForProblems(image) > - > image.WriteAlternates() > > return has_problems > diff --git a/tools/binman/entry.py b/tools/binman/entry.py > index > 6390917e5083e40a4e3294e5d36ec300d2057fe9..ce7ef28e94b17e349544776070c457d5882661b1 > 100644 > --- a/tools/binman/entry.py > +++ b/tools/binman/entry.py > @@ -760,7 +760,7 @@ class Entry(object): > self.image_pos) > > # pylint: disable=assignment-from-none > - def GetEntries(self): > + def GetEntries(self) -> None: > """Return a list of entries contained by this entry > > Returns: > @@ -1351,6 +1351,10 @@ features to produce new behaviours. > os.mkdir(cls.fake_dir) > tout.notice(f"Fake-blob dir is '{cls.fake_dir}'") > > + def drop_absent_optional(self) -> None: > + """Entries don't have any entries, do nothing""" > + pass > + > def ensure_props(self): > """Raise an exception if properties are missing > > diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py > index > 124fa1e4ffc7d0374b27c09d32ead8429567d304..5879f377231bf94697bea97c3cfc3a2515665b8f > 100644 > --- a/tools/binman/etype/cbfs.py > +++ b/tools/binman/etype/cbfs.py > @@ -276,7 +276,8 @@ class Entry_cbfs(Entry): > for entry in self._entries.values(): > entry.ListEntries(entries, indent + 1) > > - def GetEntries(self): > + def GetEntries(self) -> dict[str, Entry]: > + """Returns the entries (tree children) of this section""" > return self._entries > > def ReadData(self, decomp=True, alt_format=None): > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py > index > 6ae5d0c8a4f3c54e1277a575b8e235cc1aba4e13..75e59c3d3a3104da7559982f64968fdd99b7bd5f > 100644 > --- a/tools/binman/etype/mkimage.py > +++ b/tools/binman/etype/mkimage.py > @@ -205,7 +205,7 @@ class Entry_mkimage(Entry_section): > self.record_missing_bintool(self.mkimage) > return data > > - def GetEntries(self): > + def GetEntries(self) -> dict[str, Entry]: > # Make a copy so we don't change the original > entries = OrderedDict(self._entries) > if self._imagename: > diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py > index > 1d50bb477534702019502d08f8648d4c9a367c0f..03c4f7c6ec74c6fa75c9362e2fecdb0e6ab568cb > 100644 > --- a/tools/binman/etype/section.py > +++ b/tools/binman/etype/section.py > @@ -537,7 +537,7 @@ class Entry_section(Entry): > for entry in self._entries.values(): > entry.WriteMap(fd, indent + 1) > > - def GetEntries(self): > + def GetEntries(self) -> dict[str, Entry]: > return self._entries > > def GetContentsByPhandle(self, phandle, source_entry, required): > @@ -772,9 +772,17 @@ class Entry_section(Entry): > todo) > return True > > - def drop_absent(self): > - """Drop entries which are absent""" > - self._entries = {n: e for n, e in self._entries.items() if not > e.absent} > + def drop_absent_optional(self) -> None: > + """Drop entries which are absent. > + Call for all nodes in the tree. Leaf nodes will do nothing per > + definition. Sections however have _entries and should drop all > children > + which are absent. > + """ > + self._entries = {n: e for n, e in self._entries.items() if not > (e.absent and e.optional)} > + # Drop nodes first before traversing children to avoid superfluous > calls > + # to children of absent nodes. > + for e in self.GetEntries().values(): > + e.drop_absent_optional() > > def _SetEntryOffsetSize(self, name, offset, size): > """Set the offset and size of an entry > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > index > 4a8d330c8f89adc3c248199760a936dd243158a2..d707e2aa555b39316d206a4c170ebb83aa6daf72 > 100644 > --- a/tools/binman/ftest.py > +++ b/tools/binman/ftest.py > @@ -251,7 +251,7 @@ class TestFunctional(unittest.TestCase): > # ATF and OP_TEE > TestFunctional._MakeInputFile('bl31.elf', > tools.read_file(cls.ElfTestFile('elf_sections'))) > - TestFunctional._MakeInputFile('tee.elf', > + TestFunctional.tee_elf_path = > TestFunctional._MakeInputFile('tee.elf', > tools.read_file(cls.ElfTestFile('elf_sections'))) > > # Newer OP_TEE file in v1 binary format > @@ -6459,16 +6459,18 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > def testAbsent(self): > """Check handling of absent entries""" > data = self._DoReadFile('262_absent.dts') > - self.assertEqual(U_BOOT_DATA + U_BOOT_IMG_DATA, data) > + self.assertEqual(U_BOOT_DATA + b'aa' + U_BOOT_IMG_DATA, data) > > - def testPackTeeOsOptional(self): > - """Test that an image with an optional TEE binary can be created""" > + def testPackTeeOsElf(self): > + """Test that an image with a TEE elf binary can be created""" > entry_args = { > 'tee-os-path': 'tee.elf', > } > + tee_path = self.tee_elf_path > data = self._DoReadFileDtb('263_tee_os_opt.dts', > entry_args=entry_args)[0] > - self.assertEqual(U_BOOT_DATA + U_BOOT_IMG_DATA, data) > + self.assertEqual(U_BOOT_DATA + tools.read_file(tee_path) + > + U_BOOT_IMG_DATA, data) > > def checkFitTee(self, dts, tee_fname): > """Check that a tee-os entry works and returns data > @@ -6724,7 +6726,7 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > > node = dtb.GetNode('/configurations/conf-missing-tee-1') > self.assertEqual('atf-1', node.props['firmware'].value) > - self.assertEqual(['u-boot', 'atf-2'], > + self.assertEqual(['u-boot', 'tee', 'atf-2'], > fdt_util.GetStringList(node, 'loadables')) > > def testTooldir(self): > diff --git a/tools/binman/image.py b/tools/binman/image.py > index > 24ce0af7c72b5256a9a71963a6d94c080ed8bdd4..698cfa4148e1a0f762d90d99ee3bc581cff5105a > 100644 > --- a/tools/binman/image.py > +++ b/tools/binman/image.py > @@ -183,6 +183,8 @@ class Image(section.Entry_section): > fname = tools.get_output_filename(self._filename) > tout.info("Writing image to '%s'" % fname) > with open(fname, 'wb') as fd: > + # For final image, don't write absent blobs to file > + self.drop_absent_optional() > data = self.GetPaddedData() > fd.write(data) > tout.info("Wrote %#x bytes" % len(data)) > > -- > 2.43.0 >
Hi Yannic, This series just got merged to master and I noticed it failing building imx8mm_venice on my Ubuntu 20.04 dev host which has Python 3.8.10. make imx8mm_venice_defconfig make ... BINMAN .binman_stamp binman: 'type' object is not subscriptable make: *** [Makefile:1336: .binman_stamp] Error 1 Do you know what the minimum version of python required is or what would need to be changed here to support the previous version of python that worked? Best Regards, Tim
