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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
