Re: [PATCH v3 1/9] tests: introduce tree-wide code style checking

[Daniel P . Berrangé]

On Wed, Jul 20, 2022 at 05:25:00PM +0100, Peter Maydell wrote:
> On Thu, 7 Jul 2022 at 17:37, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The logic provide is inspired by the GNULIB 'top/maint.mk' file,
> > but has been re-implemented in a simple Python script, using a
> > YAML config file, in an attempt to make it easier to understand
> > than the make rules.
> >
> > This commit does the bare minimum introducing the basic infra:
> >
> >  - tests/style.py - the script for evaluating coding style rules
> >  - tests/style.yml - the config defining the coding style rules
> >
> > The concept behind the style checking is to perform simple regular
> > expression matches across the source file content.
> 
> > As such this style matching framework is not proposed as a solution for
> > all possible coding style rules. It is general enough that it can
> > accomplish many useful checks, and is intended to be complimentary to
> > any style checkers with semantic knowledge of the code like libclang,
> > or pylint/flake8.
> 
> So would the intention be that we try to obsolete checkpatch,
> or will we still have checkpatch because it can find some
> style issues that this framework cannot handle?
> 
> I think that on balance I'm in favour of this patchseries if
> it is part of a path where we say "we are going to drop
> checkpatch and replace it with X, Y and Z" (and we actually
> implement that path and don't just end up with another
> half-completed transition :-)). I'm much less in favour if
> it's just "we added yet another thing to the pile"...

I would certainly like to see us eventually remove
checkpatch.pl because of the various downsides it has.

The caveat is that I've not actually looked in any detail
at what things checkpatch.pl actually checks for. Without
looking my guess-timate is that we could probably replace
90% of it without much trouble. Perhaps we'll just decide
some of the checkjs in checkpatch aren't worth the burden
of maintaining its usage.

> > If a file is known to intentionally violate a style check rule
> > this can be recorded in the style.yml and will result in it being
> > ignored.  The '--ignored' flag can be used to see ignored failures.
> 
> Is it possible to have an individual "suppress this style check
> in this one place" mechanism? Dropping an entire file from the
> style check is certainly going to be useful for some situations,
> but very often I would expect there might be one place in a
> multi-thousand line C file where we want to violate a style
> rule and it would be nice not to lose the coverage on all the
> rest of the file as a result. Plus a whole-file suppression that
> lives somewhere other than the source file is going to tend to
> hang around for ages after we refactor/delete whatever bit of
> source code it was that meant we needed the suppression, whereas
> if the suppression is in the source file itself then you see it
> when you're working on that bit of code.

We could possibly come up with a way to put a magic comment
on the end of a line (eg '// style:ignore' ), and have it applied
automatically as an exclusion. A magic comment the line before
is hard though, given that we're aiming to match things linewise
for speed.

> >  meson.build            |   2 +
> >  tests/Makefile.include |   3 +-
> >  tests/meson.build      |  17 ++++
> >  tests/style.py         | 218 +++++++++++++++++++++++++++++++++++++++++
> >  tests/style.yml        |  88 +++++++++++++++++
> 
> I think this should live in scripts/, same as checkpatch.

Sure, I don't mind.

> > diff --git a/meson.build b/meson.build
> > index 65a885ea69..d8ef24bacb 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -18,6 +18,8 @@ config_host = keyval.load(meson.current_build_dir() / 
> > 'config-host.mak')
> >  enable_modules = 'CONFIG_MODULES' in config_host
> >  enable_static = 'CONFIG_STATIC' in config_host
> >
> > +in_gitrepo = run_command('test', '-d', '.git', check: false).returncode() 
> > == 0
> 
> Should we use Meson's fs.is_dir() rather than running a shell?
> (https://mesonbuild.com/Fs-module.html)

Will investigate 


> > diff --git a/tests/style.yml b/tests/style.yml
> > new file mode 100644
> > index 0000000000..b4e7c6111f
> > --- /dev/null
> > +++ b/tests/style.yml
> > @@ -0,0 +1,88 @@
> > +# Source code style checking rules
> > +#
> > +# Each top level key defines a new check, that is
> > +# exposed as a test case in the meson 'style' test
> > +# suite.
> > +#
> 
> You should say somewhere here which of the half a dozen
> possible regular expression syntaxes is used.

ok


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|