enabling W=1 by default

enabling W=1 by default

[Andy Shevchenko]

Hi!

looking at the redness of the output of `make W=1` here is the question:
isn't it a good time to enable `make W=1` by default. Yes, I understand
the impact, but at least we can do it mandatory for a _new_ code submitted to
U-Boot, right?

Ideally I would have what Linux kernel has for a few releases already, i.e.
Werror by default and getting close to make a clean builds with that and
make W=1` at least against default configurations (yeah, with U-Boot there is
probably no default, but sandbox one).

-- 
With Best Regards,
Andy Shevchenko

Re: enabling W=1 by default

[Simon Glass]

Hi Andy,

On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Hi!
>
> looking at the redness of the output of `make W=1` here is the question:
> isn't it a good time to enable `make W=1` by default. Yes, I understand
> the impact, but at least we can do it mandatory for a _new_ code submitted to
> U-Boot, right?
>
> Ideally I would have what Linux kernel has for a few releases already, i.e.
> Werror by default and getting close to make a clean builds with that and
> make W=1` at least against default configurations (yeah, with U-Boot there is
> probably no default, but sandbox one).

Warnings should be warnings...if you would like to enable it for CI
that is fine by me, but the U-Boot makefile shouldn't do it. It
defeats the purpose of having a distinction between errors and
warnings.

Regards,
Simon

Re: enabling W=1 by default

[Heinrich Schuchardt]

On 10/21/24 18:32, Simon Glass wrote:
> Hi Andy,
>
> On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> Hi!
>>
>> looking at the redness of the output of `make W=1` here is the question:
>> isn't it a good time to enable `make W=1` by default. Yes, I understand
>> the impact, but at least we can do it mandatory for a _new_ code submitted to
>> U-Boot, right?
>>
>> Ideally I would have what Linux kernel has for a few releases already, i.e.
>> Werror by default and getting close to make a clean builds with that and
>> make W=1` at least against default configurations (yeah, with U-Boot there is
>> probably no default, but sandbox one).
>
> Warnings should be warnings...if you would like to enable it for CI
> that is fine by me, but the U-Boot makefile shouldn't do it. It
> defeats the purpose of having a distinction between errors and
> warnings.
>
> Regards,
> Simon

Most contributors don't have access to our CI.

doc/develop/sending_patches.rst does not indicate that patches leading
to build warnings won't be accepted.

We should at least amend the documentation.

Best regards

Heinrich

Re: enabling W=1 by default

[Tom Rini]

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

On Mon, Oct 21, 2024 at 07:07:43PM +0200, Heinrich Schuchardt wrote:
> On 10/21/24 18:32, Simon Glass wrote:
> > Hi Andy,
> > 
> > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > 
> > > Hi!
> > > 
> > > looking at the redness of the output of `make W=1` here is the question:
> > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > U-Boot, right?
> > > 
> > > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > > Werror by default and getting close to make a clean builds with that and
> > > make W=1` at least against default configurations (yeah, with U-Boot there is
> > > probably no default, but sandbox one).
> > 
> > Warnings should be warnings...if you would like to enable it for CI
> > that is fine by me, but the U-Boot makefile shouldn't do it. It
> > defeats the purpose of having a distinction between errors and
> > warnings.
> > 
> > Regards,
> > Simon
> 
> Most contributors don't have access to our CI.
> 
> doc/develop/sending_patches.rst does not indicate that patches leading
> to build warnings won't be accepted.
> 
> We should at least amend the documentation.

Yes, please, someone.

-- 
Tom

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

