All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Makefile: Deal with missing blobs consistently
@ 2022-10-10 20:00 Simon Glass
  2022-10-10 20:00 ` [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag Simon Glass
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Simon Glass @ 2022-10-10 20:00 UTC (permalink / raw)
  To: u-boot
  Cc: Rasmus Villemoes, Tom Rini, Simon Glass, Alper Nebi Yasak,
	Heinrich Schuchardt, Marek Behún, Pali Rohár,
	Quentin Schulz

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(-)

-- 
2.38.0.rc2.412.g84df46c1b4-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag
  2022-10-10 20:00 [PATCH 0/5] Makefile: Deal with missing blobs consistently Simon Glass
@ 2022-10-10 20:00 ` Simon Glass
  2022-10-10 20:36   ` Tom Rini
  2022-10-10 20:00 ` [PATCH 2/5] Makefile: Correct a missing FORCE on the binman rule Simon Glass
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Simon Glass @ 2022-10-10 20:00 UTC (permalink / raw)
  To: u-boot; +Cc: Rasmus Villemoes, Tom Rini, Simon Glass

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)
-- 
2.38.0.rc2.412.g84df46c1b4-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/5] Makefile: Correct a missing FORCE on the binman rule
  2022-10-10 20:00 [PATCH 0/5] Makefile: Deal with missing blobs consistently Simon Glass
  2022-10-10 20:00 ` [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag Simon Glass
@ 2022-10-10 20:00 ` Simon Glass
  2022-10-10 20:07   ` Pali Rohár
  2022-10-10 20:00 ` [PATCH 3/5] doc: Correct the path to the Makefile documentation Simon Glass
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Simon Glass @ 2022-10-10 20:00 UTC (permalink / raw)
  To: u-boot
  Cc: Rasmus Villemoes, Tom Rini, Simon Glass, Heinrich Schuchardt,
	Marek Behún, Pali Rohár, Quentin Schulz

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
-- 
2.38.0.rc2.412.g84df46c1b4-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] doc: Correct the path to the Makefile documentation
  2022-10-10 20:00 [PATCH 0/5] Makefile: Deal with missing blobs consistently Simon Glass
  2022-10-10 20:00 ` [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag Simon Glass
  2022-10-10 20:00 ` [PATCH 2/5] Makefile: Correct a missing FORCE on the binman rule Simon Glass
@ 2022-10-10 20:00 ` Simon Glass
  2022-10-10 20:00 ` [PATCH 4/5] binman: Use an exit code when blobs are missing Simon Glass
  2022-10-10 20:00 ` [PATCH 5/5] buildman: Detect binman reporting missing blobs Simon Glass
  4 siblings, 0 replies; 15+ messages in thread
From: Simon Glass @ 2022-10-10 20:00 UTC (permalink / raw)
  To: u-boot; +Cc: Rasmus Villemoes, Tom Rini, Simon Glass

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
-- 
2.38.0.rc2.412.g84df46c1b4-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-10 20:00 [PATCH 0/5] Makefile: Deal with missing blobs consistently Simon Glass
                   ` (2 preceding siblings ...)
  2022-10-10 20:00 ` [PATCH 3/5] doc: Correct the path to the Makefile documentation Simon Glass
@ 2022-10-10 20:00 ` Simon Glass
  2022-10-10 20:40   ` Tom Rini
  2022-10-11  8:07   ` Quentin Schulz
  2022-10-10 20:00 ` [PATCH 5/5] buildman: Detect binman reporting missing blobs Simon Glass
  4 siblings, 2 replies; 15+ messages in thread
From: Simon Glass @ 2022-10-10 20:00 UTC (permalink / raw)
  To: u-boot; +Cc: Rasmus Villemoes, Tom Rini, Simon Glass, Alper Nebi Yasak

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()
-- 
2.38.0.rc2.412.g84df46c1b4-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] buildman: Detect binman reporting missing blobs
  2022-10-10 20:00 [PATCH 0/5] Makefile: Deal with missing blobs consistently Simon Glass
                   ` (3 preceding siblings ...)
  2022-10-10 20:00 ` [PATCH 4/5] binman: Use an exit code when blobs are missing Simon Glass
@ 2022-10-10 20:00 ` Simon Glass
  4 siblings, 0 replies; 15+ messages in thread
From: Simon Glass @ 2022-10-10 20:00 UTC (permalink / raw)
  To: u-boot; +Cc: Rasmus Villemoes, Tom Rini, Simon Glass

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 + '/', '')
-- 
2.38.0.rc2.412.g84df46c1b4-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/5] Makefile: Correct a missing FORCE on the binman rule
  2022-10-10 20:00 ` [PATCH 2/5] Makefile: Correct a missing FORCE on the binman rule Simon Glass
@ 2022-10-10 20:07   ` Pali Rohár
  0 siblings, 0 replies; 15+ messages in thread
