Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1757263AbdLQCQI (ORCPT <rfc822;w@1wt.eu>);
        Sat, 16 Dec 2017 21:16:08 -0500
Received: from aserp2120.oracle.com ([141.146.126.78]:56700 "EHLO
        aserp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1756959AbdLQCQE (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 16 Dec 2017 21:16:04 -0500
Message-ID: <1513476850.31439.108.camel@oracle.com>
Subject: Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via
 a wrapper program
From: Knut Omang <knut.omang@oracle.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: linux-kernel@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Nicolas Palix <nicolas.palix@imag.fr>,
        Jonathan Corbet <corbet@lwn.net>,
        Santosh Shilimkar <santosh.shilimkar@oracle.com>,
        Matthew Wilcox <willy@infradead.org>, cocci@systeme.lip6.fr,
        rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org,
        linux-doc@vger.kernel.org, Doug Ledford <dledford@redhat.com>,
        =?ISO-8859-1?Q?Micka=EBl_Sala=FCn?= <mic@digikod.net>,
        Shuah Khan <shuah@kernel.org>, linux-kbuild@vger.kernel.org,
        Michal Marek <michal.lkml@markovi.net>,
        Julia Lawall <Julia.Lawall@lip6.fr>,
        John Haxby <john.haxby@oracle.com>,
        =?ISO-8859-1?Q?=C5smund_=D8stvold?= <asmund.ostvold@oracle.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Kees Cook <keescook@chromium.org>, netdev@vger.kernel.org,
        Gilles Muller <Gilles.Muller@lip6.fr>,
        "David S. Miller" <davem@davemloft.net>, Joe Perches <joe@perches.com>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        =?ISO-8859-1?Q?H=E5kon?= Bugge <haakon.bugge@oracle.com>,
        Andy Whitcroft <apw@canonical.com>,
        "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com>
Date: Sun, 17 Dec 2017 03:14:10 +0100
In-Reply-To: <20171216094745.5e41ac51@xeon-e3>
References: <cover.630ac8faeeda67bf7a778c158174422042942d08.1513430008.git-series.knut.omang@oracle.com>
         <20171216094745.5e41ac51@xeon-e3>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.24.6 (3.24.6-1.fc26) 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8747 signatures=668649
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.0.1-1711220000 definitions=main-1712170028
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 7876
Lines: 155

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:25 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > This patch series implements features to make it easier to run checkers on the
> > entire kernel as part of automatic and developer testing.
> > 
> > This is done by replacing the sparse specific setup for the C={1,2} variable
> > in the makefiles with setup for running scripts/runchecks, a new program that
> > can run any number of different "checkers". The behaviour of runchecks is
> > defined by simple "global" configuration in scripts/runchecks.cfg which can be
> > extended by local configuration applying to individual files, directories or
> > subtrees in the source.
> > 
> > It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
> > (patch #3).
> > 
> > The runchecks.cfg files are parsed according to this minimal language:
> > 
> >        # comments
> >        # "Global configuration in scripts/runchecks.cfg:
> >        checker <name>
> >        typedef NAME regex
> >        run <list of checkers or "all"
> > 
> >        # "local" configuration:
> >        line_len <n>
> >        except checkpatch_type [files ...]
> >        pervasive checkpatch_type1 [checkpatch_type2 ...]
> > 
> > With "make C=2" runchecks first parse the file scripts/runchecks.cfg, then
> > look for a file named 'runchecks.cfg' in the same directory as the source file.
> > If that file exists, it will be parsed and it's local configuration applied to
> > allow suppression on a per checker, per check and per file basis.
> > If a "local" configuration does not exist, either in the source directory or
> > above, make will simply silently ignore the file.
> > 
> > The idea is that the community can work to add runchecks.cfg files to
> > directories, serving both as documentation and as a way for subsystem
> > maintainers to enforce policies and individual tastes as well as TODOs and/or
> > priorities, to make it easier for newcomers to contribute in this area. By
> > ignoring directories/subtrees without such files, automation can start right
> > away as it is trivially possible to run errorless with C=2 for the entire
> > kernel.
> > 
> > For the checker maintainers this should be a benefit as well: new
> > or improved checks would generate new errors/warnings. With automatic testing
> > for the checkers, these new checks would generate error reports and cause
> > builds to fail. Adding the new check a 'pervasive' option at the top level or
> > even for specific files, marked with a "new check - please consider fixing" comment
> > or similar would make those builds pass while documenting and making the new check
> > more visible.
> > 
> > The patches includes a documentation file with some more details.
> > 
> > This patch set has evolved from an earlier shell script implementation I made
> > as only a wrapper script around checkpatch. That version have been used for a
> > number of years on a driver project I worked on where we had automatic checkin
> > regression testing. I extended that to also run checkpatch to avoid having to
> > clean up frequent unintended whitespace changes and style violations from others...
> > 
> > I have also tested this version on some directories I am familiar with.  The
> > result of that work is available in two patch sets of 10 and 11 patches, but we
> > agreed that it would be better to post them as separate patch sets later.
> > 
> > Those patch sets illustrates how I picture the "flow" from just "reining in" the
> > checkpatch detections to actually fixing classes of checkpatch issues one by
> > one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
> > any commit boundary.
> > 
> > The combined set is available here:
> > 
> >    git://github.com/knuto/linux.git  branch runchecks
> > 
> > I only include version 0 of runchecks.cfg in the two directories I
> > worked on here as the final two patches. These files both documents where
> > the issues are in those two directories, and can be used by the maintainer
> > to indicate to potential helpers what to focus on as I have tried to
> > illustrate by means of comments.
> > 
> > Changes from v1:
> > -----------------
> > Based on feedback, the implementation is completely rewritten and extended.
> > Instead of patches to checkpatch, and a sole checkpatch focus, it is now a
> > generic solution implemented in python, for any type of checkers, extendable
> > through some basic generic functionality, and for special needs by subclassing
> > the Checker class in the implementation.
> > 
> > This implementation fully supports checkpatch, sparse and
> > checkdoc == kernel-doc -none, and also has been tested with coccicheck.
> > To facilitate the same mechanism of using check types to filter what
> > checks to be suppressed, I introduced the concept of "typedefs" which allows
> > runchecks to effectively augment the check type space of the checker in cases
> > where types either are not available at all (checkdoc) or where only a few
> > can be filtered out (sparse)
> > 
> > With this in place it also became trivial to make the look and feel similar
> > for sparse and checkdoc as for checkpatch, with some optional color support
> > too, to make fixing issues in the code, the goal of this whole exercise,
> > much more pleasant IMHO :-)
> > 
> > Thanks,
> > Knut
> > 
> > Knut Omang (5):
> >   runchecks: Generalize make C={1,2} to support multiple checkers
> >   Documentation: Add doc for runchecks, a checker runner
> >   checkpatch: Improve --fix-inplace for TABSTOP
> >   rds: Add runchecks.cfg for net/rds
> >   RDMA/core: Add runchecks.cfg for drivers/infiniband/core
> > 
> >  Documentation/dev-tools/coccinelle.rst |  12 +-
> >  Documentation/dev-tools/index.rst      |   1 +-
> >  Documentation/dev-tools/runchecks.rst  | 215 ++++++++-
> >  Documentation/dev-tools/sparse.rst     |  30 +-
> >  Documentation/kbuild/kbuild.txt        |   9 +-
> >  Makefile                               |  23 +-
> >  drivers/infiniband/core/runchecks.cfg  |  83 +++-
> >  net/rds/runchecks.cfg                  |  76 +++-
> >  scripts/Makefile.build                 |   4 +-
> >  scripts/checkpatch.pl                  |   2 +-
> >  scripts/runchecks                      | 734 ++++++++++++++++++++++++++-
> >  scripts/runchecks.cfg                  |  63 ++-
> >  scripts/runchecks_help.txt             |  43 ++-
> >  13 files changed, 1274 insertions(+), 21 deletions(-)
> >  create mode 100644 Documentation/dev-tools/runchecks.rst
> >  create mode 100644 drivers/infiniband/core/runchecks.cfg
> >  create mode 100644 net/rds/runchecks.cfg
> >  create mode 100755 scripts/runchecks
> >  create mode 100644 scripts/runchecks.cfg
> >  create mode 100644 scripts/runchecks_help.txt
> > 
> > base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215
> 
> I like the ability to add more checkers and keep then in the main
> upstream tree. But adding overrides for specific subsystems goes against
> the policy that all subsystems should be treated equally.

This is a tool to enable automated testing for as many checks as possible, as soon as
possible. Like any tool, it can be misused, but that's IMHO an orthogonal problem that I
think the maintainers will be more than capable of preventing.

Think of this as a tightening screw: We eliminate errors class by class or file by file,
and in the same commit narrows in the list of exceptions. That way we can fix issues piece
by piece while avoiding a lot of regressions in already clean parts.

> There was discussion at Kernel Summit about how the different
> subsystems already have different rules. This appears to be a
> way to make that worse.

IMHO this is a tool that should help maintainers implement the policies they desire.
But the tool itself does not dictate any such.

Thanks,
Knut