Hi Simon,
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:
> >
> > Optional blobs should mark themselves as absent to avoid being packed
> > into an image.
> > Extend the documentation of this behaviour. Although the documentation
> > implied this before, the "optional" property had not been explained
> > properly before.
> > Note that the intended behaviour is the same, this patch just documents
> > it.
>
> This seems to indicate that it is just a doc change
>
> > The actual behaviour will change as now absent entries are no longer
> > packed into an image. The image map will also reflect this.
>
> But this indicates that it changes the functionality. Can you clarify
> this in your commit message?
Will clarify. I wrote that the intended! behaviour is the same, but due to a
bug, the actual behaviour changes ...
The bug being that even though the documentation says optional blobs will be
removed, they are not.
>
> > As a result, the CheckForProblems() function will no longer alert on
> > optional (blob) entries. This is because the missing optional images
> > were removed before CheckForProblems is called.
> > This, as well, is intended behaviour.
>
> This breaks testExtblobOptional() for me.
... That is why this test fails I believe. Since it relies on the bug to be
present (i.e. absent optional blobs still included in the image).
>
> I applied your patch: tools: binman: drop "faked" return value from
> check_fake_fname
>
> Then I applied this one and got the error.
>
> I could not apply patch 2 (ools: binman: ftest: pass allow_fake_blob
> to _DoReadFileDtb) as it broke a lot of tests.
>
> For testExtblobOptional() I see that you have fixed it by the end of
> the series, but I get 11/12 test failures . Are you able to make the
> series bisectable, so that 'binman test' works for each commit? It's
> hard for me to comment on the series in this state.
Yes, will do.
>
> Also there's no need to put an 'RFC' tag on this series. We do want to
> resolve this, one way or another.
I put RFC, because imo it is not to the quality standards expected of patches.
You wanted to review the current state, but I didn't clean up yet, so that is
why I sent as RFC.
Yannic
>
> >
> > Reported-by: Simon Glass <s...@chromium.org>
> > Signed-off-by: Yannic Moog <y.m...@phytec.de>
> > ---
> > tools/binman/binman.rst | 7 +++++++
> > tools/binman/etype/blob.py | 2 ++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> > index
> > 84b1331df5c006f72167016b7f33de73f4a269fa..392e507d449c75eeba4431fe2db01d820b
> > ce8f61 100644
> > --- a/tools/binman/binman.rst
> > +++ b/tools/binman/binman.rst
> > @@ -1143,6 +1143,13 @@ Optional entries
> >
> > Some entries need to exist only if certain conditions are met. For example,
> > an
> > entry may want to appear in the image only if a file has a particular
> > format.
> > +Also, the ``optional`` property may be used to mark entries as optional::
> > +
> > + tee-os {
> > + filename = "tee.bin";
> > + optional;
> > + };
> > +
> > Obviously the entry must exist in the image description for it to be
> > processed
> > at all, so a way needs to be found to have the entry remove itself.
> >
> > diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
> > index
> > 970fae91cd9b2a31b3f31d2ecdb77e27a89d69fb..acd9ae34074e4b4b9792b107cc80514dfd
> > d88b3b 100644
> > --- a/tools/binman/etype/blob.py
> > +++ b/tools/binman/etype/blob.py
> > @@ -52,6 +52,8 @@ class Entry_blob(Entry):
> > fake_size = self.assume_size
> > self._pathname = self.check_fake_fname(self._filename,
> > fake_size)
> > self.missing = True
> > + if self.optional:
> > + self.mark_absent("missing but optional")
> > if not self.faked:
> > content_size = 0
> > if self.assume_size: # Ensure we get test coverage on next
> > line
> >
> > --
> > 2.43.0
> >
>
> Regards,
> Simon
