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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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.
   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.

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.

James