* [PATCH 0/6] Test and linter fixups
@ 2025-10-01 9:51 David Gibson
2025-10-01 9:51 ` [PATCH 1/6] test: Convince make not to accidentally delete exetool David Gibson
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:51 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
Before starting to convert testcases to use tunbridge, I wanted to add
linting for the Python test scripts. While doing that I discovered a
new crop of cppcheck and clang-tidy false positives, and a Makefile
bug.
Here's a batch of fixes.
I didn't manage to get through the whole testsuite with this. I keep
getting hangs on rampstream_out, which I *think* are unrelated.
David Gibson (6):
test: Convince make not to accidentally delete exetool
test: Add linting of Python test scripts
clang-tidy: Suppress redundant expression warning
cppcheck: Suppress the suppression of a suppression
cppcheck: Suppress a buggy cppcheck warning
cppcheck: Suppress variable scope warnings in dhcpv6()
dhcpv6.c | 8 ++++++++
linux_dep.h | 2 +-
tcp.c | 5 +++++
test/Makefile | 16 +++++++++++++++-
test/build/build.py | 5 +++--
test/build/static_checkers.sh | 6 +++++-
vhost_user.c | 1 +
7 files changed, 38 insertions(+), 5 deletions(-)
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/6] test: Convince make not to accidentally delete exetool
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
@ 2025-10-01 9:51 ` David Gibson
2025-10-02 3:26 ` David Gibson
2025-10-01 9:51 ` [PATCH 2/6] test: Add linting of Python test scripts David Gibson
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:51 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
Our exeter tests make use of exetool, a tool that's part of the exeter
tree. Both Yumei and I have seen cases where exetool is missing, despite
it being from an external tree we shouldn't be touching.
I haven't pinned down the exact circumstances in which this happens, but I
think this is occurring because the way the Makefile refers to it can make
it look like an intermediate target that make may delete when interrupted.
Mark it as .PRECIOUS to prevent that.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
test/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/test/Makefile b/test/Makefile
index 49388276..69987d0b 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -81,6 +81,8 @@ pull-%: %
exeter:
git clone https://gitlab.com/dgibson/exeter.git
+# Don't delete this, make, even though it looks like an intermediate target
+.PRECIOUS: exeter/exetool/exetool
exeter/exetool/exetool: pull-exeter
mbuto:
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/6] test: Add linting of Python test scripts
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
2025-10-01 9:51 ` [PATCH 1/6] test: Convince make not to accidentally delete exetool David Gibson
@ 2025-10-01 9:51 ` David Gibson
2025-10-01 10:23 ` Stefano Brivio
2025-10-01 9:51 ` [PATCH 3/6] clang-tidy: Suppress redundant expression warning David Gibson
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:51 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
We currently have one test moved to the new exeter based framwork written
in Python. We plan to add many more, so add linting (flake8) and type
checking (mypy) of those scripts. This can be invoked manually with
"make flake8" or "make mypy" in test/, and is also added to the static
checkers test set.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
test/Makefile | 14 +++++++++++++-
test/build/build.py | 5 +++--
test/build/static_checkers.sh | 6 +++++-
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/test/Makefile b/test/Makefile
index 69987d0b..2637e0ed 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -8,6 +8,8 @@
BATS = bats -j $(shell nproc)
EXETOOL = exeter/exetool/exetool
WGET = wget -c
+FLAKE8 = flake8
+MYPY = mypy --strict
DEBIAN_IMGS = debian-8.11.0-openstack-amd64.qcow2 \
debian-10-nocloud-amd64.qcow2 \
@@ -65,11 +67,15 @@ LOCAL_ASSETS = mbuto.img mbuto.mem.img podman/bin/podman QEMU_EFI.fd \
ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
EXETER_PYPATH = exeter/py3
+EXETER_PYTHON = build/build.py
EXETER_BATS = smoke/smoke.sh.bats \
- build/build.py.bats build/static_checkers.sh.bats
+ $(EXETER_PYTHON:%=%.bats) build/static_checkers.sh.bats
BATS_FILES = $(EXETER_BATS) \
podman/test/system/505-networking-pasta.bats
+# Python test code (for linters)
+PYPKGS = $(EXETER_PYTHON)
+
CFLAGS = -Wall -Werror -Wextra -pedantic -std=c99
assets: $(ASSETS)
@@ -130,6 +136,12 @@ medium.bin:
big.bin:
dd if=/dev/urandom bs=1M count=10 of=$@
+flake8: pull-exeter
+ PYTHONPATH=$(EXETER_PYPATH) $(FLAKE8) $(PYPKGS)
+
+mypy: pull-exeter
+ PYTHONPATH=$(EXETER_PYPATH) $(MYPY) $(PYPKGS)
+
$(EXETER_BATS): %.bats: % $(EXETOOL)
PYTHONPATH=$(EXETER_PYPATH) $(EXETOOL) bats -- $< > $@
diff --git a/test/build/build.py b/test/build/build.py
index e49287c9..e3de8305 100755
--- a/test/build/build.py
+++ b/test/build/build.py
@@ -18,11 +18,12 @@ import os
from pathlib import Path
import subprocess
import tempfile
-from typing import Iterable, Iterator
+from typing import Iterator
import exeter
-def sh(cmd):
+
+def sh(cmd: str) -> None:
"""Run given command in a shell"""
subprocess.run(cmd, shell=True)
diff --git a/test/build/static_checkers.sh b/test/build/static_checkers.sh
index 42806e79..228b99ae 100755
--- a/test/build/static_checkers.sh
+++ b/test/build/static_checkers.sh
@@ -21,6 +21,10 @@ exeter_set_description cppcheck "passt sources pass cppcheck"
exeter_register clang_tidy make -C .. clang-tidy
exeter_set_description clang_tidy "passt sources pass clang-tidy"
-exeter_main "$@"
+exeter_register flake8 make flake8
+exeter_set_description flake8 "passt tests in Python pass flake8"
+exeter_register mypy make mypy
+exeter_set_description mypy "passt tests in Python pass mypy --strict"
+exeter_main "$@"
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/6] clang-tidy: Suppress redundant expression warning
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
2025-10-01 9:51 ` [PATCH 1/6] test: Convince make not to accidentally delete exetool David Gibson
2025-10-01 9:51 ` [PATCH 2/6] test: Add linting of Python test scripts David Gibson
@ 2025-10-01 9:51 ` David Gibson
2025-10-01 9:52 ` [PATCH 4/6] cppcheck: Suppress the suppression of a suppression David Gibson
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:51 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
clang-tidy 20.1.8 doesn't like (VHOST_USER_MAX_VQS / 2), because it expands
to (2 / 2). But in the context of the #define, this makes logical sense,
so suppress the warning.
I'm not sure why it isn't firing on the debug() line just below. Possibly
it only complains once per expression per function, so we only have to
suppress it once?
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
vhost_user.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/vhost_user.c b/vhost_user.c
index fa343a86..223332d5 100644
--- a/vhost_user.c
+++ b/vhost_user.c
@@ -939,6 +939,7 @@ static bool vu_get_queue_num_exec(struct vu_dev *vdev,
{
(void)vdev;
+ /* NOLINTNEXTLINE(misc-redundant-expression) */
vmsg_set_reply_u64(vmsg, VHOST_USER_MAX_VQS / 2);
debug("VHOST_USER_MAX_VQS %u", VHOST_USER_MAX_VQS / 2);
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/6] cppcheck: Suppress the suppression of a suppression
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
` (2 preceding siblings ...)
2025-10-01 9:51 ` [PATCH 3/6] clang-tidy: Suppress redundant expression warning David Gibson
@ 2025-10-01 9:52 ` David Gibson
2025-10-01 9:52 ` [PATCH 5/6] cppcheck: Suppress a buggy cppcheck warning David Gibson
2025-10-01 9:52 ` [PATCH 6/6] cppcheck: Suppress variable scope warnings in dhcpv6() David Gibson
5 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:52 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
Apparently cppcheck 2.18.3 no longer trips the funcArgNamesDifferent
warning on our (re-)definition of close_range(). So instead we get an
unmatchedSuppression warning.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
linux_dep.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/linux_dep.h b/linux_dep.h
index 1d9e1669..89e590c8 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -142,7 +142,7 @@ struct tcp_info_linux {
#endif
__attribute__ ((weak))
-/* cppcheck-suppress funcArgNamesDifferent */
+/* cppcheck-suppress [funcArgNamesDifferent,unmatchedSuppression] */
int close_range(unsigned int first, unsigned int last, int flags) {
return syscall(SYS_close_range, first, last, flags);
}
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/6] cppcheck: Suppress a buggy cppcheck warning
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
` (3 preceding siblings ...)
2025-10-01 9:52 ` [PATCH 4/6] cppcheck: Suppress the suppression of a suppression David Gibson
@ 2025-10-01 9:52 ` David Gibson
2025-10-01 9:52 ` [PATCH 6/6] cppcheck: Suppress variable scope warnings in dhcpv6() David Gibson
5 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:52 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
Another cppcheck package in Fedora, another bogus false positive. This
one seems to not realise that a variable has been initialised by
getsockopt() under a complicated set of circumstances.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
tcp.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tcp.c b/tcp.c
index 48b1ef29..7da41797 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1030,6 +1030,11 @@ int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn,
return 0;
}
+ /* This trips a cppcheck bug in some versions, including
+ * cppcheck 2.18.3.
+ * https://sourceforge.net/p/cppcheck/discussion/general/thread/fecde59085/
+ */
+ /* cppcheck-suppress [uninitvar,unmatchedSuppression] */
conn->seq_ack_to_tap = tinfo->tcpi_bytes_acked +
conn->seq_init_from_tap;
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 6/6] cppcheck: Suppress variable scope warnings in dhcpv6()
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
` (4 preceding siblings ...)
2025-10-01 9:52 ` [PATCH 5/6] cppcheck: Suppress a buggy cppcheck warning David Gibson
@ 2025-10-01 9:52 ` David Gibson
5 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-01 9:52 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
At least some cppcheck versions (2.18.3 for me) complain that the _storage
variables in dhcpv6() could be reduced in scope. That's not actually the
case, because although we don't reference the variables, we may touch
their memory via pointers after the blocks in question. There's no
reasonable way for cppcheck to determine that, though, so suppress its
warnings.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
dhcpv6.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/dhcpv6.c b/dhcpv6.c
index c1a27aba..e4df0db5 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -550,10 +550,18 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
{
const struct opt_server_id *server_id = NULL;
const struct opt_hdr *client_id = NULL;
+ /* The _storage variables can't be local to the blocks they're used in,
+ * because IOV_*_HEADER() may return pointers to them which are
+ * dereferenced afterwards. Since we don't have Rust-like lifetime
+ * tracking, cppcheck can't reasonably determine that, so we must
+ * suppress its warnings. */
+ /* cppcheck-suppress [variableScope,unmatchedSuppression] */
struct opt_server_id server_id_storage;
struct iov_tail opt, client_id_base;
const struct opt_ia_na *ia = NULL;
+ /* cppcheck-suppress [variableScope,unmatchedSuppression] */
struct opt_hdr client_id_storage;
+ /* cppcheck-suppress [variableScope,unmatchedSuppression] */
struct opt_ia_na ia_storage;
const struct in6_addr *src;
struct msg_hdr mh_storage;
--
2.51.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] test: Add linting of Python test scripts
2025-10-01 9:51 ` [PATCH 2/6] test: Add linting of Python test scripts David Gibson
@ 2025-10-01 10:23 ` Stefano Brivio
2025-10-01 10:40 ` Paul Holzinger
2025-10-02 1:09 ` David Gibson
0 siblings, 2 replies; 13+ messages in thread
From: Stefano Brivio @ 2025-10-01 10:23 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Wed, 1 Oct 2025 19:51:58 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> We currently have one test moved to the new exeter based framwork written
> in Python. We plan to add many more, so add linting (flake8) and type
> checking (mypy) of those scripts. This can be invoked manually with
> "make flake8" or "make mypy" in test/, and is also added to the static
> checkers test set.
I never used a Python linter, so I'm not sure if it's as bad as Go or
Rust linters taking the whole poetry away, as it happened for instance
in my most recent experience with 'cargo fmt':
https://github.com/AsahiLinux/muvm/compare/68094c02c19b6f5d5e3def6d29379c1244c9a5e4..9af11c334a1ce37f533c056d982f8608c8d80d27#diff-e1a95ce380b9a8a317f97cccce1cbfd3dccd343dc62169ed1340208ab304fab9L106
https://github.com/AsahiLinux/muvm/pull/111#discussion_r1863551727
(you need to click around before you get to it, no idea how to share
a proper link that opens that comment right away)
Maybe with Python it's not as annoying? I don't have a strong
preference against this, I just wanted to raise a possible downside.
In general, my thought is that coding style serves a purpose, and it's
used by humans for humans, so it's flexible, with exceptions, and
reasons behind it and behind those exceptions.
If we just pass everything through a linter, well, except for the
purposes of revision control, we don't really need a coding style?
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> test/Makefile | 14 +++++++++++++-
> test/build/build.py | 5 +++--
> test/build/static_checkers.sh | 6 +++++-
> 3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/test/Makefile b/test/Makefile
> index 69987d0b..2637e0ed 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -8,6 +8,8 @@
> BATS = bats -j $(shell nproc)
> EXETOOL = exeter/exetool/exetool
> WGET = wget -c
> +FLAKE8 = flake8
> +MYPY = mypy --strict
>
> DEBIAN_IMGS = debian-8.11.0-openstack-amd64.qcow2 \
> debian-10-nocloud-amd64.qcow2 \
> @@ -65,11 +67,15 @@ LOCAL_ASSETS = mbuto.img mbuto.mem.img podman/bin/podman QEMU_EFI.fd \
> ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
>
> EXETER_PYPATH = exeter/py3
> +EXETER_PYTHON = build/build.py
> EXETER_BATS = smoke/smoke.sh.bats \
> - build/build.py.bats build/static_checkers.sh.bats
> + $(EXETER_PYTHON:%=%.bats) build/static_checkers.sh.bats
> BATS_FILES = $(EXETER_BATS) \
> podman/test/system/505-networking-pasta.bats
>
> +# Python test code (for linters)
> +PYPKGS = $(EXETER_PYTHON)
> +
> CFLAGS = -Wall -Werror -Wextra -pedantic -std=c99
>
> assets: $(ASSETS)
> @@ -130,6 +136,12 @@ medium.bin:
> big.bin:
> dd if=/dev/urandom bs=1M count=10 of=$@
>
> +flake8: pull-exeter
> + PYTHONPATH=$(EXETER_PYPATH) $(FLAKE8) $(PYPKGS)
> +
> +mypy: pull-exeter
> + PYTHONPATH=$(EXETER_PYPATH) $(MYPY) $(PYPKGS)
> +
> $(EXETER_BATS): %.bats: % $(EXETOOL)
> PYTHONPATH=$(EXETER_PYPATH) $(EXETOOL) bats -- $< > $@
>
> diff --git a/test/build/build.py b/test/build/build.py
> index e49287c9..e3de8305 100755
> --- a/test/build/build.py
> +++ b/test/build/build.py
> @@ -18,11 +18,12 @@ import os
> from pathlib import Path
> import subprocess
> import tempfile
> -from typing import Iterable, Iterator
> +from typing import Iterator
>
> import exeter
>
> -def sh(cmd):
> +
> +def sh(cmd: str) -> None:
> """Run given command in a shell"""
> subprocess.run(cmd, shell=True)
>
> diff --git a/test/build/static_checkers.sh b/test/build/static_checkers.sh
> index 42806e79..228b99ae 100755
> --- a/test/build/static_checkers.sh
> +++ b/test/build/static_checkers.sh
> @@ -21,6 +21,10 @@ exeter_set_description cppcheck "passt sources pass cppcheck"
> exeter_register clang_tidy make -C .. clang-tidy
> exeter_set_description clang_tidy "passt sources pass clang-tidy"
>
> -exeter_main "$@"
> +exeter_register flake8 make flake8
> +exeter_set_description flake8 "passt tests in Python pass flake8"
>
> +exeter_register mypy make mypy
> +exeter_set_description mypy "passt tests in Python pass mypy --strict"
>
> +exeter_main "$@"
--
Stefano
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] test: Add linting of Python test scripts
2025-10-01 10:23 ` Stefano Brivio
@ 2025-10-01 10:40 ` Paul Holzinger
2025-10-01 10:48 ` Stefano Brivio
2025-10-02 1:09 ` David Gibson
1 sibling, 1 reply; 13+ messages in thread
From: Paul Holzinger @ 2025-10-01 10:40 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev, David Gibson
On 01/10/2025 12:23, Stefano Brivio wrote:
> On Wed, 1 Oct 2025 19:51:58 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
>> We currently have one test moved to the new exeter based framwork written
>> in Python. We plan to add many more, so add linting (flake8) and type
>> checking (mypy) of those scripts. This can be invoked manually with
>> "make flake8" or "make mypy" in test/, and is also added to the static
>> checkers test set.
> I never used a Python linter, so I'm not sure if it's as bad as Go or
> Rust linters taking the whole poetry away, as it happened for instance
> in my most recent experience with 'cargo fmt':
>
> https://github.com/AsahiLinux/muvm/compare/68094c02c19b6f5d5e3def6d29379c1244c9a5e4..9af11c334a1ce37f533c056d982f8608c8d80d27#diff-e1a95ce380b9a8a317f97cccce1cbfd3dccd343dc62169ed1340208ab304fab9L106
>
> https://github.com/AsahiLinux/muvm/pull/111#discussion_r1863551727
> (you need to click around before you get to it, no idea how to share
> a proper link that opens that comment right away)
Not really on topic for this series but since you brought up rust there
is actually a "#[rustfmt::skip]" attribute that can be used to skip the
formatting selectively in case you need it in the future.
--
Paul Holzinger
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] test: Add linting of Python test scripts
2025-10-01 10:40 ` Paul Holzinger
@ 2025-10-01 10:48 ` Stefano Brivio
2025-10-02 1:31 ` David Gibson
0 siblings, 1 reply; 13+ messages in thread
From: Stefano Brivio @ 2025-10-01 10:48 UTC (permalink / raw)
To: Paul Holzinger; +Cc: passt-dev, David Gibson
On Wed, 1 Oct 2025 12:40:09 +0200
Paul Holzinger <pholzing@redhat.com> wrote:
> On 01/10/2025 12:23, Stefano Brivio wrote:
> > On Wed, 1 Oct 2025 19:51:58 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> >> We currently have one test moved to the new exeter based framwork written
> >> in Python. We plan to add many more, so add linting (flake8) and type
> >> checking (mypy) of those scripts. This can be invoked manually with
> >> "make flake8" or "make mypy" in test/, and is also added to the static
> >> checkers test set.
> > I never used a Python linter, so I'm not sure if it's as bad as Go or
> > Rust linters taking the whole poetry away, as it happened for instance
> > in my most recent experience with 'cargo fmt':
> >
> > https://github.com/AsahiLinux/muvm/compare/68094c02c19b6f5d5e3def6d29379c1244c9a5e4..9af11c334a1ce37f533c056d982f8608c8d80d27#diff-e1a95ce380b9a8a317f97cccce1cbfd3dccd343dc62169ed1340208ab304fab9L106
> >
> > https://github.com/AsahiLinux/muvm/pull/111#discussion_r1863551727
> > (you need to click around before you get to it, no idea how to share
> > a proper link that opens that comment right away)
>
> Not really on topic for this series but since you brought up rust there
> is actually a "#[rustfmt::skip]" attribute that can be used to skip the
> formatting selectively in case you need it in the future.
Ah, thanks, I didn't know about it. Now that you mention that, I just
found out that, with flake8, it looks like one can ignore the entire
file with "# flake8: noqa" on a line of its own:
https://stackoverflow.com/a/64431741
but still, my doubts remain.
--
Stefano
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] test: Add linting of Python test scripts
2025-10-01 10:23 ` Stefano Brivio
2025-10-01 10:40 ` Paul Holzinger
@ 2025-10-02 1:09 ` David Gibson
1 sibling, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-02 1:09 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 5250 bytes --]
On Wed, Oct 01, 2025 at 12:23:51PM +0200, Stefano Brivio wrote:
> On Wed, 1 Oct 2025 19:51:58 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > We currently have one test moved to the new exeter based framwork written
> > in Python. We plan to add many more, so add linting (flake8) and type
> > checking (mypy) of those scripts. This can be invoked manually with
> > "make flake8" or "make mypy" in test/, and is also added to the static
> > checkers test set.
>
> I never used a Python linter, so I'm not sure if it's as bad as Go or
> Rust linters taking the whole poetry away, as it happened for instance
> in my most recent experience with 'cargo fmt':
>
> https://github.com/AsahiLinux/muvm/compare/68094c02c19b6f5d5e3def6d29379c1244c9a5e4..9af11c334a1ce37f533c056d982f8608c8d80d27#diff-e1a95ce380b9a8a317f97cccce1cbfd3dccd343dc62169ed1340208ab304fab9L106
>
> https://github.com/AsahiLinux/muvm/pull/111#discussion_r1863551727
> (you need to click around before you get to it, no idea how to share
> a proper link that opens that comment right away)
>
> Maybe with Python it's not as annoying? I don't have a strong
> preference against this, I just wanted to raise a possible downside.
I've been using flake8 for a while now. It _is_ pedantic about
formatting, but so far at least, I haven't felt like it conflicted
with anything I wanted to express. Finding missing or redundant
imports and the like is certainly useful, since there's no compiler to
do so. There is certainly some overlap with what mypy does.
> In general, my thought is that coding style serves a purpose, and it's
> used by humans for humans, so it's flexible, with exceptions, and
> reasons behind it and behind those exceptions.
>
> If we just pass everything through a linter, well, except for the
> purposes of revision control, we don't really need a coding style?
flake8 is partly about code formatting, but not entirely. Think of it
as sitting somewhere in between indent and cppcheck, maybe?
>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > test/Makefile | 14 +++++++++++++-
> > test/build/build.py | 5 +++--
> > test/build/static_checkers.sh | 6 +++++-
> > 3 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/test/Makefile b/test/Makefile
> > index 69987d0b..2637e0ed 100644
> > --- a/test/Makefile
> > +++ b/test/Makefile
> > @@ -8,6 +8,8 @@
> > BATS = bats -j $(shell nproc)
> > EXETOOL = exeter/exetool/exetool
> > WGET = wget -c
> > +FLAKE8 = flake8
> > +MYPY = mypy --strict
> >
> > DEBIAN_IMGS = debian-8.11.0-openstack-amd64.qcow2 \
> > debian-10-nocloud-amd64.qcow2 \
> > @@ -65,11 +67,15 @@ LOCAL_ASSETS = mbuto.img mbuto.mem.img podman/bin/podman QEMU_EFI.fd \
> > ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
> >
> > EXETER_PYPATH = exeter/py3
> > +EXETER_PYTHON = build/build.py
> > EXETER_BATS = smoke/smoke.sh.bats \
> > - build/build.py.bats build/static_checkers.sh.bats
> > + $(EXETER_PYTHON:%=%.bats) build/static_checkers.sh.bats
> > BATS_FILES = $(EXETER_BATS) \
> > podman/test/system/505-networking-pasta.bats
> >
> > +# Python test code (for linters)
> > +PYPKGS = $(EXETER_PYTHON)
> > +
> > CFLAGS = -Wall -Werror -Wextra -pedantic -std=c99
> >
> > assets: $(ASSETS)
> > @@ -130,6 +136,12 @@ medium.bin:
> > big.bin:
> > dd if=/dev/urandom bs=1M count=10 of=$@
> >
> > +flake8: pull-exeter
> > + PYTHONPATH=$(EXETER_PYPATH) $(FLAKE8) $(PYPKGS)
> > +
> > +mypy: pull-exeter
> > + PYTHONPATH=$(EXETER_PYPATH) $(MYPY) $(PYPKGS)
> > +
> > $(EXETER_BATS): %.bats: % $(EXETOOL)
> > PYTHONPATH=$(EXETER_PYPATH) $(EXETOOL) bats -- $< > $@
> >
> > diff --git a/test/build/build.py b/test/build/build.py
> > index e49287c9..e3de8305 100755
> > --- a/test/build/build.py
> > +++ b/test/build/build.py
> > @@ -18,11 +18,12 @@ import os
> > from pathlib import Path
> > import subprocess
> > import tempfile
> > -from typing import Iterable, Iterator
> > +from typing import Iterator
> >
> > import exeter
> >
> > -def sh(cmd):
> > +
> > +def sh(cmd: str) -> None:
> > """Run given command in a shell"""
> > subprocess.run(cmd, shell=True)
> >
> > diff --git a/test/build/static_checkers.sh b/test/build/static_checkers.sh
> > index 42806e79..228b99ae 100755
> > --- a/test/build/static_checkers.sh
> > +++ b/test/build/static_checkers.sh
> > @@ -21,6 +21,10 @@ exeter_set_description cppcheck "passt sources pass cppcheck"
> > exeter_register clang_tidy make -C .. clang-tidy
> > exeter_set_description clang_tidy "passt sources pass clang-tidy"
> >
> > -exeter_main "$@"
> > +exeter_register flake8 make flake8
> > +exeter_set_description flake8 "passt tests in Python pass flake8"
> >
> > +exeter_register mypy make mypy
> > +exeter_set_description mypy "passt tests in Python pass mypy --strict"
> >
> > +exeter_main "$@"
>
> --
> Stefano
>
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] test: Add linting of Python test scripts
2025-10-01 10:48 ` Stefano Brivio
@ 2025-10-02 1:31 ` David Gibson
0 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-02 1:31 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Paul Holzinger, passt-dev
[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]
On Wed, Oct 01, 2025 at 12:48:36PM +0200, Stefano Brivio wrote:
> On Wed, 1 Oct 2025 12:40:09 +0200
> Paul Holzinger <pholzing@redhat.com> wrote:
>
> > On 01/10/2025 12:23, Stefano Brivio wrote:
> > > On Wed, 1 Oct 2025 19:51:58 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > >> We currently have one test moved to the new exeter based framwork written
> > >> in Python. We plan to add many more, so add linting (flake8) and type
> > >> checking (mypy) of those scripts. This can be invoked manually with
> > >> "make flake8" or "make mypy" in test/, and is also added to the static
> > >> checkers test set.
> > > I never used a Python linter, so I'm not sure if it's as bad as Go or
> > > Rust linters taking the whole poetry away, as it happened for instance
> > > in my most recent experience with 'cargo fmt':
> > >
> > > https://github.com/AsahiLinux/muvm/compare/68094c02c19b6f5d5e3def6d29379c1244c9a5e4..9af11c334a1ce37f533c056d982f8608c8d80d27#diff-e1a95ce380b9a8a317f97cccce1cbfd3dccd343dc62169ed1340208ab304fab9L106
> > >
> > > https://github.com/AsahiLinux/muvm/pull/111#discussion_r1863551727
> > > (you need to click around before you get to it, no idea how to share
> > > a proper link that opens that comment right away)
> >
> > Not really on topic for this series but since you brought up rust there
> > is actually a "#[rustfmt::skip]" attribute that can be used to skip the
> > formatting selectively in case you need it in the future.
>
> Ah, thanks, I didn't know about it. Now that you mention that, I just
> found out that, with flake8, it looks like one can ignore the entire
> file with "# flake8: noqa" on a line of its own:
Right. More selective suppressions are also possible. I have exactly
one in exeter, for a case where I really am doing something odd for
testing reasons.
Fwiw, I gave up on using pylint, a different linting tool, because it
whinged too much. flake8 I've been fairly happy with.
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] test: Convince make not to accidentally delete exetool
2025-10-01 9:51 ` [PATCH 1/6] test: Convince make not to accidentally delete exetool David Gibson
@ 2025-10-02 3:26 ` David Gibson
0 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2025-10-02 3:26 UTC (permalink / raw)
To: passt-dev, Stefano Brivio
[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]
On Wed, Oct 01, 2025 at 07:51:57PM +1000, David Gibson wrote:
> Our exeter tests make use of exetool, a tool that's part of the exeter
> tree. Both Yumei and I have seen cases where exetool is missing, despite
> it being from an external tree we shouldn't be touching.
>
> I haven't pinned down the exact circumstances in which this happens, but I
> think this is occurring because the way the Makefile refers to it can make
> it look like an intermediate target that make may delete when interrupted.
> Mark it as .PRECIOUS to prevent that.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
This doesn't seem to be working, I'll try a different approach.
> ---
> test/Makefile | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/test/Makefile b/test/Makefile
> index 49388276..69987d0b 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -81,6 +81,8 @@ pull-%: %
> exeter:
> git clone https://gitlab.com/dgibson/exeter.git
>
> +# Don't delete this, make, even though it looks like an intermediate target
> +.PRECIOUS: exeter/exetool/exetool
> exeter/exetool/exetool: pull-exeter
>
> mbuto:
> --
> 2.51.0
>
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-10-02 4:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-10-01 9:51 [PATCH 0/6] Test and linter fixups David Gibson
2025-10-01 9:51 ` [PATCH 1/6] test: Convince make not to accidentally delete exetool David Gibson
2025-10-02 3:26 ` David Gibson
2025-10-01 9:51 ` [PATCH 2/6] test: Add linting of Python test scripts David Gibson
2025-10-01 10:23 ` Stefano Brivio
2025-10-01 10:40 ` Paul Holzinger
2025-10-01 10:48 ` Stefano Brivio
2025-10-02 1:31 ` David Gibson
2025-10-02 1:09 ` David Gibson
2025-10-01 9:51 ` [PATCH 3/6] clang-tidy: Suppress redundant expression warning David Gibson
2025-10-01 9:52 ` [PATCH 4/6] cppcheck: Suppress the suppression of a suppression David Gibson
2025-10-01 9:52 ` [PATCH 5/6] cppcheck: Suppress a buggy cppcheck warning David Gibson
2025-10-01 9:52 ` [PATCH 6/6] cppcheck: Suppress variable scope warnings in dhcpv6() David Gibson
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).