Re: enabling W=1 by default

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > looking at the redness of the output of `make W=1` here is the question:
> > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > U-Boot, right?
> >
> > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > Werror by default and getting close to make a clean builds with that and
> > make W=1` at least against default configurations (yeah, with U-Boot there is
> > probably no default, but sandbox one).
> 
> Warnings should be warnings...

Yes, and ideally the code should not have warnings, right?

Otherwise how can we do better? It's quite similar to what you wrote WRT
documenting the function prototypes, the same applies to the new contribution
WRT `make W=1`.

> if you would like to enable it for CI that is fine by me,

Yes, that's the idea, but I'm not the owner of any U-Boot CIs,
hence it's a proposal.

> but the U-Boot makefile shouldn't do it. It defeats the purpose of
> having a distinction between errors and warnings.

While it's not what I wanted, I disagree on your comment. The idea is to make
rules stricter (for new code) to make it better and that's why Linus enabled
Werror by default in the Linux kernel. And personally I consider that as a good
thing to follow.

-- 
With Best Regards,
Andy Shevchenko

Re: enabling W=1 by default

[Simon Glass]

Hi Andy,

On Tue, 22 Oct 2024 at 15:23, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > looking at the redness of the output of `make W=1` here is the question:
> > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > U-Boot, right?
> > >
> > > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > > Werror by default and getting close to make a clean builds with that and
> > > make W=1` at least against default configurations (yeah, with U-Boot there is
> > > probably no default, but sandbox one).
> >
> > Warnings should be warnings...
>
> Yes, and ideally the code should not have warnings, right?
>
> Otherwise how can we do better? It's quite similar to what you wrote WRT
> documenting the function prototypes, the same applies to the new contribution
> WRT `make W=1`.
>
> > if you would like to enable it for CI that is fine by me,
>
> Yes, that's the idea, but I'm not the owner of any U-Boot CIs,
> hence it's a proposal.

You can still do a patch...but I see we already use 'buildman -E' in
CI, so perhaps it is already working?

>
> > but the U-Boot makefile shouldn't do it. It defeats the purpose of
> > having a distinction between errors and warnings.
>
> While it's not what I wanted, I disagree on your comment. The idea is to make
> rules stricter (for new code) to make it better and that's why Linus enabled
> Werror by default in the Linux kernel. And personally I consider that as a good
> thing to follow.

I'll note that coreboot enabled this and it is a right pain. Since
coreboot always produces copious amounts of pointless output, there is
then a warning hidden somewhere in the middle and the build then fails
inexplicably. At least with U-Boot we can use -s and only get messages
which require user action.

But anyway, so long as we have this enabled in CI, we are not
introducing new warnings. But another point is that we tend to get
more warnings when moving to a newer toolchain...and devicetree has
tons of warnings now.

Regards,
Simon

Re: enabling W=1 by default

[Tom Rini]

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

On Tue, Oct 22, 2024 at 08:13:40PM +0200, Simon Glass wrote:
> Hi Andy,
> 
> On Tue, 22 Oct 2024 at 15:23, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> > > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > looking at the redness of the output of `make W=1` here is the question:
> > > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > > U-Boot, right?
> > > >
> > > > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > > > Werror by default and getting close to make a clean builds with that and
> > > > make W=1` at least against default configurations (yeah, with U-Boot there is
> > > > probably no default, but sandbox one).
> > >
> > > Warnings should be warnings...
> >
> > Yes, and ideally the code should not have warnings, right?
> >
> > Otherwise how can we do better? It's quite similar to what you wrote WRT
> > documenting the function prototypes, the same applies to the new contribution
> > WRT `make W=1`.
> >
> > > if you would like to enable it for CI that is fine by me,
> >
> > Yes, that's the idea, but I'm not the owner of any U-Boot CIs,
> > hence it's a proposal.
> 
> You can still do a patch...but I see we already use 'buildman -E' in
> CI, so perhaps it is already working?

In general, yes, it is.

> > > but the U-Boot makefile shouldn't do it. It defeats the purpose of
> > > having a distinction between errors and warnings.
> >
> > While it's not what I wanted, I disagree on your comment. The idea is to make
> > rules stricter (for new code) to make it better and that's why Linus enabled
> > Werror by default in the Linux kernel. And personally I consider that as a good
> > thing to follow.
> 
> I'll note that coreboot enabled this and it is a right pain. Since
> coreboot always produces copious amounts of pointless output, there is
> then a warning hidden somewhere in the middle and the build then fails
> inexplicably. At least with U-Boot we can use -s and only get messages
> which require user action.

Personally, I find -Werror very helpful when developing code because it
means I either (a) made a mistake or (b) forgot to if-out some code I
didn't intend. This is also personal preference. But I think "make
KCFLAGS=-Werror -sj" is great, personally. And I'll re-run without the
-j if I can't see what failed right there.

> But anyway, so long as we have this enabled in CI, we are not
> introducing new warnings. But another point is that we tend to get
> more warnings when moving to a newer toolchain...and devicetree has
> tons of warnings now.

It doesn't matter at this point, but perhaps if device tree warnings
were errors there'd have been motivation these past many years to (a)
resync and (b) fix any number of them. Unfortunately it's like migration
notices and just noise to ignore.

-- 
Tom

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

Re: enabling W=1 by default

[Alexander Dahl]

Hello,

Am Tue, Oct 22, 2024 at 04:23:07PM +0300 schrieb Andy Shevchenko:
> On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > looking at the redness of the output of `make W=1` here is the question:
> > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > U-Boot, right?
> > >
> > > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > > Werror by default and getting close to make a clean builds with that and
> > > make W=1` at least against default configurations (yeah, with U-Boot there is
> > > probably no default, but sandbox one).
> > 
> > Warnings should be warnings...
> 
> Yes, and ideally the code should not have warnings, right?

