[PATCH 0/5] Makefile: Deal with missing blobs consistently
Missing blobs should cause the build (with make) to fail, but at present success is returned. This is because binman currently produces an exit code of 0 in this case.
Of course this is not correct, since the images cannot actually be used.
This series fixes that and adjusts buildman to deal sensibly with the situation.
It also includes a buildman patch to deal with N: entries in the MAINTAINER files and a few other minor niggles noticed along the way.
Simon Glass (5): buildman: Handle the MAINTAINERS 'N' tag Makefile: Correct a missing FORCE on the binman rule doc: Correct the path to the Makefile documentation binman: Use an exit code when blobs are missing buildman: Detect binman reporting missing blobs
Makefile | 2 +- scripts/Kbuild.include | 2 +- tools/binman/binman.rst | 4 ++++ tools/binman/cmdline.py | 3 +++ tools/binman/control.py | 7 ++++++- tools/buildman/boards.py | 11 +++++++++++ tools/buildman/builderthread.py | 6 +++++- 7 files changed, 31 insertions(+), 4 deletions(-)
This is needed for some soon-to-be-applied patches. Scan the configs/ directory to see if any of the files match.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Tom Rini trini@konsulko.com ---
tools/buildman/boards.py | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/buildman/boards.py b/tools/buildman/boards.py index cdc4d9ffd27..0bb0723b18e 100644 --- a/tools/buildman/boards.py +++ b/tools/buildman/boards.py @@ -368,6 +368,17 @@ class MaintainersDatabase: targets.append(front) elif tag == 'S:': status = rest + elif tag == 'N:': + # Just scan the configs directory since that's all we care + # about + for dirpath, _, fnames in os.walk('configs'): + for fname in fnames: + path = os.path.join(dirpath, fname) + front, match, rear = path.partition('configs/') + if not front and match: + front, match, rear = rear.rpartition('_defconfig') + if match and not rear: + targets.append(front) elif line == '\n': for target in targets: self.database[target] = (status, maintainers)
On Mon, Oct 10, 2022 at 02:00:28PM -0600, Simon Glass wrote:
This is needed for some soon-to-be-applied patches. Scan the configs/ directory to see if any of the files match.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Tom Rini trini@konsulko.com
Putting this on my local branch with many defconfigs listed under "N:" and confirming it works:
Tested-by: Tom Rini trini@konsulko.com
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org ---
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 45f10759a13..2e8ae768c51 100644 --- a/Makefile +++ b/Makefile @@ -1111,7 +1111,7 @@ endef PHONY += inputs inputs: $(INPUTS-y)
-all: .binman_stamp inputs +all: .binman_stamp inputs FORCE ifeq ($(CONFIG_BINMAN),y) $(call if_changed,binman) endif
On Monday 10 October 2022 14:00:29 Simon Glass wrote:
This is required for if_changed to work correctly. Add it.
That is truth.
Reviewed-by: Pali Rohár pali@kernel.org
Signed-off-by: Simon Glass sjg@chromium.org
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 45f10759a13..2e8ae768c51 100644 --- a/Makefile +++ b/Makefile @@ -1111,7 +1111,7 @@ endef PHONY += inputs inputs: $(INPUTS-y)
-all: .binman_stamp inputs +all: .binman_stamp inputs FORCE ifeq ($(CONFIG_BINMAN),y) $(call if_changed,binman) endif -- 2.38.0.rc2.412.g84df46c1b4-goog
This is out-of-date now. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
scripts/Kbuild.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 9c14310ad40..62e0207f91b 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -229,7 +229,7 @@ objectify = $(foreach o,$(1),$(if $(filter /%,$(o)),$(o),$(obj)/$(o))) # if_changed_dep - as if_changed, but uses fixdep to reveal dependencies # including used config symbols # if_changed_rule - as if_changed but execute rule instead -# See Documentation/kbuild/makefiles.txt for more info +# See doc/develop/makefiles.rst for more info
ifneq ($(KBUILD_NOCMDDEP),1) # Check if both arguments are the same including their order. Result is empty
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/binman/binman.rst | 4 ++++ tools/binman/cmdline.py | 3 +++ tools/binman/control.py | 7 ++++++- 3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 4ee6f41f35e..c0512e68e67 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -1458,6 +1458,10 @@ space-separated list of directories to search for binary blobs:: odroid-c4/build/board/hardkernel/odroidc4/firmware \ odroid-c4/build/scp_task" binman ...
+Note that binman fails with error code 103 when there are missing blobs. If you +wish to continue anyway, you can pass `-W` to binman. + + Code coverage -------------
diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 1d1ca43993d..144c0c916a2 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -128,6 +128,9 @@ controlled by a description in the board device tree.''' default=False, help='Update the binman node with offset/size info') build_parser.add_argument('--update-fdt-in-elf', type=str, help='Update an ELF file with the output dtb: infile,outfile,begin_sym,end_sym') + build_parser.add_argument( + '-W', '--ignore-missing-blobs', action='store_true', default=False, + help='Return success even if there are missing blobs')
subparsers.add_parser( 'bintool-docs', help='Write out bintool documentation (see bintool.rst)') diff --git a/tools/binman/control.py b/tools/binman/control.py index bfe63a15204..119b1176b7a 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -742,7 +742,12 @@ def Binman(args): elf.UpdateFile(*elf_params, data)
if invalid: - tout.warning("\nSome images are invalid") + msg = '\nSome images are invalid' + if args.ignore_missing_blobs: + tout.warning(msg) + else: + tout.error("\nSome images are invalid") + return 103
# Use this to debug the time take to pack the image #state.TimingShow()
On Mon, Oct 10, 2022 at 02:00:31PM -0600, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
I'm still not sure I like this, rather than changing the default "make" behavior. And then it gets me a bit worried that we have CI not doing some other things "right" as we don't want to ignore warnings in CI, we want warnings to become errors so that new warnings don't make it in-tree.
Hi Tom,
On Mon, 10 Oct 2022 at 14:41, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 02:00:31PM -0600, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
I'm still not sure I like this, rather than changing the default "make" behavior.
I did change that, in the sense that 'make' now fails if there are missing blobs.
And then it gets me a bit worried that we have CI not doing some other things "right" as we don't want to ignore warnings in CI, we want warnings to become errors so that new warnings don't make it in-tree.
That would be quite a big effort, and unrelated to this series. Here are some warning types I'm aware of:
- migration - device tree - compiler - missing blobs
Do we need a per-board whitelist for warnings? It seems pretty tricky to me.
It is true that warnings are ignored in CI and this does create problems...I'd love to make them into errors if we can.
Regards, Simon
On Mon, Oct 10, 2022 at 04:25:36PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 14:41, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 02:00:31PM -0600, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
I'm still not sure I like this, rather than changing the default "make" behavior.
I did change that, in the sense that 'make' now fails if there are missing blobs.
And then it gets me a bit worried that we have CI not doing some other things "right" as we don't want to ignore warnings in CI, we want warnings to become errors so that new warnings don't make it in-tree.
That would be quite a big effort, and unrelated to this series. Here are some warning types I'm aware of:
- migration
- device tree
- compiler
- missing blobs
Do we need a per-board whitelist for warnings? It seems pretty tricky to me.
It is true that warnings are ignored in CI and this does create problems...I'd love to make them into errors if we can.
This is I guess also a related concern. When I say warnings, I mean C compiler warnings. That we have flags to suppress "warnings" that are other kinds of valid warnings is at least a little confusing.
Hi Tom,
On Tue, 11 Oct 2022 at 15:05, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 04:25:36PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 14:41, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 02:00:31PM -0600, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
I'm still not sure I like this, rather than changing the default "make" behavior.
I did change that, in the sense that 'make' now fails if there are missing blobs.
And then it gets me a bit worried that we have CI not doing some other things "right" as we don't want to ignore warnings in CI, we want warnings to become errors so that new warnings don't make it in-tree.
That would be quite a big effort, and unrelated to this series. Here are some warning types I'm aware of:
- migration
- device tree
- compiler
- missing blobs
Do we need a per-board whitelist for warnings? It seems pretty tricky to me.
It is true that warnings are ignored in CI and this does create problems...I'd love to make them into errors if we can.
This is I guess also a related concern. When I say warnings, I mean C compiler warnings. That we have flags to suppress "warnings" that are other kinds of valid warnings is at least a little confusing.
Yes. But of course binman doesn't have any other kind of warning, so so long as we don't propagate that flag further, it makes sense I think.
The warnings in U-Boot are at least somewhat out of control IMO, as per my list above.
Regards, Simon
On Wed, Oct 12, 2022 at 06:59:21AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 11 Oct 2022 at 15:05, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 04:25:36PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 14:41, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 02:00:31PM -0600, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
I'm still not sure I like this, rather than changing the default "make" behavior.
I did change that, in the sense that 'make' now fails if there are missing blobs.
And then it gets me a bit worried that we have CI not doing some other things "right" as we don't want to ignore warnings in CI, we want warnings to become errors so that new warnings don't make it in-tree.
That would be quite a big effort, and unrelated to this series. Here are some warning types I'm aware of:
- migration
- device tree
- compiler
- missing blobs
Do we need a per-board whitelist for warnings? It seems pretty tricky to me.
It is true that warnings are ignored in CI and this does create problems...I'd love to make them into errors if we can.
This is I guess also a related concern. When I say warnings, I mean C compiler warnings. That we have flags to suppress "warnings" that are other kinds of valid warnings is at least a little confusing.
Yes. But of course binman doesn't have any other kind of warning, so so long as we don't propagate that flag further, it makes sense I think.
The warnings in U-Boot are at least somewhat out of control IMO, as per my list above.
Well, lets dig down in to those for a minute?
For "missing blobs", if we're passing the "fake these blobs" flag AND we don't ever default to passing that flag in regular build rules, I can see making it so that we only see those messages either when we don't pass the fake these blobs flag or we have a verbose option set.
For device tree warnings? I think we're mirroring the kernel still in terms of ignoring problems so it's likely a matter of poking board maintainers to resync their trees and/or address the warnings upstream too (which upstream would appreciate).
For migration warnings, well, which? Are there some that can be easily done and compile tested? Looking at the Makefile atm, I think the CONFIG_DM migration warning logic can go away as there's no boards missing that. There's nothing in the "this passed so long ago we should delete boards" list, but it would be a good idea (so I'll fire off some builds in a moment) to see who trips up on each and email maintainers.
Hi Tom,
On Wed, 12 Oct 2022 at 08:29, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 12, 2022 at 06:59:21AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 11 Oct 2022 at 15:05, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 04:25:36PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 10 Oct 2022 at 14:41, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 10, 2022 at 02:00:31PM -0600, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
I'm still not sure I like this, rather than changing the default "make" behavior.
I did change that, in the sense that 'make' now fails if there are missing blobs.
And then it gets me a bit worried that we have CI not doing some other things "right" as we don't want to ignore warnings in CI, we want warnings to become errors so that new warnings don't make it in-tree.
That would be quite a big effort, and unrelated to this series. Here are some warning types I'm aware of:
- migration
- device tree
- compiler
- missing blobs
Do we need a per-board whitelist for warnings? It seems pretty tricky to me.
It is true that warnings are ignored in CI and this does create problems...I'd love to make them into errors if we can.
This is I guess also a related concern. When I say warnings, I mean C compiler warnings. That we have flags to suppress "warnings" that are other kinds of valid warnings is at least a little confusing.
Yes. But of course binman doesn't have any other kind of warning, so so long as we don't propagate that flag further, it makes sense I think.
The warnings in U-Boot are at least somewhat out of control IMO, as per my list above.
Well, lets dig down in to those for a minute?
For "missing blobs", if we're passing the "fake these blobs" flag AND we don't ever default to passing that flag in regular build rules, I can see making it so that we only see those messages either when we don't pass the fake these blobs flag or we have a verbose option set.
Yes I suppose we could do that, so long as buildman shows the build as a failure as [1]. We want to make sure that people don't think that the build actually succeeded, even with missing blobs. It can be a warning (suppressed with -W or forced to an error with -E) but it must start off as a warning.
For device tree warnings? I think we're mirroring the kernel still in terms of ignoring problems so it's likely a matter of poking board maintainers to resync their trees and/or address the warnings upstream too (which upstream would appreciate).
Should we start a whitelist here and require new boards to compile cleanly?
For migration warnings, well, which? Are there some that can be easily done and compile tested? Looking at the Makefile atm, I think the CONFIG_DM migration warning logic can go away as there's no boards missing that. There's nothing in the "this passed so long ago we should delete boards" list, but it would be a good idea (so I'll fire off some builds in a moment) to see who trips up on each and email maintainers.
Yes, the Makefile ones. Of course there are quite a few we don't have, like DM_ETH.
Also DM_SPI should be dropped I think, since we finished that some years ago.
I'll start a new thread on DM_SPL
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20221011141541.538175-6-sjg...
Hi Simon,
On 10/10/22 22:00, Simon Glass wrote:
At present binman returns success when told to handle missing blobs. This is confusing this in fact the resulting image cannot work.
Use exit code 103 to signal this problem, with a -W option to convert it to a warning.
Signed-off-by: Simon Glass sjg@chromium.org
tools/binman/binman.rst | 4 ++++ tools/binman/cmdline.py | 3 +++ tools/binman/control.py | 7 ++++++- 3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 4ee6f41f35e..c0512e68e67 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -1458,6 +1458,10 @@ space-separated list of directories to search for binary blobs:: odroid-c4/build/board/hardkernel/odroidc4/firmware \ odroid-c4/build/scp_task" binman ...
+Note that binman fails with error code 103 when there are missing blobs. If you +wish to continue anyway, you can pass `-W` to binman.
Code coverage
diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 1d1ca43993d..144c0c916a2 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -128,6 +128,9 @@ controlled by a description in the board device tree.''' default=False, help='Update the binman node with offset/size info') build_parser.add_argument('--update-fdt-in-elf', type=str, help='Update an ELF file with the output dtb: infile,outfile,begin_sym,end_sym')
build_parser.add_argument(
'-W', '--ignore-missing-blobs', action='store_true', default=False,help='Return success even if there are missing blobs') subparsers.add_parser( 'bintool-docs', help='Write out bintool documentation (see bintool.rst)')diff --git a/tools/binman/control.py b/tools/binman/control.py index bfe63a15204..119b1176b7a 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -742,7 +742,12 @@ def Binman(args): elf.UpdateFile(*elf_params, data)
if invalid:
tout.warning("\nSome images are invalid")
msg = '\nSome images are invalid'if args.ignore_missing_blobs:tout.warning(msg)else:tout.error("\nSome images are invalid")
I think you meant to have msg variable used here.
Also, you could flatten this by having a if not args.ingore_missing_blobs which has the return and then the new tout.warning left on the same indentation level of the current one.
Cheers, Quentin
Buildman should consider a build as a success (with warnings) if missing blobs have been dealt with by binman, even though buildman itself returns and error code overall. This is how other warnings are dealt with.
We cannot easily access the 103 exit code, so detect the problem in the output.
With this change, missing blobs result in an exit code of 101, although they still indicate failure.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/builderthread.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 6240e08c767..065d836d68c 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -288,10 +288,14 @@ class BuilderThread(threading.Thread): args.append('cfg') result = self.Make(commit, brd, 'build', cwd, *args, env=env) + if (result.return_code == 2 and + ('Some images are invalid' in result.stderr)): + # This is handled later by the check for output in + # stderr + result.return_code = 0 if adjust_cfg: errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg) if errs: - print('errs', errs) result.stderr += errs result.return_code = 1 result.stderr = result.stderr.replace(src_dir + '/', '')
participants (4)
-
Pali Rohár -
Quentin Schulz -
Simon Glass -
Tom Rini