[PATCH v2 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.
Changes in v2: - Fix missing usage of the msg variable - Simplify code to remove the 'else'
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 | 6 +++++- tools/buildman/boards.py | 11 +++++++++++ tools/buildman/builderthread.py | 6 +++++- 7 files changed, 30 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 Tested-by: Tom Rini trini@konsulko.com Suggested-by: Tom Rini trini@konsulko.com ---
(no changes since v1)
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 Tue, Oct 11, 2022 at 08:15:37AM -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 Tested-by: Tom Rini trini@konsulko.com Suggested-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org ---
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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 Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote:
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
endif
2.38.0.rc1.362.ged0d419d3c-goog
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote:
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote:
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
I agree, this is really written in the wrong way.
Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands.
And binman call should be moved to different non-phony target with the correct output file name.
endif
2.38.0.rc1.362.ged0d419d3c-goog
Hi Pali,
On Sun, 16 Oct 2022 at 04:16, Pali Rohár pali@kernel.org wrote:
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote:
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote:
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
I agree, this is really written in the wrong way.
Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands.
OK, will need to figure out where to put the binman thing then.
And binman call should be moved to different non-phony target with the correct output file name.
Binman can generate all sorts of files. I will see if I can use .binman-stamp since that is always generated.
Regards, Simon
On Wednesday 19 October 2022 19:55:43 Simon Glass wrote:
Hi Pali,
On Sun, 16 Oct 2022 at 04:16, Pali Rohár pali@kernel.org wrote:
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote:
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote:
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
I agree, this is really written in the wrong way.
Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands.
OK, will need to figure out where to put the binman thing then.
And binman call should be moved to different non-phony target with the correct output file name.
Binman can generate all sorts of files. I will see if I can use .binman-stamp since that is always generated.
Regards, Simon
Would not something like this work?
BINMAN_INPUTS := .... # fill this BINMAN_OUTPUTS := .... # fill this
INPUTS-$(CONFIG_BINMAN) += .binman_stamp
.binman_stamp: $(BINMAN_INPUTS) FORCE $(call if_changed,binman)
$(BINMAN_OUTPUTS): .binman_stamp
all: inputs
Hi Pali,
On Thu, 20 Oct 2022 at 13:54, Pali Rohár pali@kernel.org wrote:
On Wednesday 19 October 2022 19:55:43 Simon Glass wrote:
Hi Pali,
On Sun, 16 Oct 2022 at 04:16, Pali Rohár pali@kernel.org wrote:
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote:
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote:
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
I agree, this is really written in the wrong way.
Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands.
OK, will need to figure out where to put the binman thing then.
And binman call should be moved to different non-phony target with the correct output file name.
Binman can generate all sorts of files. I will see if I can use .binman-stamp since that is always generated.
Regards, Simon
Would not something like this work?
BINMAN_INPUTS := .... # fill this BINMAN_OUTPUTS := .... # fill this
INPUTS-$(CONFIG_BINMAN) += .binman_stamp
.binman_stamp: $(BINMAN_INPUTS) FORCE $(call if_changed,binman)
$(BINMAN_OUTPUTS): .binman_stamp
all: inputs
Sadly we don't know the outputs without asking binman.
I'll send out another rev of this patch by the end of the week.
Regards, Simon
On Friday 21 October 2022 14:17:11 Simon Glass wrote:
Hi Pali,
On Thu, 20 Oct 2022 at 13:54, Pali Rohár pali@kernel.org wrote:
On Wednesday 19 October 2022 19:55:43 Simon Glass wrote:
Hi Pali,
On Sun, 16 Oct 2022 at 04:16, Pali Rohár pali@kernel.org wrote:
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote:
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote:
This is required for if_changed to work correctly. Add it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Pali Rohár pali@kernel.org
(no changes since v1)
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
I agree, this is really written in the wrong way.
Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands.
OK, will need to figure out where to put the binman thing then.
And binman call should be moved to different non-phony target with the correct output file name.
Binman can generate all sorts of files. I will see if I can use .binman-stamp since that is always generated.
Regards, Simon
Would not something like this work?
BINMAN_INPUTS := .... # fill this BINMAN_OUTPUTS := .... # fill this
INPUTS-$(CONFIG_BINMAN) += .binman_stamp
.binman_stamp: $(BINMAN_INPUTS) FORCE $(call if_changed,binman)
$(BINMAN_OUTPUTS): .binman_stamp
all: inputs
Sadly we don't know the outputs without asking binman.
So what about extending binman via some cmdline arg to just prints outputs?
I'll send out another rev of this patch by the end of the week.
Regards, Simon
Hi Pali,
On Fri, 21 Oct 2022 at 14:22, Pali Rohár pali@kernel.org wrote:
On Friday 21 October 2022 14:17:11 Simon Glass wrote:
Hi Pali,
On Thu, 20 Oct 2022 at 13:54, Pali Rohár pali@kernel.org wrote:
On Wednesday 19 October 2022 19:55:43 Simon Glass wrote:
Hi Pali,
On Sun, 16 Oct 2022 at 04:16, Pali Rohár pali@kernel.org wrote:
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote:
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass sjg@chromium.org wrote: > > This is required for if_changed to work correctly. Add it. > > Signed-off-by: Simon Glass sjg@chromium.org > Reviewed-by: Pali Rohár pali@kernel.org > --- > > (no changes since v1) > > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 3866cc62f9a..d28e8b4e316 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)
'all' is usually used as a phony target.
I think this went wrong.
if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact.
FORCE is never used for a phony target because it is added to 'PHONY'.
PHONY += all
I agree, this is really written in the wrong way.
Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands.
OK, will need to figure out where to put the binman thing then.
And binman call should be moved to different non-phony target with the correct output file name.
Binman can generate all sorts of files. I will see if I can use .binman-stamp since that is always generated.
Regards, Simon
Would not something like this work?
BINMAN_INPUTS := .... # fill this BINMAN_OUTPUTS := .... # fill this
INPUTS-$(CONFIG_BINMAN) += .binman_stamp
.binman_stamp: $(BINMAN_INPUTS) FORCE $(call if_changed,binman)
$(BINMAN_OUTPUTS): .binman_stamp
all: inputs
Sadly we don't know the outputs without asking binman.
So what about extending binman via some cmdline arg to just prints outputs?
Yes I've thought about that but not done it. Things to do:
- put intermediate files in a separate output subdir from the final output - have a way to determine the output filenames without actually building the output
I'll send out another rev of this patch by the end of the week.
Regards, Simon
This is out-of-date now. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
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 ---
Changes in v2: - Fix missing usage of the msg variable - Simplify code to remove the 'else'
tools/binman/binman.rst | 4 ++++ tools/binman/cmdline.py | 3 +++ tools/binman/control.py | 6 +++++- 3 files changed, 12 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..da52abad077 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -742,7 +742,11 @@ def Binman(args): elf.UpdateFile(*elf_params, data)
if invalid: - tout.warning("\nSome images are invalid") + msg = '\nSome images are invalid' + if not args.ignore_missing_blobs: + tout.error(msg) + return 103 + tout.warning(msg)
# Use this to debug the time take to pack the image #state.TimingShow()
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 ---
(no changes since v1)
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)
-
Masahiro Yamada -
Pali Rohár -
Simon Glass -
Tom Rini