+1

> Otherwise how can we do better? It's quite similar to what you wrote WRT
> documenting the function prototypes, the same applies to the new contribution
> WRT `make W=1`.
> 
> > if you would like to enable it for CI that is fine by me,
> 
> Yes, that's the idea, but I'm not the owner of any U-Boot CIs,
> hence it's a proposal.
> 
> > but the U-Boot makefile shouldn't do it. It defeats the purpose of
> > having a distinction between errors and warnings.
> 
> While it's not what I wanted, I disagree on your comment. The idea is to make
> rules stricter (for new code) to make it better and that's why Linus enabled
> Werror by default in the Linux kernel. And personally I consider that as a good
> thing to follow.

Long term experience: each time you upgrade your toolchain you get new
warnings.  Each package (u-boot, kernel, userland, does not matter
which) enabling -Werror breaks the BSP build.  What should a developer
do then?  Fix each warning in each foreign project and bring it
upstream?  Or disable -Werror?  Last thing is what is usually done.
You can see several patches in buildroot or ptxdist disabling -Werror
for this reason.

Just my 2 cents.

Greets
Alex

Re: enabling W=1 by default

[Andy Shevchenko]

On Wed, Oct 23, 2024 at 09:52:09AM +0200, Alexander Dahl wrote:
> Am Tue, Oct 22, 2024 at 04:23:07PM +0300 schrieb Andy Shevchenko:
> > On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> > > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > looking at the redness of the output of `make W=1` here is the question:
> > > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > > U-Boot, right?
> > > >
> > > > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > > > Werror by default and getting close to make a clean builds with that and
> > > > make W=1` at least against default configurations (yeah, with U-Boot there is
> > > > probably no default, but sandbox one).
> > > 
> > > Warnings should be warnings...
> > 
> > Yes, and ideally the code should not have warnings, right?
> 
> +1
> 
> > Otherwise how can we do better? It's quite similar to what you wrote WRT
> > documenting the function prototypes, the same applies to the new contribution
> > WRT `make W=1`.
> > 
> > > if you would like to enable it for CI that is fine by me,
> > 
> > Yes, that's the idea, but I'm not the owner of any U-Boot CIs,
> > hence it's a proposal.
> > 
> > > but the U-Boot makefile shouldn't do it. It defeats the purpose of
> > > having a distinction between errors and warnings.
> > 
> > While it's not what I wanted, I disagree on your comment. The idea is to make
> > rules stricter (for new code) to make it better and that's why Linus enabled
> > Werror by default in the Linux kernel. And personally I consider that as a good
> > thing to follow.
> 
> Long term experience: each time you upgrade your toolchain you get new
> warnings.  Each package (u-boot, kernel, userland, does not matter
> which) enabling -Werror breaks the BSP build.  What should a developer
> do then?  Fix each warning in each foreign project and bring it
> upstream?  Or disable -Werror?  Last thing is what is usually done.
> You can see several patches in buildroot or ptxdist disabling -Werror
> for this reason.

That's why the set of enabled/disabled warnings are spread over W=<n> and hence
hidden when known to be PITA. W=1 is kinda special in a sense that we put the
warnings that might affect code generation, size of the binary, etc. In some cases
it even might prevent security bugs.

-- 
With Best Regards,
Andy Shevchenko

Re: enabling W=1 by default

[Ilias Apalodimas]

Hi Andy

On Wed, 23 Oct 2024 at 17:52, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Oct 23, 2024 at 09:52:09AM +0200, Alexander Dahl wrote:
> > Am Tue, Oct 22, 2024 at 04:23:07PM +0300 schrieb Andy Shevchenko:
> > > On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> > > > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > looking at the redness of the output of `make W=1` here is the question:
> > > > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > > > U-Boot, right?

I think this is a good idea and perhaps we can do it per subsystem. I
cleaned up EFI a few years ago, but some new have crept in.
I've sent a bunch of no brainer patches, once I clean all of it (and
assuming it's doable), I 'll try enabling W=1 as the default for
efi_loader

thanks
/Ilias
> > > > >
> > > > > Ideally I would have what Linux kernel has for a few releases already, i.e.
> > > > > Werror by default and getting close to make a clean builds with that and
> > > > > make W=1` at least against default configurations (yeah, with U-Boot there is
> > > > > probably no default, but sandbox one).
> > > >
> > > > Warnings should be warnings...
> > >
> > > Yes, and ideally the code should not have warnings, right?
> >
> > +1
> >
> > > Otherwise how can we do better? It's quite similar to what you wrote WRT
> > > documenting the function prototypes, the same applies to the new contribution
> > > WRT `make W=1`.
> > >
> > > > if you would like to enable it for CI that is fine by me,
> > >
> > > Yes, that's the idea, but I'm not the owner of any U-Boot CIs,
> > > hence it's a proposal.
> > >
> > > > but the U-Boot makefile shouldn't do it. It defeats the purpose of
> > > > having a distinction between errors and warnings.
> > >
> > > While it's not what I wanted, I disagree on your comment. The idea is to make
> > > rules stricter (for new code) to make it better and that's why Linus enabled
> > > Werror by default in the Linux kernel. And personally I consider that as a good
> > > thing to follow.
> >
> > Long term experience: each time you upgrade your toolchain you get new
> > warnings.  Each package (u-boot, kernel, userland, does not matter
> > which) enabling -Werror breaks the BSP build.  What should a developer
> > do then?  Fix each warning in each foreign project and bring it
> > upstream?  Or disable -Werror?  Last thing is what is usually done.
> > You can see several patches in buildroot or ptxdist disabling -Werror
> > for this reason.
>
> That's why the set of enabled/disabled warnings are spread over W=<n> and hence
> hidden when known to be PITA. W=1 is kinda special in a sense that we put the
> warnings that might affect code generation, size of the binary, etc. In some cases
> it even might prevent security bugs.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: enabling W=1 by default

[Andy Shevchenko]

On Sat, Oct 26, 2024 at 11:10:40AM +0300, Ilias Apalodimas wrote:
> On Wed, 23 Oct 2024 at 17:52, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Oct 23, 2024 at 09:52:09AM +0200, Alexander Dahl wrote:
> > > Am Tue, Oct 22, 2024 at 04:23:07PM +0300 schrieb Andy Shevchenko:
> > > > On Mon, Oct 21, 2024 at 06:32:21PM +0200, Simon Glass wrote:
> > > > > On Mon, 21 Oct 2024 at 16:27, Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > >
> > > > > > looking at the redness of the output of `make W=1` here is the question:
> > > > > > isn't it a good time to enable `make W=1` by default. Yes, I understand
> > > > > > the impact, but at least we can do it mandatory for a _new_ code submitted to
> > > > > > U-Boot, right?
> 
> I think this is a good idea and perhaps we can do it per subsystem. I
> cleaned up EFI a few years ago, but some new have crept in.
> I've sent a bunch of no brainer patches, once I clean all of it (and
> assuming it's doable), I 'll try enabling W=1 as the default for
> efi_loader

Thank you!

-- 
With Best Regards,
Andy Shevchenko