[libvirt PATCH 0/3] Misc Python-adjacent fixes
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
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@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
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@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Specifically E111 indentation is not a multiple of four This commit is better viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@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
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@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
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>
participants (2)
-
Andrea Bolognani -
Erik Skultety