Am Freitag, dem 30.05.2025 um 12:18 +0100 schrieb Simon Glass:
> Hi Yannic,
>
> On Tue, 27 May 2025 at 14:24, Yannic Moog <y.m...@phytec.de> 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.
> >
> > Signed-off-by: Yannic Moog <y.m...@phytec.de>
> > ---
> > tools/binman/control.py | 8 ++++----
> > tools/binman/entry.py | 6 +++++-
> > tools/binman/etype/cbfs.py | 3 ++-
> > tools/binman/etype/mkimage.py | 2 +-
> > tools/binman/etype/section.py | 18 +++++++++++++-----
> > tools/binman/image.py | 4 +++-
> > 6 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/binman/control.py b/tools/binman/control.py
> > index
> > 81f61e3e152a9eab558cfc9667131a38082b61a1..c2a4d3eae9d4e8f4a3c5be1abfb1469ba1
> > c0ed93 100644
> > --- a/tools/binman/control.py
> > +++ b/tools/binman/control.py
> > @@ -18,6 +18,7 @@ import re
> >
> > import sys
> >
> > +import image
>
> This is done later in this file, to avoid warnings about missing
> modules when trying to get help ('binman -h'). I am guessing you need
> it for the type declaration below.
>
> It may be that this is not necessary anymore though, in which case
> could you put this in a separate patch, use the 'from binman import'
> syntax and drop the other 'imports' from below?
Would like to leave this untouched to keep diff short after thinking about it.
>
> > from binman import bintool
> > from binman import cbfs_util
> > from binman import elf
> > @@ -669,7 +670,7 @@ def CheckForProblems(image):
> > for bintool in missing_bintool_list])))
> > return any([missing_list, faked_list, missing_bintool_list])
> >
> > -def ProcessImage(image, update_fdt, write_map, get_contents=True,
> > +def ProcessImage(image: image.Image, update_fdt, write_map,
> > get_contents=True,
> > allow_resize=True, allow_missing=False,
> > allow_fake_blobs=False):
> > """Perform all steps for this image, including checking and # writing
> > it.
> > @@ -697,7 +698,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 +736,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..ce7ef28e94b17e349544776070c457d588
> > 2661b1 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
> > 886e34ef8221ad50e9f881e8abad16015c9790b5..bb97b227ef78356b08b5cc31b04d98c6fa
> > 0633ef 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.GetEntries().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
> > 19ee30a13ca75a5d05a17d4c26424e84934b6ea2..c9362c6bbf740da946abe2fadefe35ebfc
> > 89fced 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
> > 44b1b85e93496d0ca114907f42a98da1f0af09b9..8e5c20e3017c9b320ebfdfd3bf82489ed6
> > 161c98 100644
> > --- a/tools/binman/etype/section.py
> > +++ b/tools/binman/etype/section.py
> > @@ -450,7 +450,7 @@ class Entry_section(Entry):
> > def _PackEntries(self):
> > """Pack all entries into the section"""
> > offset = self._skip_at_start
> > - for entry in self._entries.values():
> > + for entry in self.GetEntries().values():
>
> As per previous patch, this should not be needed.
Agree.
>
> > offset = entry.Pack(offset)
> > return offset
> >
> > @@ -533,7 +533,7 @@ class Entry_section(Entry):
> > for entry in self.GetEntries().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):
> > @@ -768,9 +768,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
>
> should this be: all optional children ?
Yes, forgot to update, thanks.
>
> > + 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():
>
> As above, I think self._entries is correct
>
> > + e.drop_absent_optional()
> >
> > def _SetEntryOffsetSize(self, name, offset, size):
> > """Set the offset and size of an entry
> > diff --git a/tools/binman/image.py b/tools/binman/image.py
> > index
> > 24ce0af7c72b5256a9a71963a6d94c080ed8bdd4..86543ad8db36b38afc1957fde7e2015206
> > 7b79aa 100644
> > --- a/tools/binman/image.py
> > +++ b/tools/binman/image.py
> > @@ -15,7 +15,7 @@ import sys
> > from binman.entry import Entry
> > from binman.etype import fdtmap
> > from binman.etype import image_header
> > -from binman.etype import section
> > +from etype import section
>
> Why this change? I suspect it will break when installed with 'pip
> install binary-manager'
I only used this to get lsp working, will drop this change.
>
> > from dtoc import fdt
> > from dtoc import fdt_util
> > from u_boot_pylib import tools
> > @@ -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
> >
>
> Regards,
> Simon
