[RFC 0/2] Do not stop with an error when mkimage fails

[RFC 0/2] Do not stop with an error when mkimage fails

[Heiko Thiery]

While converting to binman for an imx8mq board, it has been found that
building in the u-boot CI fails. This is because an imx8mq requires an
external binary (signed_hdmi_imx8m.bin). If this file cannot be found
mkimage fails. To work around the problem the exception is caught, an
error message is printed and binman continues.

Heiko Thiery (2):
  patman: introduce RunException
  binman: catch RunException for mkimage runtime failure

 tools/binman/etype/mkimage.py |  8 +++++++-
 tools/patman/tools.py         | 16 +++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.30.2

[RFC 1/2] patman: introduce RunException

[Heiko Thiery]

The RunException will be throws when the a command's return_code is not
equal zero. With this an external caller can catch that and has access
to the command/args, the result code, the stdout and stderr output.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
 tools/patman/tools.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/patman/tools.py b/tools/patman/tools.py
index 710f1fdcd3..ca1c9114ab 100644
--- a/tools/patman/tools.py
+++ b/tools/patman/tools.py
@@ -313,6 +313,18 @@ def GetTargetCompileTool(name, cross_compile=None):
         target_name = name
     return target_name, extra_args
 
+class RunException(Exception):
+    """Exception that is thrown when the command fails"""
+    def __init__(self, args, result):
+        self.args = args
+        self.stdout = result.stdout.strip()
+        self.stderr = result.stderr.strip()
+        self.return_code = result.return_code
+
+    def __str__(self):
+        return ("Error %d running '%s': %s" %
+               (self.return_code,' '.join(self.args), self.stderr))
+
 def Run(name, *args, **kwargs):
     """Run a tool with some arguments
 
@@ -349,9 +361,7 @@ def Run(name, *args, **kwargs):
         result = command.RunPipe([all_args], capture=True, capture_stderr=True,
                                  env=env, raise_on_error=False, binary=binary)
         if result.return_code:
-            raise Exception("Error %d running '%s': %s" %
-               (result.return_code,' '.join(all_args),
-                result.stderr))
+            raise RunException(all_args, result)
         return result.stdout
     except:
         if env and not PathHasFile(env['PATH'], name):
-- 
2.30.2

[RFC 2/2] binman: catch RunException for mkimage runtime failure

[Heiko Thiery]

In case mkimage exits with a return code other than zero do not stop.
Print an error message and go on.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
 tools/binman/etype/mkimage.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index e49977522e..24fbe79172 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -10,6 +10,7 @@ from collections import OrderedDict
 from binman.entry import Entry
 from dtoc import fdt_util
 from patman import tools
+from patman import tout
 
 class Entry_mkimage(Entry):
     """Binary produced by mkimage
@@ -51,7 +52,12 @@ class Entry_mkimage(Entry):
         input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
         tools.WriteFile(input_fname, data)
         output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
-        tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
+
+        try:
+            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
+        except Exception as e:
+            tout.Error("mkimage failed: %s" % e)
+
         self.SetContents(tools.ReadFile(output_fname))
         return True
 
-- 
2.30.2

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Wolfgang Denk]

Dear Heiko,

In message <20211104185231.2927-1-heiko.thiery@gmail.com> you wrote:
> While converting to binman for an imx8mq board, it has been found that
> building in the u-boot CI fails. This is because an imx8mq requires an
> external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> mkimage fails. To work around the problem the exception is caught, an
> error message is printed and binman continues.

But how can you continue, when mkimage fails and cannot generate the
needed image?

In your patch 2/2 we have this:

+            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
+        except Exception as e:
+            tout.Error("mkimage failed: %s" % e)
+
         self.SetContents(tools.ReadFile(output_fname))

mkimage is supposed to create an output file which name is in
output_fname; if mkimage fails and you continue, the next step is
tools.ReadFile(output_fname) trying to read that file.  How is this
possible?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"One day," said a dull voice from down below, "I'm going to  be  back
in  form again and you're going to be very sorry you said that. For a
very long time. I might even go so far as to make even more Time just
for you to be sorry in."              - Terry Pratchett, _Small Gods_

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Heiko Thiery]

Hi Wolfgang,

Am Do., 4. Nov. 2021 um 20:12 Uhr schrieb Wolfgang Denk <wd@denx.de>:
>
> Dear Heiko,
>
> In message <20211104185231.2927-1-heiko.thiery@gmail.com> you wrote:
> > While converting to binman for an imx8mq board, it has been found that
> > building in the u-boot CI fails. This is because an imx8mq requires an
> > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > mkimage fails. To work around the problem the exception is caught, an
> > error message is printed and binman continues.
>
> But how can you continue, when mkimage fails and cannot generate the
> needed image?
>
> In your patch 2/2 we have this:
>
> +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> +        except Exception as e:
> +            tout.Error("mkimage failed: %s" % e)
> +
>          self.SetContents(tools.ReadFile(output_fname))
>
> mkimage is supposed to create an output file which name is in
> output_fname; if mkimage fails and you continue, the next step is
> tools.ReadFile(output_fname) trying to read that file.  How is this
> possible?

