12:39 p.m.
Andrea Bolognani (3):
gitignore: Ignore __pycache__ directory
tests: Fix flake8 errors in virsh-auth
syntax-check: Run flake8 on all Python scripts
.gitignore | 3 +++
build-aux/syntax-check.mk | 6 ++++--
tests/virsh-auth | 20 ++++++++++----------
3 files changed, 17 insertions(+), 12 deletions(-)
--
2.26.3
12:39 p.m.
New subject: [libvirt PATCH 1/3] gitignore: Ignore __pycache__ directory
Unfortunately running Python scripts causes this directory to
be created in the *source* directory, and there doesn't seem
to be a way to prevent that from happening.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
.gitignore | 3 +++
1 file changed, 3 insertions(+)
diff --git a/.gitignore b/.gitignore
index 6d44a50061..4695391342 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,6 +13,9 @@
*.orig
.git-module-status
+# python related ignores
+__pycache__/
+
# libvirt related ignores
/build/
/ci/scratch/
--
2.26.3
1:51 a.m.
New subject: [libvirt PATCH 1/3] gitignore: Ignore __pycache__ directory
On Fri, Mar 19, 2021 at 06:39:29PM +0100, Andrea Bolognani wrote:
Unfortunately running Python scripts causes this directory to
be created in the *source* directory, and there doesn't seem
to be a way to prevent that from happening.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
12:39 p.m.
New subject: [libvirt PATCH 2/3] tests: Fix flake8 errors in virsh-auth
Specifically
E111 indentation is not a multiple of four
This commit is better viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
tests/virsh-auth | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/tests/virsh-auth b/tests/virsh-auth
index d548694190..ce3a599107 100755
--- a/tests/virsh-auth
+++ b/tests/virsh-auth
@@ -24,11 +24,11 @@ import subprocess
builddir = os.getenv("abs_top_builddir")
if builddir is None:
- builddir = os.path.join(os.getcwd(), "..")
+ builddir = os.path.join(os.getcwd(), "..")
srcdir = os.getenv("abs_top_srcdir")
if srcdir is None:
- srcdir = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
+ srcdir = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
uri = "test://" + os.path.join(srcdir, "tests",
"virsh-auth.xml")
@@ -43,15 +43,15 @@ proc = subprocess.Popen([virsh, "-c", uri,
"uri"],
out, err = proc.communicate("astrochicken")
if proc.returncode != 0:
- print("virsh failed with code %d" % proc.returncode, file=sys.stderr)
- if out != "":
- print("stdout=%s" % out)
- if err != "":
- print("stderr=%s" % err)
- sys.exit(1)
+ print("virsh failed with code %d" % proc.returncode, file=sys.stderr)
+ if out != "":
+ print("stdout=%s" % out)
+ if err != "":
+ print("stderr=%s" % err)
+ sys.exit(1)
if uri not in out:
- print("Expected '%s' in '%s'" % (uri, out),
file=sys.stderr)
- sys.exit(1)
+ print("Expected '%s' in '%s'" % (uri, out),
file=sys.stderr)
+ sys.exit(1)
sys.exit(0)
--
2.26.3
1:53 a.m.
New subject: [libvirt PATCH 2/3] tests: Fix flake8 errors in virsh-auth
On Fri, Mar 19, 2021 at 06:39:30PM +0100, Andrea Bolognani wrote:
Specifically
E111 indentation is not a multiple of four
This commit is better viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
12:39 p.m.
New subject: [libvirt PATCH 3/3] syntax-check: Run flake8 on all Python scripts
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(a)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
1:50 a.m.
New subject: [libvirt PATCH 3/3] syntax-check: Run flake8 on all Python scripts
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(a)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
4:29 a.m.
New subject: [libvirt PATCH 3/3] syntax-check: Run flake8 on all Python scripts
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
5:25 a.m.
New subject: [libvirt PATCH 3/3] syntax-check: Run flake8 on all Python scripts
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(a)redhat.com>
1333
days inactive
1336
days old
8 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety