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..c2a4d3eae9d4e8f4a3c5be1abfb1469ba1c0ed93
> 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?
> 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..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
> 886e34ef8221ad50e9f881e8abad16015c9790b5..bb97b227ef78356b08b5cc31b04d98c6fa0633ef
> 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..c9362c6bbf740da946abe2fadefe35ebfc89fced
> 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..8e5c20e3017c9b320ebfdfd3bf82489ed6161c98
> 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.
> 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 ?
> + 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..86543ad8db36b38afc1957fde7e20152067b79aa
> 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'
> 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
