[U-Boot] [PATCH 0/4] Add support for treating compiler warnings as errors

[U-Boot] [PATCH 0/4] Add support for treating compiler warnings as errors

[Daniel Schwierzeck]

To enforce a zero-warnings policy (e.g. in CI builds), all compiler
warnings have to be treated as errors.

Extend Kbuild and buildman with according options to achieve this.
Enable these new options in all Travis CI builds. All builds with
compiler warnings will now fail. Only DTC warnings are still being
ignored.

Example build which failed due to a compiler warning:
https://travis-ci.org/danielschwierzeck/u-boot/jobs/333349371#L936


Daniel Schwierzeck (4):
  Kbuild: support W=[N,]err for passing '-Werror' to the compiler
  buildman: add option -E for treating compiler warnings as errors
  travis.yml: fix 'set +e' in build script
  travis.yml: run buildman with option -E

 .travis.yml                     | 5 ++---
 scripts/Makefile.extrawarn      | 3 +++
 tools/buildman/builder.py       | 5 ++++-
 tools/buildman/builderthread.py | 2 ++
 tools/buildman/cmdline.py       | 2 ++
 tools/buildman/control.py       | 3 ++-
 6 files changed, 15 insertions(+), 5 deletions(-)

-- 
2.16.1

[U-Boot] [PATCH 1/4] Kbuild: support W=[N, ]err for passing '-Werror' to the compiler

[Daniel Schwierzeck]

Extend the Kbuild's W=N option with an optional 'err' value. This
will pass -Werror to the compiler to treat all warnings as errors.
This is useful to enforce a zero-warnings policy.

The 'err' value can also be combined with the numerical values
like this:

    make W=1
    make W=err
    make W=1,err

Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
---

 scripts/Makefile.extrawarn | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 1d3a570594..d8d93b7fe1 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -48,9 +48,12 @@ warning-3 += -Wswitch-default
 warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
 warning-3 += $(call cc-option, -Wvla)
 
+warning-err := -Werror
+
 warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
 warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
 warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
+warning += $(warning-$(findstring err, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
 
 ifeq ("$(strip $(warning))","")
         $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
-- 
2.16.1

[U-Boot] [PATCH 2/4] buildman: add option -E for treating compiler warnings as errors

[Daniel Schwierzeck]

Add a new option '-E' for treating all compiler warnings as errors.
Eventually this will pass 'W=err' to Kbuild.

Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
---

 tools/buildman/builder.py       | 5 ++++-
 tools/buildman/builderthread.py | 2 ++
 tools/buildman/cmdline.py       | 2 ++
 tools/buildman/control.py       | 3 ++-
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index acb0810457..4e72b7d60d 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -212,7 +212,8 @@ class Builder:
                  gnu_make='make', checkout=True, show_unknown=True, step=1,
                  no_subdirs=False, full_path=False, verbose_build=False,
                  incremental=False, per_board_out_dir=False,
-                 config_only=False, squash_config_y=False):
+                 config_only=False, squash_config_y=False,
+                 warnings_as_errors=False):
         """Create a new Builder object
 
         Args:
@@ -237,6 +238,7 @@ class Builder:
                 board rather than a thread-specific directory
             config_only: Only configure each build, don't build it
             squash_config_y: Convert CONFIG options with the value 'y' to '1'
+            warnings_as_errors: Treat all compiler warnings as errors
         """
         self.toolchains = toolchains
         self.base_dir = base_dir
@@ -270,6 +272,7 @@ class Builder:
         if not self.squash_config_y:
             self.config_filenames += EXTRA_CONFIG_FILENAMES
 
+        self.warnings_as_errors = warnings_as_errors
         self.col = terminal.Color()
 
         self._re_function = re.compile('(.*): In function.*')
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 9e8ca80c5b..4ce3f48c66 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -216,6 +216,8 @@ class BuilderThread(threading.Thread):
                     args.append('-s')
                 if self.builder.num_jobs is not None:
                     args.extend(['-j', str(self.builder.num_jobs)])
