Re: [Ksummit-discuss] [media-submaintainers] [MAINTAINERS SUMMIT] Pull network and Patch Acceptance Consistency

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 19 Jun 2019, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Wed, 2019-06-19 at 17:48 +0300, Laurent Pinchart wrote:
>> Hi Mauro,
>> 
>> On Wed, Jun 19, 2019 at 11:39:02AM -0300, Mauro Carvalho Chehab
>> wrote:
>> > Em Wed, 19 Jun 2019 10:33:23 +0200 Daniel Vetter escreveu:
>> > > On Wed, Jun 19, 2019 at 9:56 AM Dan Carpenter wrote:
>> > > > On Mon, Jun 17, 2019 at 10:31:15AM -0300, Mauro Carvalho Chehab
>> > > > wrote:  
>> > > > > Also, usually, the bots don't build with W=1, as, on most
>> > > > > subsystems, this cause lots of warnings[1].
>> > > > > 
>> > > > > [1] On media, we have zero warnings with W=1.
>> > > > 
>> > > > We could ask the kbuild devs if they would consider making W=1
>> > > > a per tree option.  
>> > > 
>> > > No need to ask, just add a Kconfig which sets additional cflags
>> > > for you for your tree and your good. The usual combinatorial
>> > > testing will discover the new warnings. That's at least what we
>> > > do for i915.ko (including -Werror). Gets the job done.
>> > 
>> > While this works, having a W=1 per tree would, IMHO, work better,
>> > as, as new warnings get added to W=1, we'll get those for free.
>> > 
>> > -
>> > 
>> > I don't like the idea of having -Werror being automatically added,
>> > as this may cause problems when people try to compile with a
>> > different compiler version - or on some weird architectures.
>> 
>> It's not automatic though, if it depends on a Kconfig option that is
>> disabled by default. The built bots can enable it, while users would
>> ignore it. That being said, having it as a per-tree build bot option
>> should work as well.
>
> I really don't think well made build bots would enable this.  The
> problem with -Werror is it's single threaded on the first problem. 
> What a generic build bot wants to do is compile the entire tree and
> then diff the output to find the additional warnings for everything.  I
> could see a tree specific build bot being more interested (until the
> build fails on an unrelated subsystem).
>
>> > Specially on drivers that build with COMPILE_TEST[1], depending on
>> > the architecture they're built, false-positive warnings rise,
>> > specially on unusual architecture with has different defines for
>> > some arch-specific typedefs (signed/unsigned, different integer
>> > type, usage or not of volatile, a different address space, etc).
>> 
>> All my kernel compilation scripts use -Werror, and that does a great
>> job at catching problems. It can be a bit annoying at times when
>> someone introduces a warning, but usually a fix will already be
>> posted when I notice my build breaks. The more we use -Werror
>> globally, the faster those new warnings will be caught.
>
> I buy this for small projects, and would own up to using it in my own
> because it's a great way to force contributors not to introduce
> warnings in their patches if the build breaks.  The problem with
> something huge like linux, especially when it is fairly deeply entwined
> with compiler specifics, is twofold:
>
>    1. You're going to force us to annotate all those spurious warnings
>       that we've been ignoring because gcc should get fixed; incorrectly
>       flagged uninitialized variables being the most annoying.

In i915 we basically start off with -Wall -Wextra, and then disable the
warnings that we want ignored. Some of the disables are on a per-file
basis. We then have -Werror behind a config knob.

It's a nice way to clean up warnings in our corner of the codebase, and
ensure it stays that way. I'm sure other drivers and subsystems would
have a slightly different set of warning disabled, and necessarily the
global config needs to be a union of those sets of warnings.

>    2. Different versions of gcc produce different warnings: so now we'll
>       eventually have to target a specific gcc version and not upgrade
>       until we're ready because newer versions come with shiny new
>       warnings.

On the other hand this allows us to be more aware of the new warnings
and take advantage of them. But with -Werror it obviously only works in
a limited or local setting.

> That's not to say we should forbid bots and subsystems from doing this,
> that's what we're currently doing with the per-subdir enabling of
> -Werror using subdir-ccflags-y if you look, but we shouldn't globally
> mandate it.

Agreed.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center