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

2019-09-26 Thread 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é 
> > ---
> >  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..00
> > --- 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 00..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
> > +# .
> > +
> > +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

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

2019-09-26 Thread Erik Skultety 
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é 
> ---
>  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..00
> --- 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 00..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
> +# .
> +
> +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:

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 

> +
> +if ret == 1:
> +print("Do not include a header more than once per file", file=sys.stderr)
> +
> +sys.exit(ret)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2019-09-24 Thread Daniel P . Berrangé 
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é 
---
 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..00
--- 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 00..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
+# .
+
+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
+
+if ret == 1:
+print("Do not include a header more than once per file", file=sys.stderr)
+
+sys.exit(ret)
diff --git a/cfg.mk b/cfg.mk
index 3731a2830b..bf4a8270d8 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1152,8 +1152,8 @@ endif
 
 # Don't include duplicate header in the source (either *.c or *.h)
 prohibit-duplicate-header:
-   $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[chx]$$' | xargs \
-   $(PERL) -W $(top_srcdir)/build-aux/prohibit-duplicate-header.pl
+   $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[chx]$$' | $(RUNUTF8) xargs \
+   $(PYTHON) $(top_srcdir)/build-aux/prohibit-duplicate-header.py
 
 spacing-check:
$(AM_V_GEN)$(VC_LIST) | $(GREP) '\.c$$' | xargs \
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list