From: Pali Rohár @ 2022-10-10 20:07 UTC (permalink / raw)
  To: Simon Glass
  Cc: u-boot, Rasmus Villemoes, Tom Rini, Heinrich Schuchardt,
	Marek Behún, Quentin Schulz

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
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag
  2022-10-10 20:00 ` [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag Simon Glass
@ 2022-10-10 20:36   ` Tom Rini
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Rini @ 2022-10-10 20:36 UTC (permalink / raw)
  To: Simon Glass; +Cc: u-boot, Rasmus Villemoes

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

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>

-- 
Tom

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-10 20:00 ` [PATCH 4/5] binman: Use an exit code when blobs are missing Simon Glass
@ 2022-10-10 20:40   ` Tom Rini
  2022-10-10 22:25     ` Simon Glass
  2022-10-11  8:07   ` Quentin Schulz
  1 sibling, 1 reply; 15+ messages in thread
From: Tom Rini @ 2022-10-10 20:40 UTC (permalink / raw)
  To: Simon Glass; +Cc: u-boot, Rasmus Villemoes, Alper Nebi Yasak

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

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.

-- 
Tom

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-10 20:40   ` Tom Rini
@ 2022-10-10 22:25     ` Simon Glass
  2022-10-11 21:05       ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: Simon Glass @ 2022-10-10 22:25 UTC (permalink / raw)
  To: Tom Rini; +Cc: u-boot, Rasmus Villemoes, Alper Nebi Yasak

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-10 20:00 ` [PATCH 4/5] binman: Use an exit code when blobs are missing Simon Glass
  2022-10-10 20:40   ` Tom Rini
@ 2022-10-11  8:07   ` Quentin Schulz
  1 sibling, 0 replies; 15+ messages in thread
From: Quentin Schulz @ 2022-10-11  8:07 UTC (permalink / raw)
  To: Simon Glass, u-boot; +Cc: Rasmus Villemoes, Tom Rini, Alper Nebi Yasak

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-10 22:25     ` Simon Glass
@ 2022-10-11 21:05       ` Tom Rini
  2022-10-12 12:59         ` Simon Glass
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2022-10-11 21:05 UTC (permalink / raw)
  To: Simon Glass; +Cc: u-boot, Rasmus Villemoes, Alper Nebi Yasak

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

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.

-- 
Tom

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-11 21:05       ` Tom Rini
@ 2022-10-12 12:59         ` Simon Glass
  2022-10-12 14:29           ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: Simon Glass @ 2022-10-12 12:59 UTC (permalink / raw)
  To: Tom Rini; +Cc: u-boot, Rasmus Villemoes, Alper Nebi Yasak

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-12 12:59         ` Simon Glass
@ 2022-10-12 14:29           ` Tom Rini
  2022-10-14 15:56             ` Simon Glass
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2022-10-12 14:29 UTC (permalink / raw)
  To: Simon Glass; +Cc: u-boot, Rasmus Villemoes, Alper Nebi Yasak

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

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.

-- 
Tom

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] binman: Use an exit code when blobs are missing
  2022-10-12 14:29           ` Tom Rini
@ 2022-10-14 15:56             ` Simon Glass
  0 siblings, 0 replies; 15+ messages in thread
From: Simon Glass @ 2022-10-14 15:56 UTC (permalink / raw)
  To: Tom Rini; +Cc: u-boot, Rasmus Villemoes, Alper Nebi Yasak

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@chromium.org/

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-10-14 15:59 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-10 20:00 [PATCH 0/5] Makefile: Deal with missing blobs consistently Simon Glass
2022-10-10 20:00 ` [PATCH 1/5] buildman: Handle the MAINTAINERS 'N' tag Simon Glass
2022-10-10 20:36   ` Tom Rini
2022-10-10 20:00 ` [PATCH 2/5] Makefile: Correct a missing FORCE on the binman rule Simon Glass
2022-10-10 20:07   ` Pali Rohár
2022-10-10 20:00 ` [PATCH 3/5] doc: Correct the path to the Makefile documentation Simon Glass
2022-10-10 20:00 ` [PATCH 4/5] binman: Use an exit code when blobs are missing Simon Glass
2022-10-10 20:40   ` Tom Rini
2022-10-10 22:25     ` Simon Glass
2022-10-11 21:05       ` Tom Rini
2022-10-12 12:59         ` Simon Glass
2022-10-12 14:29           ` Tom Rini
2022-10-14 15:56             ` Simon Glass
2022-10-11  8:07   ` Quentin Schulz
2022-10-10 20:00 ` [PATCH 5/5] buildman: Detect binman reporting missing blobs Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.