# ls -al mkimage*
-rw-r--r-- 1 hthiery hthiery      0 Nov  4 20:28 mkimage-out.spl.mkimage
-rw-r--r-- 1 hthiery hthiery 180392 Nov  4 20:28 mkimage.spl.mkimage

The file (mkimage-out.spl.mkimage) with size 0 seems to be created.  I
assume mkimage will create that.

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "One day," said a dull voice from down below, "I'm going to  be  back
> in  form again and you're going to be very sorry you said that. For a
> very long time. I might even go so far as to make even more Time just
> for you to be sorry in."              - Terry Pratchett, _Small Gods_

Re: [RFC 2/2] binman: catch RunException for mkimage runtime failure

[Simon Glass]

Hi Heiko,

On Thu, 4 Nov 2021 at 12:53, Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> In case mkimage exits with a return code other than zero do not stop.
> Print an error message and go on.
>
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
>  tools/binman/etype/mkimage.py | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Somehow we need to record that it failed, at least.

>
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index e49977522e..24fbe79172 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -10,6 +10,7 @@ from collections import OrderedDict
>  from binman.entry import Entry
>  from dtoc import fdt_util
>  from patman import tools
> +from patman import tout
>
>  class Entry_mkimage(Entry):
>      """Binary produced by mkimage
> @@ -51,7 +52,12 @@ class Entry_mkimage(Entry):
>          input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
>          tools.WriteFile(input_fname, data)
>          output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
> -        tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> +
> +        try:
> +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> +        except Exception as e:
> +            tout.Error("mkimage failed: %s" % e)
> +
>          self.SetContents(tools.ReadFile(output_fname))
>          return True
>
> --
> 2.30.2
>

Regards,
SImon

Re: [RFC 1/2] patman: introduce RunException

[Simon Glass]

On Thu, 4 Nov 2021 at 12:53, Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> The RunException will be throws when the a command's return_code is not
> equal zero. With this an external caller can catch that and has access
> to the command/args, the result code, the stdout and stderr output.
>
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
>  tools/patman/tools.py | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [RFC 2/2] binman: catch RunException for mkimage runtime failure

[Heiko Thiery]

Hi Simon,

Am Fr., 5. Nov. 2021 um 03:02 Uhr schrieb Simon Glass <sjg@chromium.org>:
>
> Hi Heiko,
>
> On Thu, 4 Nov 2021 at 12:53, Heiko Thiery <heiko.thiery@gmail.com> wrote:
> >
> > In case mkimage exits with a return code other than zero do not stop.
> > Print an error message and go on.
> >
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> >  tools/binman/etype/mkimage.py | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
>
> Somehow we need to record that it failed, at least.

By record do you mean that it should not only be output as an error
message (as already done), but should be remembered like e.g. the
missing external blobs?

>
> >
> > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> > index e49977522e..24fbe79172 100644
> > --- a/tools/binman/etype/mkimage.py
> > +++ b/tools/binman/etype/mkimage.py
> > @@ -10,6 +10,7 @@ from collections import OrderedDict
> >  from binman.entry import Entry
> >  from dtoc import fdt_util
> >  from patman import tools
> > +from patman import tout
> >
> >  class Entry_mkimage(Entry):
> >      """Binary produced by mkimage
> > @@ -51,7 +52,12 @@ class Entry_mkimage(Entry):
> >          input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
> >          tools.WriteFile(input_fname, data)
> >          output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
> > -        tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > +
> > +        try:
> > +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > +        except Exception as e:
> > +            tout.Error("mkimage failed: %s" % e)
> > +
> >          self.SetContents(tools.ReadFile(output_fname))
> >          return True
> >
> > --
> > 2.30.2
> >
>
> Regards,
> SImon

-- 
Heiko

Re: [RFC 2/2] binman: catch RunException for mkimage runtime failure

[Simon Glass]

Hi Heiko,

On Fri, 5 Nov 2021 at 01:50, Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> Hi Simon,
>
> Am Fr., 5. Nov. 2021 um 03:02 Uhr schrieb Simon Glass <sjg@chromium.org>:
> >
> > Hi Heiko,
> >
> > On Thu, 4 Nov 2021 at 12:53, Heiko Thiery <heiko.thiery@gmail.com> wrote:
> > >
> > > In case mkimage exits with a return code other than zero do not stop.
> > > Print an error message and go on.
> > >
> > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > ---
> > >  tools/binman/etype/mkimage.py | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > Somehow we need to record that it failed, at least.
>
> By record do you mean that it should not only be output as an error
> message (as already done), but should be remembered like e.g. the
> missing external blobs?

