[RFC 0/2] Do not stop with an error when mkimage fails
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(-)
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):
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
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
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
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
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
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
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_
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
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.
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.
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
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
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 :)
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
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
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
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
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
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.
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
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.
participants (6)
-
Heiko Thiery -
Michael Walle -
Rasmus Villemoes -
Simon Glass -
Tom Rini -
Wolfgang Denk