[libvirt] [PATCH v3 04/22] build-aux: rewrite duplicate header checker in Python

[Daniel P. Berrangé]

On Thu, Sep 26, 2019 at 02:46:50PM +0200, Erik Skultety wrote:
> On Tue, Sep 24, 2019 at 03:58:45PM +0100, Daniel P. Berrangé wrote:
> > As part of an goal to eliminate Perl from libvirt build tools,
> > rewrite the prohibit-duplicate-header.pl tool in Python.
> >
> > This was a straight conversion, manually going line-by-line to
> > change the syntax from Perl to Python. Thus the overall structure
> > of the file and approach is the same.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >  Makefile.am                            |  2 +-
> >  build-aux/prohibit-duplicate-header.pl | 26 ------------
> >  build-aux/prohibit-duplicate-header.py | 57 ++++++++++++++++++++++++++
> >  cfg.mk                                 |  4 +-
> >  4 files changed, 60 insertions(+), 29 deletions(-)
> >  delete mode 100644 build-aux/prohibit-duplicate-header.pl
> >  create mode 100644 build-aux/prohibit-duplicate-header.py
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 8f688d40d0..a7d8da7146 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -47,7 +47,7 @@ EXTRA_DIST = \
> >    build-aux/header-ifdef.pl \
> >    build-aux/minimize-po.py \
> >    build-aux/mock-noinline.pl \
> > -  build-aux/prohibit-duplicate-header.pl \
> > +  build-aux/prohibit-duplicate-header.py \
> >    build-aux/useless-if-before-free \
> >    build-aux/vc-list-files \
> >    ci/Makefile \
> > diff --git a/build-aux/prohibit-duplicate-header.pl b/build-aux/prohibit-duplicate-header.pl
> > deleted file mode 100644
> > index 4a2ea65665..0000000000
> > --- a/build-aux/prohibit-duplicate-header.pl
> > +++ /dev/null
> > @@ -1,26 +0,0 @@
> > -#!/usr/bin/env perl
> > -
> > -use strict;
> > -
> > -my $file = " ";
> > -my $ret = 0;
> > -my %includes = ( );
> > -my $lineno = 0;
> > -
> > -while (<>) {
> > -    if (not $file eq $ARGV) {
> > -        %includes = ( );
> > -        $file = $ARGV;
> > -        $lineno = 0;
> > -    }
> > -    $lineno++;
> > -    if (/^# *include *[<"]([^>"]*\.h)[">]/) {
> > -        $includes{$1}++;
> > -        if ($includes{$1} == 2) {
> > -            $ret = 1;
> > -            print STDERR "$ARGV:$lineno: $_";
> > -            print STDERR "Do not include a header more than once per file\n";
> > -        }
> > -    }
> > -}
> > -exit $ret;
> > diff --git a/build-aux/prohibit-duplicate-header.py b/build-aux/prohibit-duplicate-header.py
> > new file mode 100644
> > index 0000000000..5ed3f82ba7
> > --- /dev/null
> > +++ b/build-aux/prohibit-duplicate-header.py
> > @@ -0,0 +1,57 @@
> > +#!/usr/bin/env python
> > +#
> > +# Copyright (C) 2016-2019 Red Hat, Inc.
> > +#
> > +# This library is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU Lesser General Public
> > +# License as published by the Free Software Foundation; either
> > +# version 2.1 of the License, or (at your option) any later version.
> > +#
> > +# This library is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +# Lesser General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU Lesser General Public
> > +# License along with this library.  If not, see
> > +# <http://www.gnu.org/licenses/>.
> > +
> > +from __future__ import print_function
> > +
> > +import re
> > +import sys
> > +
> > +
> > +def check_file(filename):
> > +    includes = {}
> > +    lineno = 0
> > +    errs = False
> > +    with open(filename, "r") as fh:
> > +        headerprog = re.compile(r'''^# *include *[<"]([^>"]*\.h)[">]\s*$''')
> > +        for line in fh:
> > +            lineno = lineno + 1
> > +
> > +            headermatch = headerprog.match(line)
> > +            if headermatch is not None:
> > +                inc = headermatch.group(1)
> > +
> > +                if inc in includes:
> > +                    print("%s:%d: %s" % (filename, lineno, inc),
> > +                          file=sys.stderr)
> > +                    errs = True
> > +                else:
> > +                    includes[inc] = True
> > +
> > +    return errs
> > +
> > +
> > +ret = 0
> > +
> > +for filename in sys.argv[1:]:
> > +    if check_file(filename):
> > +        ret = 1
> 
> so, how about instead of using @ret at all we make @errs global and then
> simply do:

I'm biased against referencing global variables from within methods,
because I feel it leads to harder to understand code when there's a
mix of local & global vars used. It especially makes later refactoring
of code more error prone as code being refactored interacts with global
state in unpredictable ways.

Now  'errs' is quite a simple usage pattern, but I'd still prefer to
avoid mixing in globals.

> 
> if err:
>     print("Do not include a header...")
> 
> It's just an alternative, maybe a tiny bit more intuitive.
> 
> Other than that, same question about the re.compile as in the previous patch,
> but those are just nitpicks:
> 
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> 

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