Yes that's right. If you say

self.missing = True

in there it should do the right thing.

Also, I wonder if we should have a separate return value from mkimage
so we know for sure that the cause was missing input files? We can
worry about that later, I suppose.

Regards,
Simon


>
> >
> > >
> > > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> > > index e49977522e..24fbe79172 100644
> > > --- a/tools/binman/etype/mkimage.py
> > > +++ b/tools/binman/etype/mkimage.py
> > > @@ -10,6 +10,7 @@ from collections import OrderedDict
> > >  from binman.entry import Entry
> > >  from dtoc import fdt_util
> > >  from patman import tools
> > > +from patman import tout
> > >
> > >  class Entry_mkimage(Entry):
> > >      """Binary produced by mkimage
> > > @@ -51,7 +52,12 @@ class Entry_mkimage(Entry):
> > >          input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
> > >          tools.WriteFile(input_fname, data)
> > >          output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
> > > -        tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > > +
> > > +        try:
> > > +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > > +        except Exception as e:
> > > +            tout.Error("mkimage failed: %s" % e)
> > > +
> > >          self.SetContents(tools.ReadFile(output_fname))
> > >          return True
> > >
> > > --
> > > 2.30.2
> > >
> >
> > Regards,
> > SImon
>
> --
> Heiko

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Wolfgang Denk]

Dear Heiko,

In message <CAEyMn7Zgn1ej3kGzg2kiCyqK5DQhkSdXNAUavZGs+3L_fb1Lvg@mail.gmail.com> you wrote:
>
> > > While converting to binman for an imx8mq board, it has been found that
> > > building in the u-boot CI fails. This is because an imx8mq requires an
> > > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > > mkimage fails. To work around the problem the exception is caught, an
> > > error message is printed and binman continues.
> >
> > But how can you continue, when mkimage fails and cannot generate the
> > needed image?

Let me rephrase:

	How can can you continue, when mkimage fails and cannot
	_succesfully_ generate the needed image?

> > In your patch 2/2 we have this:
> >
> > +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > +        except Exception as e:
> > +            tout.Error("mkimage failed: %s" % e)
> > +
> >          self.SetContents(tools.ReadFile(output_fname))
> >
> > mkimage is supposed to create an output file which name is in
> > output_fname; if mkimage fails and you continue, the next step is
> > tools.ReadFile(output_fname) trying to read that file.  How is this
> > possible?
>
> # ls -al mkimage*
> -rw-r--r-- 1 hthiery hthiery      0 Nov  4 20:28 mkimage-out.spl.mkimage
> -rw-r--r-- 1 hthiery hthiery 180392 Nov  4 20:28 mkimage.spl.mkimage
>
> The file (mkimage-out.spl.mkimage) with size 0 seems to be created.  I
> assume mkimage will create that.

So in this situation we know that mkimage failed, and it generated
an empty output file.

I guess the output file is _not_ empty when no errors occur?

which reasonable results can you expect when you ignore an error and
just continue with garbage data as if nothing happened?

Sorry, but this makes no sense to me.  If there is an error, it
should be reported and - if possible - handled.  If this is not
possible, then the correct thing is to abort.  Ignoring errors and
trying to continue is always the worst thing to do.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"I go on working for the same reason a hen goes on laying eggs."
- H. L. Mencken

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Heiko Thiery]

Hi Wolfgang,

Am So., 7. Nov. 2021 um 15:48 Uhr schrieb Wolfgang Denk <wd@denx.de>:
>
> Dear Heiko,
>
> In message <CAEyMn7Zgn1ej3kGzg2kiCyqK5DQhkSdXNAUavZGs+3L_fb1Lvg@mail.gmail.com> you wrote:
> >
> > > > While converting to binman for an imx8mq board, it has been found that
> > > > building in the u-boot CI fails. This is because an imx8mq requires an
> > > > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > > > mkimage fails. To work around the problem the exception is caught, an
> > > > error message is printed and binman continues.
> > >
> > > But how can you continue, when mkimage fails and cannot generate the
> > > needed image?
>
> Let me rephrase:
>
>         How can can you continue, when mkimage fails and cannot
>         _succesfully_ generate the needed image?
>
> > > In your patch 2/2 we have this:
> > >
> > > +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > > +        except Exception as e:
> > > +            tout.Error("mkimage failed: %s" % e)
> > > +
> > >          self.SetContents(tools.ReadFile(output_fname))
> > >
> > > mkimage is supposed to create an output file which name is in
> > > output_fname; if mkimage fails and you continue, the next step is
> > > tools.ReadFile(output_fname) trying to read that file.  How is this
> > > possible?
> >
> > # ls -al mkimage*
> > -rw-r--r-- 1 hthiery hthiery      0 Nov  4 20:28 mkimage-out.spl.mkimage
> > -rw-r--r-- 1 hthiery hthiery 180392 Nov  4 20:28 mkimage.spl.mkimage
> >
> > The file (mkimage-out.spl.mkimage) with size 0 seems to be created.  I
> > assume mkimage will create that.
>
> So in this situation we know that mkimage failed, and it generated
> an empty output file.
>
> I guess the output file is _not_ empty when no errors occur?
>
> which reasonable results can you expect when you ignore an error and
> just continue with garbage data as if nothing happened?
>
> Sorry, but this makes no sense to me.  If there is an error, it
> should be reported and - if possible - handled.  If this is not
> possible, then the correct thing is to abort.  Ignoring errors and
> trying to continue is always the worst thing to do.

The only reason I want to introduce this is because I want to have my
imx8mq board built by CI. This board needs an external HDMI firmware
which is used by mkimage. But because this firmware is not available
in the CI build, it comes to the abort. With other boards it is also
so that in the CI external blobs are not available and these make
nevertheless without error a binman run. In this case only a warning
is output.

I know this is not a perfect solution but I don't know how to get my
board merged without doing this kind of workaround for the U-Boot CI.

-- 
Heiko

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3566 bytes --]

On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
> Hi Wolfgang,
> 
> Am So., 7. Nov. 2021 um 15:48 Uhr schrieb Wolfgang Denk <wd@denx.de>:
> >
> > Dear Heiko,
> >
> > In message <CAEyMn7Zgn1ej3kGzg2kiCyqK5DQhkSdXNAUavZGs+3L_fb1Lvg@mail.gmail.com> you wrote:
> > >
> > > > > While converting to binman for an imx8mq board, it has been found that
> > > > > building in the u-boot CI fails. This is because an imx8mq requires an
> > > > > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > > > > mkimage fails. To work around the problem the exception is caught, an
> > > > > error message is printed and binman continues.
> > > >
> > > > But how can you continue, when mkimage fails and cannot generate the
> > > > needed image?
> >
> > Let me rephrase:
> >
> >         How can can you continue, when mkimage fails and cannot
> >         _succesfully_ generate the needed image?
> >
> > > > In your patch 2/2 we have this:
> > > >
> > > > +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > > > +        except Exception as e:
> > > > +            tout.Error("mkimage failed: %s" % e)
> > > > +
> > > >          self.SetContents(tools.ReadFile(output_fname))
> > > >
> > > > mkimage is supposed to create an output file which name is in
> > > > output_fname; if mkimage fails and you continue, the next step is
> > > > tools.ReadFile(output_fname) trying to read that file.  How is this
> > > > possible?
> > >
> > > # ls -al mkimage*
> > > -rw-r--r-- 1 hthiery hthiery      0 Nov  4 20:28 mkimage-out.spl.mkimage
> > > -rw-r--r-- 1 hthiery hthiery 180392 Nov  4 20:28 mkimage.spl.mkimage
> > >
> > > The file (mkimage-out.spl.mkimage) with size 0 seems to be created.  I
> > > assume mkimage will create that.
> >
> > So in this situation we know that mkimage failed, and it generated
> > an empty output file.
> >
> > I guess the output file is _not_ empty when no errors occur?
> >
> > which reasonable results can you expect when you ignore an error and
> > just continue with garbage data as if nothing happened?
> >
> > Sorry, but this makes no sense to me.  If there is an error, it
> > should be reported and - if possible - handled.  If this is not
> > possible, then the correct thing is to abort.  Ignoring errors and
> > trying to continue is always the worst thing to do.
> 
> The only reason I want to introduce this is because I want to have my
> imx8mq board built by CI. This board needs an external HDMI firmware
> which is used by mkimage. But because this firmware is not available
> in the CI build, it comes to the abort. With other boards it is also
> so that in the CI external blobs are not available and these make
> nevertheless without error a binman run. In this case only a warning
> is output.
> 
> I know this is not a perfect solution but I don't know how to get my
> board merged without doing this kind of workaround for the U-Boot CI.

Unfortunately in these days of needing multiple inputs to create a
functional image and also needing to have CI be able to be at all
useful, what we do in many many many cases is yell loudly to the user
that the resulting file here will NOT work and why.  So yes, some "yell
it won't work but not return non-zero exit status" is the norm.

I would be very much open however to some way to handle this
differently.  Some environment variable our tools check for and then
yell-but-succeed?  Some other idea?  I'm just thinking out loud here.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Rasmus Villemoes]

On 09/11/2021 20.42, Tom Rini wrote:
> On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
>> Hi Wolfgang,
>>

>> I know this is not a perfect solution but I don't know how to get my
>> board merged without doing this kind of workaround for the U-Boot CI.
> 
> Unfortunately in these days of needing multiple inputs to create a
> functional image and also needing to have CI be able to be at all
> useful, what we do in many many many cases is yell loudly to the user
> that the resulting file here will NOT work and why.  So yes, some "yell
> it won't work but not return non-zero exit status" is the norm.
> 
> I would be very much open however to some way to handle this
> differently.  Some environment variable our tools check for and then
> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.

Yes, I believe the build system must be taught some env var (or other
means) for opting in to this behavior. I've mentioned this example
before: Imagine some CI system tracking master branches of various Yocto
meta-layers. One of those recipes is responsible for providing one of
those infamous binary blobs, but one day the recipe is updated to
provide another version, so now the filename is different (this has
happened for some imx8m recently). No amount of yelling is going to
prevent the CI from thinking "great, the U-Boot build succeeded, now
let's deploy that artifact to the test rack and run our test suite".
Insta-brick. Apart from forcing one to drive to the office to unbrick
the victim boards, figuring out what went wrong is also going to be
quite unpleasant, since doing a manual build seems to work just fine.

And even in the interactive, developer-working-in-a-terminal, case,
there's no guarantee that the yelling hasn't scrolled away because of
other make output after the should-have-failed target has been "built".

Rasmus

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Rasmus Villemoes]

On 10/11/2021 01.18, Rasmus Villemoes wrote:
> On 09/11/2021 20.42, Tom Rini wrote:
>> On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
>>> Hi Wolfgang,
>>>
> 
>>> I know this is not a perfect solution but I don't know how to get my
>>> board merged without doing this kind of workaround for the U-Boot CI.
>>
>> Unfortunately in these days of needing multiple inputs to create a
>> functional image and also needing to have CI be able to be at all
>> useful, what we do in many many many cases is yell loudly to the user
>> that the resulting file here will NOT work and why.  So yes, some "yell
>> it won't work but not return non-zero exit status" is the norm.
>>
>> I would be very much open however to some way to handle this
>> differently.  Some environment variable our tools check for and then
>> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.
> 
> Yes, I believe the build system must be taught some env var (or other
> means) for opting in to this behavior. 

Oh, and it should of course only paper over missing binary blobs, not
arbitrary errors from mkimage or other tools. The easiest way to do that
is probably to create some dummy blob(s) [only when CREATE_BROKEN_IMAGES
is set of course] before calling the tool that will consume the blobs.

Rasmus

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Simon Glass]

Hi,

On Tue, 9 Nov 2021 at 12:42, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
> > Hi Wolfgang,
> >
> > Am So., 7. Nov. 2021 um 15:48 Uhr schrieb Wolfgang Denk <wd@denx.de>:
> > >
> > > Dear Heiko,
> > >
> > > In message <CAEyMn7Zgn1ej3kGzg2kiCyqK5DQhkSdXNAUavZGs+3L_fb1Lvg@mail.gmail.com> you wrote:
> > > >
> > > > > > While converting to binman for an imx8mq board, it has been found that
> > > > > > building in the u-boot CI fails. This is because an imx8mq requires an
> > > > > > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > > > > > mkimage fails. To work around the problem the exception is caught, an
> > > > > > error message is printed and binman continues.
> > > > >
> > > > > But how can you continue, when mkimage fails and cannot generate the
> > > > > needed image?
> > >
> > > Let me rephrase:
> > >
> > >         How can can you continue, when mkimage fails and cannot
> > >         _succesfully_ generate the needed image?
> > >
> > > > > In your patch 2/2 we have this:
> > > > >
> > > > > +            tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
> > > > > +        except Exception as e:
> > > > > +            tout.Error("mkimage failed: %s" % e)
> > > > > +
> > > > >          self.SetContents(tools.ReadFile(output_fname))
> > > > >
> > > > > mkimage is supposed to create an output file which name is in
> > > > > output_fname; if mkimage fails and you continue, the next step is
> > > > > tools.ReadFile(output_fname) trying to read that file.  How is this
> > > > > possible?
> > > >
> > > > # ls -al mkimage*
> > > > -rw-r--r-- 1 hthiery hthiery      0 Nov  4 20:28 mkimage-out.spl.mkimage
> > > > -rw-r--r-- 1 hthiery hthiery 180392 Nov  4 20:28 mkimage.spl.mkimage
> > > >
> > > > The file (mkimage-out.spl.mkimage) with size 0 seems to be created.  I
> > > > assume mkimage will create that.
> > >
> > > So in this situation we know that mkimage failed, and it generated
> > > an empty output file.
> > >
> > > I guess the output file is _not_ empty when no errors occur?
> > >
> > > which reasonable results can you expect when you ignore an error and
> > > just continue with garbage data as if nothing happened?
> > >
> > > Sorry, but this makes no sense to me.  If there is an error, it
> > > should be reported and - if possible - handled.  If this is not
> > > possible, then the correct thing is to abort.  Ignoring errors and
> > > trying to continue is always the worst thing to do.
> >
> > The only reason I want to introduce this is because I want to have my
> > imx8mq board built by CI. This board needs an external HDMI firmware
> > which is used by mkimage. But because this firmware is not available
> > in the CI build, it comes to the abort. With other boards it is also
> > so that in the CI external blobs are not available and these make
> > nevertheless without error a binman run. In this case only a warning
> > is output.
> >
> > I know this is not a perfect solution but I don't know how to get my
> > board merged without doing this kind of workaround for the U-Boot CI.
>
> Unfortunately in these days of needing multiple inputs to create a
> functional image and also needing to have CI be able to be at all
> useful, what we do in many many many cases is yell loudly to the user
> that the resulting file here will NOT work and why.  So yes, some "yell
> it won't work but not return non-zero exit status" is the norm.
>
> I would be very much open however to some way to handle this
> differently.  Some environment variable our tools check for and then
> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.

I think the approach in this series works, but it might be nicer to:

- output a special error return code from mkimage when a file is missing
- make sure that binman detects this situation and reports its warning
about some blobs being missing

Regards,
Simon

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

On Wed, Nov 10, 2021 at 01:26:12AM +0100, Rasmus Villemoes wrote:
> On 10/11/2021 01.18, Rasmus Villemoes wrote:
> > On 09/11/2021 20.42, Tom Rini wrote:
> >> On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
> >>> Hi Wolfgang,
> >>>
> > 
> >>> I know this is not a perfect solution but I don't know how to get my
> >>> board merged without doing this kind of workaround for the U-Boot CI.
> >>
> >> Unfortunately in these days of needing multiple inputs to create a
> >> functional image and also needing to have CI be able to be at all
> >> useful, what we do in many many many cases is yell loudly to the user
> >> that the resulting file here will NOT work and why.  So yes, some "yell
> >> it won't work but not return non-zero exit status" is the norm.
> >>
> >> I would be very much open however to some way to handle this
> >> differently.  Some environment variable our tools check for and then
> >> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.
> > 
> > Yes, I believe the build system must be taught some env var (or other
> > means) for opting in to this behavior. 
> 
> Oh, and it should of course only paper over missing binary blobs, not
> arbitrary errors from mkimage or other tools. The easiest way to do that
> is probably to create some dummy blob(s) [only when CREATE_BROKEN_IMAGES
> is set of course] before calling the tool that will consume the blobs.

Some patches along those lines would be most welcome :)

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Michael Walle]

Am 2021-11-10 02:37, schrieb Tom Rini:
> On Wed, Nov 10, 2021 at 01:26:12AM +0100, Rasmus Villemoes wrote:
>> On 10/11/2021 01.18, Rasmus Villemoes wrote:
>> > On 09/11/2021 20.42, Tom Rini wrote:
>> >> On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
>> >>> Hi Wolfgang,
>> >>>
>> >
>> >>> I know this is not a perfect solution but I don't know how to get my
>> >>> board merged without doing this kind of workaround for the U-Boot CI.
>> >>
>> >> Unfortunately in these days of needing multiple inputs to create a
>> >> functional image and also needing to have CI be able to be at all
>> >> useful, what we do in many many many cases is yell loudly to the user
>> >> that the resulting file here will NOT work and why.  So yes, some "yell
>> >> it won't work but not return non-zero exit status" is the norm.
>> >>
>> >> I would be very much open however to some way to handle this
>> >> differently.  Some environment variable our tools check for and then
>> >> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.
>> >
>> > Yes, I believe the build system must be taught some env var (or other
>> > means) for opting in to this behavior.
>> 
>> Oh, and it should of course only paper over missing binary blobs, not
>> arbitrary errors from mkimage or other tools. The easiest way to do 
>> that
>> is probably to create some dummy blob(s) [only when 
>> CREATE_BROKEN_IMAGES
>> is set of course] before calling the tool that will consume the blobs.
> 
> Some patches along those lines would be most welcome :)

If I understand Simon correctly, then it is possible to fill in missing
blobs later with binman (or an fit image in general?). Which is quite
useful in a build system, because you don't have a chain of dependent
packages, but you can assemble all the binaries in a final step.

But this also means, that "missing binary blobs" are not an error but
may even be expected. Unfortunately, this all falls apart if the binary
is inside an image produced by mkimage (like the hdmi phy firmware in
for the imx8). Though the finaly image is generated by binman, you
cannot replace/insert the hdmi phy firmware later on [except you'd
unpack the imx image, replace the firmware, pack it again and then
replace it again in the outer image]. IMHO quite a drawback if we're
saying binman isn't replacing mkimage.

-michael

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Simon Glass]

Hi Michael,

On Wed, 10 Nov 2021 at 01:28, Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-11-10 02:37, schrieb Tom Rini:
> > On Wed, Nov 10, 2021 at 01:26:12AM +0100, Rasmus Villemoes wrote:
> >> On 10/11/2021 01.18, Rasmus Villemoes wrote:
> >> > On 09/11/2021 20.42, Tom Rini wrote:
> >> >> On Tue, Nov 09, 2021 at 08:21:07PM +0100, Heiko Thiery wrote:
> >> >>> Hi Wolfgang,
> >> >>>
> >> >
> >> >>> I know this is not a perfect solution but I don't know how to get my
> >> >>> board merged without doing this kind of workaround for the U-Boot CI.
> >> >>
> >> >> Unfortunately in these days of needing multiple inputs to create a
> >> >> functional image and also needing to have CI be able to be at all
> >> >> useful, what we do in many many many cases is yell loudly to the user
> >> >> that the resulting file here will NOT work and why.  So yes, some "yell
> >> >> it won't work but not return non-zero exit status" is the norm.
> >> >>
> >> >> I would be very much open however to some way to handle this
> >> >> differently.  Some environment variable our tools check for and then
> >> >> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.
> >> >
> >> > Yes, I believe the build system must be taught some env var (or other
> >> > means) for opting in to this behavior.
> >>
> >> Oh, and it should of course only paper over missing binary blobs, not
> >> arbitrary errors from mkimage or other tools. The easiest way to do
> >> that
> >> is probably to create some dummy blob(s) [only when
> >> CREATE_BROKEN_IMAGES
> >> is set of course] before calling the tool that will consume the blobs.
> >
> > Some patches along those lines would be most welcome :)
>
> If I understand Simon correctly, then it is possible to fill in missing
> blobs later with binman (or an fit image in general?). Which is quite
> useful in a build system, because you don't have a chain of dependent
> packages, but you can assemble all the binaries in a final step.

Yes binman is designed to handle that final step, but it is also
useful to do a 'preiminary' build in-tree when there are missing
images.

It is possible to use the 'replace' command to update images after the
image is built. It should continue to follow the various rules in the
image definition. But it is better to just build the whole image from
scratch once everything is there.

>
> But this also means, that "missing binary blobs" are not an error but
> may even be expected. Unfortunately, this all falls apart if the binary
> is inside an image produced by mkimage (like the hdmi phy firmware in
> for the imx8). Though the finaly image is generated by binman, you
> cannot replace/insert the hdmi phy firmware later on [except you'd
> unpack the imx image, replace the firmware, pack it again and then
> replace it again in the outer image]. IMHO quite a drawback if we're
> saying binman isn't replacing mkimage.

We need all the binaries to be available at once to package the image.

Looking at the above, we could perhaps do one of two things:

- add a flag to mkimage to tell it to use an empty file for anything
missing (would need to know if it did so with an exit code)
- update binman to create empty files for mkimage for anything that is
missing, before calling mkimage (this would be a little harder as we
would need to understand the FIT format in the image defintiion)

That way, binman can know whether the image is valid or not.

Regards,
Simon

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Wolfgang Denk]

Dear Heiko,

In message <CAEyMn7Y7t63GwPLNd-CSZiHanE_UmpOReYS1cFAz8sq0M=EYmA@mail.gmail.com> you wrote:
>
> > Sorry, but this makes no sense to me.  If there is an error, it
> > should be reported and - if possible - handled.  If this is not
> > possible, then the correct thing is to abort.  Ignoring errors and
> > trying to continue is always the worst thing to do.
>
> The only reason I want to introduce this is because I want to have my
> imx8mq board built by CI. This board needs an external HDMI firmware
> which is used by mkimage. But because this firmware is not available
> in the CI build, it comes to the abort. With other boards it is also
> so that in the CI external blobs are not available and these make
> nevertheless without error a binman run. In this case only a warning
> is output.
>
> I know this is not a perfect solution but I don't know how to get my
> board merged without doing this kind of workaround for the U-Boot CI.

It's not only not a perfect solution, it is the intentional
introduction of a bug, and thus totally unacceptable.

If there is a file missing in the CI, then add/create it there.

But do remove necessary error handling which would cause hard to
detec failures when for normal use.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The moral of the story is: "Don't stop to  tighten  your  shoe  laces
during the Olympics 100m finals".
                             - Kevin Jones in <DEJo68.K1t@bri.hp.com>

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Wolfgang Denk]

Dear Tom,

In message <20211109194224.GB24579@bill-the-cat> you wrote:
> 
> > The only reason I want to introduce this is because I want to have my
> > imx8mq board built by CI. This board needs an external HDMI firmware
> > which is used by mkimage. But because this firmware is not available
> > in the CI build, it comes to the abort. With other boards it is also
> > so that in the CI external blobs are not available and these make
> > nevertheless without error a binman run. In this case only a warning
> > is output.
...
> Unfortunately in these days of needing multiple inputs to create a
> functional image and also needing to have CI be able to be at all
> useful, what we do in many many many cases is yell loudly to the user
> that the resulting file here will NOT work and why.  So yes, some "yell
> it won't work but not return non-zero exit status" is the norm.

This is a terrible degradtion from standard programming style, then.

> I would be very much open however to some way to handle this
> differently.  Some environment variable our tools check for and then
> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.

Well, why not fix the root cause?

Heiko writes that "an external HDMI firmware" is needed - so the fix
is to provide one, or at least a dummy file which is good enough for
the build to succeed.  It should be trivial to create a dummy file
in the CI context, no?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"A little knowledge is a dangerous thing."                - Doug Gwyn

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Wolfgang Denk]

Dear Rasmus,

In message <3253160d-b2e1-2101-5cd4-b8549b5acbae@prevas.dk> you wrote:
>
> > Yes, I believe the build system must be taught some env var (or other
> > means) for opting in to this behavior. 
>
> Oh, and it should of course only paper over missing binary blobs, not
> arbitrary errors from mkimage or other tools. The easiest way to do that
> is probably to create some dummy blob(s) [only when CREATE_BROKEN_IMAGES
> is set of course] before calling the tool that will consume the blobs.

Would it not make much more sense that the CI actually creates the
needed data, at least dummy versions of it?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
:       I've tried (in vi) "g/[a-z]\n[a-z]/s//_/"...but that doesn't
: cut it.  Any ideas?  (I take it that it may be a two-pass sort of solution).
In the first pass, install perl. :-) Larry Wall <6849@jpl-devvax.JPL.NASA.GOV>

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Heiko Thiery]

Hi Wolfgang,

Am Do., 11. Nov. 2021 um 13:24 Uhr schrieb Wolfgang Denk <wd@denx.de>:
>
> Dear Heiko,
>
> In message <CAEyMn7Y7t63GwPLNd-CSZiHanE_UmpOReYS1cFAz8sq0M=EYmA@mail.gmail.com> you wrote:
> >
> > > Sorry, but this makes no sense to me.  If there is an error, it
> > > should be reported and - if possible - handled.  If this is not
> > > possible, then the correct thing is to abort.  Ignoring errors and
> > > trying to continue is always the worst thing to do.
> >
> > The only reason I want to introduce this is because I want to have my
> > imx8mq board built by CI. This board needs an external HDMI firmware
> > which is used by mkimage. But because this firmware is not available
> > in the CI build, it comes to the abort. With other boards it is also
> > so that in the CI external blobs are not available and these make
> > nevertheless without error a binman run. In this case only a warning
> > is output.
> >
> > I know this is not a perfect solution but I don't know how to get my
> > board merged without doing this kind of workaround for the U-Boot CI.
>
> It's not only not a perfect solution, it is the intentional
> introduction of a bug, and thus totally unacceptable.
>
> If there is a file missing in the CI, then add/create it there.
>
> But do remove necessary error handling which would cause hard to
> detec failures when for normal use.

I understand your attitude and am meanwhile also of this opinion. The
idea with the creation of dummy files I also had. I think we should go
in this direction. Everything else would probably be connected with
bigger changes.

-- 
Heiko

Re: [RFC 0/2] Do not stop with an error when mkimage fails

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

On Thu, Nov 11, 2021 at 01:27:22PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211109194224.GB24579@bill-the-cat> you wrote:
> > 
> > > The only reason I want to introduce this is because I want to have my
> > > imx8mq board built by CI. This board needs an external HDMI firmware
> > > which is used by mkimage. But because this firmware is not available
> > > in the CI build, it comes to the abort. With other boards it is also
> > > so that in the CI external blobs are not available and these make
> > > nevertheless without error a binman run. In this case only a warning
> > > is output.
> ...
> > Unfortunately in these days of needing multiple inputs to create a
> > functional image and also needing to have CI be able to be at all
> > useful, what we do in many many many cases is yell loudly to the user
> > that the resulting file here will NOT work and why.  So yes, some "yell
> > it won't work but not return non-zero exit status" is the norm.
> 
> This is a terrible degradtion from standard programming style, then.

Yes, there's a lot of things to grumble about with firmware for modern
SoCs.

> > I would be very much open however to some way to handle this
> > differently.  Some environment variable our tools check for and then
> > yell-but-succeed?  Some other idea?  I'm just thinking out loud here.
> 
> Well, why not fix the root cause?
> 
> Heiko writes that "an external HDMI firmware" is needed - so the fix
> is to provide one, or at least a dummy file which is good enough for
> the build to succeed.  It should be trivial to create a dummy file
> in the CI context, no?

Yes, generally we provide some dummy file so that we can link and
complain to stdout that things won't function.  This series is (and it
seems like within the confines of "this sucks to have to do as a
concept") updating that mechanism to cover yet another case where we
need some external blob that we either can't redistribute or it would
just be wrong to fork and include some other project within our sources.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]