+                if self.builder.warnings_as_errors:
+                    args.append('W=err')
                 config_args = ['%s_defconfig' % brd.target]
                 config_out = ''
                 args.extend(self.builder.toolchains.GetMakeArguments(brd))
diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index 74247f0aff..6949d6bf2c 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -32,6 +32,8 @@ def ParseArgs():
           help="Don't build, just configure each commit")
     parser.add_option('-e', '--show_errors', action='store_true',
           default=False, help='Show errors and warnings')
+    parser.add_option('-E', '--warnings-as-errors', action='store_true',
+          default=False, help='Treat all compiler warnings as errors')
     parser.add_option('-f', '--force-build', dest='force_build',
           action='store_true', default=False,
           help='Force build of boards even if already built')
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 73b1a14fb6..3cac9f7cf6 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -263,7 +263,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
             incremental=options.incremental,
             per_board_out_dir=options.per_board_out_dir,
             config_only=options.config_only,
-            squash_config_y=not options.preserve_config_y)
+            squash_config_y=not options.preserve_config_y,
+            warnings_as_errors=options.warnings_as_errors)
     builder.force_config_on_failure = not options.quick
     if make_func:
         builder.do_make = make_func
-- 
2.16.1

[U-Boot] [PATCH 3/4] travis.yml: fix 'set +e' in build script

[Daniel Schwierzeck]

The build script should not manipulate shell flags (especially '-e').
A non-zero exit value can also be catched with 'cmd || ret=$?'.

Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
---

 .travis.yml | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 2a98c4bb11..1e55e1b7f1 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -101,9 +101,8 @@ script:
  #
  # Exit code 129 means warnings only.
  - if [[ "${BUILDMAN}" != "" ]]; then
-     set +e;
-     tools/buildman/buildman -P ${BUILDMAN};
-     ret=$?;
+     ret=0;
+     tools/buildman/buildman -P ${BUILDMAN} || ret=$?;
      if [[ $ret -ne 0 && $ret -ne 129 ]]; then
        tools/buildman/buildman -sdeP ${BUILDMAN};
        exit $ret;
-- 
2.16.1

[U-Boot] [PATCH 4/4] travis.yml: run buildman with option -E

[Daniel Schwierzeck]

This forces all compiler warnings to be treated as errors.

Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>

---

 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 1e55e1b7f1..59d1dd99e8 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -102,7 +102,7 @@ script:
  # Exit code 129 means warnings only.
  - if [[ "${BUILDMAN}" != "" ]]; then
      ret=0;
-     tools/buildman/buildman -P ${BUILDMAN} || ret=$?;
+     tools/buildman/buildman -P -E ${BUILDMAN} || ret=$?;
      if [[ $ret -ne 0 && $ret -ne 129 ]]; then
        tools/buildman/buildman -sdeP ${BUILDMAN};
        exit $ret;
-- 
2.16.1

[U-Boot] [PATCH 1/4] Kbuild: support W=[N, ]err for passing '-Werror' to the compiler

[Masahiro Yamada]

Hi Daniel,


