Currenty we only check files that end in .py, but we have at
least a couple of scripts that don't have that suffix and we
nonetheless want to keep compliant with the code style.

Extend the sc_flake8 syntax-check rule so that any file that
contains a Python 3 shebang is fed to flake8 too.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 build-aux/syntax-check.mk | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 2bd7e2aae4..51a498a897 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -877,8 +877,10 @@ FLAKE8_IGNORE = E501,W504
 
 sc_flake8:
 	@if [ -n "$(FLAKE8)" ]; then \
-		$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs \
-			$(FLAKE8) --ignore $(FLAKE8_IGNORE) --show-source; \
+		DOT_PY=$$($(VC_LIST_EXCEPT) | $(GREP) '\.py$$'); \
+		BANG_PY=$$($(VC_LIST_EXCEPT) | xargs grep -l '^#!/usr/bin/env python3$$'); \
+		ALL_PY=$$(printf "%s\n%s" "$$DOT_PY" "$$BANG_PY" | sort -u); \
+		echo "$$ALL_PY" | xargs $(FLAKE8) --ignore $(FLAKE8_IGNORE) --show-source; \
 	else \
 		echo '$(ME): skipping test $@: flake8 not installed' 1>&2; \
 	fi
-- 
2.26.3

On Fri, Mar 19, 2021 at 06:39:31PM +0100, Andrea Bolognani wrote:
> Currenty we only check files that end in .py, but we have at
> least a couple of scripts that don't have that suffix and we
> nonetheless want to keep compliant with the code style.
> 
> Extend the sc_flake8 syntax-check rule so that any file that
> contains a Python 3 shebang is fed to flake8 too.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  build-aux/syntax-check.mk | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> index 2bd7e2aae4..51a498a897 100644
> --- a/build-aux/syntax-check.mk
> +++ b/build-aux/syntax-check.mk
> @@ -877,8 +877,10 @@ FLAKE8_IGNORE = E501,W504
>  
>  sc_flake8:
>  	@if [ -n "$(FLAKE8)" ]; then \
> -		$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs \
> -			$(FLAKE8) --ignore $(FLAKE8_IGNORE) --show-source; \
> +		DOT_PY=$$($(VC_LIST_EXCEPT) | $(GREP) '\.py$$'); \
> +		BANG_PY=$$($(VC_LIST_EXCEPT) | xargs grep -l '^#!/usr/bin/env python3$$'); \
> +		ALL_PY=$$(printf "%s\n%s" "$$DOT_PY" "$$BANG_PY" | sort -u); \

Not that I'd be against ^this, but I think it might be worth (even for
consistency reasons) to mandate that all Python scripts to use the '.py'
extension explicitly instead and rename the ones that violate this. To support
my argument, there are 34 scripts that use a suffix and 2 (one of which is the
latest CI helper) that don't.

Erik

On Mon, 2021-03-22 at 07:50 +0100, Erik Skultety wrote:
> On Fri, Mar 19, 2021 at 06:39:31PM +0100, Andrea Bolognani wrote:
> >  sc_flake8:
> >  	@if [ -n "$(FLAKE8)" ]; then \
> > -		$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs \
> > -			$(FLAKE8) --ignore $(FLAKE8_IGNORE) --show-source; \
> > +		DOT_PY=$$($(VC_LIST_EXCEPT) | $(GREP) '\.py$$'); \
> > +		BANG_PY=$$($(VC_LIST_EXCEPT) | xargs grep -l '^#!/usr/bin/env python3$$'); \
> > +		ALL_PY=$$(printf "%s\n%s" "$$DOT_PY" "$$BANG_PY" | sort -u); \
> 
> Not that I'd be against ^this, but I think it might be worth (even for
> consistency reasons) to mandate that all Python scripts to use the '.py'
> extension explicitly instead and rename the ones that violate this. To support
> my argument, there are 34 scripts that use a suffix and 2 (one of which is the
> latest CI helper) that don't.

It's a time-honored tradition to omit the suffix for scripts which
are called directly by the user, which is why you don't install
packages using dnf.py or build software using meson.py :)

If you don't limit yourself to Python specifically, you'll find several more
examples of this happening in libvirt:

  $ git grep -lE '^#!/' | sed -E 's/\.in$//g' | grep -Ev '\.[^.]+$'
  build-aux/useless-if-before-free
  build-aux/vc-list-files
  ci/helper
  examples/sh/virt-lxc-convert
  run
  tests/libvirtd-fail
  tests/libvirtd-pool
  tests/qemucapsfixreplies
  tests/qemuvhostuserdata/usr/libexec/qemu/vhost-user/test-vhost-user-gpu
  tests/virsh-auth
  tests/virsh-checkpoint
  tests/virsh-cpuset
  tests/virsh-define-dev-segfault
  tests/virsh-int-overflow
  tests/virsh-optparse
  tests/virsh-output
  tests/virsh-output-commands
  tests/virsh-read-bufsiz
  tests/virsh-read-non-seekable
  tests/virsh-schedinfo
  tests/virsh-self-test
  tests/virsh-snapshot
  tests/virsh-start
  tests/virsh-undefine
  tests/virsh-uriprecedence
  tests/virsh-vcpupin
  tests/virt-aa-helper-test
  tools/virt-pki-validate
  tools/virt-sanlock-cleanup
  tools/virt-xml-validate

Finally, if we wanted to enforce the convention that all Python
script in the repository have to be named something.py, then we'd
have to introduce a new syntax-check rule for that...

-- 
Andrea Bolognani / Red Hat / Virtualization

On Mon, Mar 22, 2021 at 10:29:32AM +0100, Andrea Bolognani wrote:
> On Mon, 2021-03-22 at 07:50 +0100, Erik Skultety wrote:
> > On Fri, Mar 19, 2021 at 06:39:31PM +0100, Andrea Bolognani wrote:
> > >  sc_flake8:
> > >  	@if [ -n "$(FLAKE8)" ]; then \
> > > -		$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs \
> > > -			$(FLAKE8) --ignore $(FLAKE8_IGNORE) --show-source; \
> > > +		DOT_PY=$$($(VC_LIST_EXCEPT) | $(GREP) '\.py$$'); \
> > > +		BANG_PY=$$($(VC_LIST_EXCEPT) | xargs grep -l '^#!/usr/bin/env python3$$'); \
> > > +		ALL_PY=$$(printf "%s\n%s" "$$DOT_PY" "$$BANG_PY" | sort -u); \
> > 
> > Not that I'd be against ^this, but I think it might be worth (even for
> > consistency reasons) to mandate that all Python scripts to use the '.py'
> > extension explicitly instead and rename the ones that violate this. To support
> > my argument, there are 34 scripts that use a suffix and 2 (one of which is the
> > latest CI helper) that don't.
> 
> It's a time-honored tradition to omit the suffix for scripts which
> are called directly by the user, which is why you don't install
> packages using dnf.py or build software using meson.py :)

Fair enough:
Reviewed-by: Erik Skultety <eskultet@redhat.com>