2018-01-26 2:21 GMT+09:00 Daniel Schwierzeck <daniel.schwierzeck@gmail.com>:
> Extend the Kbuild's W=N option with an optional 'err' value. This
> will pass -Werror to the compiler to treat all warnings as errors.
> This is useful to enforce a zero-warnings policy.
>
> The 'err' value can also be combined with the numerical values
> like this:
>
>     make W=1
>     make W=err
>     make W=1,err
>
> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> ---
>
>  scripts/Makefile.extrawarn | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 1d3a570594..d8d93b7fe1 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -48,9 +48,12 @@ warning-3 += -Wswitch-default
>  warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
>  warning-3 += $(call cc-option, -Wvla)
>
> +warning-err := -Werror
> +
>  warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>  warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>  warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
> +warning += $(warning-$(findstring err, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>
>  ifeq ("$(strip $(warning))","")
>          $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
> --
> 2.16.1


I saw a similar patch before in linux-kbuild ML.


Kbuild provides a way to specify user-specific options.
See the following lines in the top-level Makefile:


  # Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments
  KBUILD_CPPFLAGS += $(KCPPFLAGS)
  KBUILD_AFLAGS += $(KAFLAGS)
  KBUILD_CFLAGS += $(KCFLAGS)



"make W=err" is a shorthand of "make KCFLAGS=-Werror", right?

I tend to hesitate to add another way
to do the same thing...



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/4] Kbuild: support W=[N, ]err for passing '-Werror' to the compiler

[Daniel Schwierzeck]

Hi Masahiro,

On 26.01.2018 02:09, Masahiro Yamada wrote:
> Hi Daniel,
> 
> 
> 2018-01-26 2:21 GMT+09:00 Daniel Schwierzeck <daniel.schwierzeck@gmail.com>:
>> Extend the Kbuild's W=N option with an optional 'err' value. This
>> will pass -Werror to the compiler to treat all warnings as errors.
>> This is useful to enforce a zero-warnings policy.
>>
>> The 'err' value can also be combined with the numerical values
>> like this:
>>
>>     make W=1
>>     make W=err
>>     make W=1,err
>>
>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> ---
>>
>>  scripts/Makefile.extrawarn | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
>> index 1d3a570594..d8d93b7fe1 100644
>> --- a/scripts/Makefile.extrawarn
>> +++ b/scripts/Makefile.extrawarn
>> @@ -48,9 +48,12 @@ warning-3 += -Wswitch-default
>>  warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
>>  warning-3 += $(call cc-option, -Wvla)
>>
>> +warning-err := -Werror
>> +
>>  warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>  warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>  warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>> +warning += $(warning-$(findstring err, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>
>>  ifeq ("$(strip $(warning))","")
>>          $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
>> --
>> 2.16.1
> 
> 
> I saw a similar patch before in linux-kbuild ML.
> 
> 
> Kbuild provides a way to specify user-specific options.
> See the following lines in the top-level Makefile:
> 
> 
>   # Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments
>   KBUILD_CPPFLAGS += $(KCPPFLAGS)
>   KBUILD_AFLAGS += $(KAFLAGS)
>   KBUILD_CFLAGS += $(KCFLAGS)
> 
> 
> 
> "make W=err" is a shorthand of "make KCFLAGS=-Werror", right?
> 
> I tend to hesitate to add another way
> to do the same thing...
> 

I didn't noticed that possibility, thanks for the pointer. I'll withdraw
this patch and add a short pointer in the README instead.

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180126/b4535051/attachment.sig>

[U-Boot] [PATCH 2/4] buildman: add option -E for treating compiler warnings as errors

[Simon Glass]

`On 25 January 2018 at 10:21, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
> Add a new option '-E' for treating all compiler warnings as errors.
> Eventually this will pass 'W=err' to Kbuild.

nit: I read 'eventually' as 'in a future patch'

How about 'This will cause buildman to pass ...' ?

>
> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> ---
>
>  tools/buildman/builder.py       | 5 ++++-
>  tools/buildman/builderthread.py | 2 ++
>  tools/buildman/cmdline.py       | 2 ++
>  tools/buildman/control.py       | 3 ++-
>  4 files changed, 10 insertions(+), 2 deletions(-)
>

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

[U-Boot] [PATCH 3/4] travis.yml: fix 'set +e' in build script

[Simon Glass]

On 25 January 2018 at 10:21, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
> The build script should not manipulate shell flags (especially '-e').
> A non-zero exit value can also be catched with 'cmd || ret=$?'.
>
> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> ---
>
>  .travis.yml | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH 4/4] travis.yml: run buildman with option -E

[Simon Glass]

On 25 January 2018 at 10:21, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
> This forces all compiler warnings to be treated as errors.
>
> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>
> ---
>
>  .travis.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

But just checking that this actually passes at present?