From mboxrd@z Thu Jan 1 00:00:00 1970
Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124])
by passt.top (Postfix) with ESMTP id EE9F45A004E
for <passt-dev@passt.top>; Wed, 07 Aug 2024 00:11:34 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
s=mimecast20190719; t=1722982293;
h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
to:to:cc:cc:mime-version:mime-version:content-type:content-type:
content-transfer-encoding:content-transfer-encoding:
in-reply-to:in-reply-to:references:references;
bh=4fg88/XiiS+FfuGowpNX7DFYW2Ua3BReyGDDSde8bi8=;
b=EBCKZcvVxih/YInapex6VUtYnK2GhP0nRRAjqocrdjAh3ir4VK0NKhJBoojJtFWPhjH76E
1H/XnCBIb4JQTwDf78V6eWzRVNrSdLF99NTaTra8OY5lmY7GYGwohW8twDTO2i4LC7Rl4G
C6swl6XV0phsnpfAe1wuHnwdX8oeVHo=
Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com
[209.85.214.198]) by relay.mimecast.com with ESMTP with STARTTLS
(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
us-mta-119-yZM9xZv1N--0A1_jvSCW5g-1; Tue, 06 Aug 2024 18:11:32 -0400
X-MC-Unique: yZM9xZv1N--0A1_jvSCW5g-1
Received: by mail-pl1-f198.google.com with SMTP id d9443c01a7336-1fca868b53cso11550395ad.1
for <passt-dev@passt.top>; Tue, 06 Aug 2024 15:11:31 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1722982291; x=1723587091;
h=content-transfer-encoding:mime-version:organization:references
:in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state
:from:to:cc:subject:date:message-id:reply-to;
bh=4fg88/XiiS+FfuGowpNX7DFYW2Ua3BReyGDDSde8bi8=;
b=aT+S8zTVdjBAHHVvQal2l/W39denuPO84UxNn+gTPaLVxR8Kdja/5d+WDhHAtMiMZ6
taK0n2B2NlVIQbs3aE8vs6OVIg3nOJKakSQn/5fdAkMzLbb7mzf1SwjHzyDQrzTp8IwP
pd1LK9JB8QEG+vKZZm2YFVERUOhdUCH2xQS1tTwAvzJtmy3RIvJKQ8sdxY/yvprt8H1C
zg0rcYR/eczW5L1n0raWFjM+QnBamMRiUsAQWt9SacL1Qe1p2nxkzTv+qAH5zg07dLy2
Euz9/+nEIH1lCNqAXOS4eR3WexHx0W0y3sSlgHJb6agFwEemUjIuUAVdom1MRAGB8e3Q
5oig==
X-Gm-Message-State: AOJu0Yx8G1GFJYgYCifozVqFmbCnzzwgjH8+vwPNzH1idEKPNzdseO1q
SEpwZtvSesLDAQEsFbTwQmGiNDESNUwi//er38zge1LKIrQW0s42aCWBK4d4KKl4NSjzoGDfDZd
pn6c3Ugyb0bJRlhiapLI+baU1uoP+Ttn/5RIo2ePwXteKACjSPA==
X-Received: by 2002:a17:902:ec83:b0:1fd:9590:6550 with SMTP id d9443c01a7336-1ff574dfe01mr176802785ad.64.1722982290844;
Tue, 06 Aug 2024 15:11:30 -0700 (PDT)
X-Google-Smtp-Source: AGHT+IGWl6DJ2s2BvjitLBVWo1Ig5GXokKhHwXXzLU7d7aP5TVQi7J0I/y3kDW+c1HAI1L5iZavlNw==
X-Received: by 2002:a17:902:ec83:b0:1fd:9590:6550 with SMTP id d9443c01a7336-1ff574dfe01mr176802435ad.64.1722982290293;
Tue, 06 Aug 2024 15:11:30 -0700 (PDT)
Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1])
by smtp.gmail.com with ESMTPSA id d9443c01a7336-1ff58f6d80csm92458965ad.119.2024.08.06.15.11.29
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Tue, 06 Aug 2024 15:11:29 -0700 (PDT)
Date: Wed, 7 Aug 2024 00:11:26 +0200
From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 06/22] test: Add exeter+Avocado based build tests
Message-ID: <20240807001126.5e9a92d3@elisabeth>
In-Reply-To: <20240805123701.1720730-7-david@gibson.dropbear.id.au>
References: <20240805123701.1720730-1-david@gibson.dropbear.id.au>
<20240805123701.1720730-7-david@gibson.dropbear.id.au>
Organization: Red Hat
X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu)
MIME-Version: 1.0
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Message-ID-Hash: GADH55EKMJKCTXFMOE2XIDD2WBKZPW4W
X-Message-ID-Hash: GADH55EKMJKCTXFMOE2XIDD2WBKZPW4W
X-MailFrom: sbrivio@redhat.com
X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header
CC: passt-dev@passt.top, Cleber Rosa <crosa@redhat.com>
X-Mailman-Version: 3.3.8
Precedence: list
List-Id: Development discussion and patches for passt <passt-dev.passt.top>
Archived-At: <https://archives.passt.top/passt-dev/20240807001126.5e9a92d3@elisabeth/>
Archived-At: <https://passt.top/hyperkitty/list/passt-dev@passt.top/message/GADH55EKMJKCTXFMOE2XIDD2WBKZPW4W/>
List-Archive: <https://archives.passt.top/passt-dev/>
List-Archive: <https://passt.top/hyperkitty/list/passt-dev@passt.top/>
List-Help: <mailto:passt-dev-request@passt.top?subject=help>
List-Owner: <mailto:passt-dev-owner@passt.top>
List-Post: <mailto:passt-dev@passt.top>
List-Subscribe: <mailto:passt-dev-join@passt.top>
List-Unsubscribe: <mailto:passt-dev-leave@passt.top>
On Mon, 5 Aug 2024 22:36:45 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> Add a new test script to run the equivalent of the tests in build/all
> using exeter and Avocado. This new version of the tests is more robust
> than the original, since it makes a temporary copy of the source tree so
> will not be affected by concurrent manual builds.
I think this is much more readable than the previous Python attempt.
On the other hand, I guess it's not an ideal candidate for a fair
comparison because this is exactly the kind of stuff where shell
scripting shines: it's a simple test that needs a few basic shell
commands.
On that subject, the shell test is about half the lines of code (just
skipping headers, it's 48 lines instead of 90... and yes, this version
now uses a copy of the source code, but that would be two lines).
In terms of time overhead, dropping delays to make the display capture
nice (a feature that we would anyway lose with exeter plus Avocado, if
I understood correctly):
$ time (make clean; make passt; make clean; make pasta; make clean; make qrap; make clean; make; d=$(mktemp -d); prefix=$d make install; prefix=$d make uninstall; )
[...]
real 0m17.449s
user 0m15.616s
sys 0m2.136s
compared to:
$ time ./run
[...]
real 0m18.217s
user 0m0.010s
sys 0m0.001s
...which I would call essentially no overhead. I didn't try out this
version yet, I suspect it would be somewhere in between.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> test/Makefile | 19 +++++---
> test/build/.gitignore | 1 +
> test/build/build.py | 105 ++++++++++++++++++++++++++++++++++++++++++
> test/run_avocado | 2 +-
> 4 files changed, 120 insertions(+), 7 deletions(-)
> create mode 100644 test/build/build.py
>
> diff --git a/test/Makefile b/test/Makefile
> index dae25312..d24fce14 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -64,15 +64,19 @@ LOCAL_ASSETS = mbuto.img mbuto.mem.img podman/bin/podman QEMU_EFI.fd \
> ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
>
> EXETER_SH = build/static_checkers.sh
> -EXETER_JOBS = $(EXETER_SH:%.sh=%.json)
> +EXETER_PY = build/build.py
> +EXETER_JOBS = $(EXETER_SH:%.sh=%.json) $(EXETER_PY:%.py=%.json)
>
> AVOCADO_JOBS = $(EXETER_JOBS) avocado/static_checkers.json
>
> -SYSTEM_PYTHON = python3
> +PYTHON = python3
> VENV = venv
> -PYTHON = $(VENV)/bin/python3
> PIP = $(VENV)/bin/pip3
> -RUN_AVOCADO = cd .. && test/$(PYTHON) test/run_avocado
> +PYPATH = exeter/py3
> +SPACE = $(subst ,, )
> +PYPATH_TEST = $(subst $(SPACE),:,$(PYPATH))
> +PYPATH_BASE = $(subst $(SPACE),:,$(PYPATH:%=test/%))
> +RUN_AVOCADO = cd .. && PYTHONPATH=$(PYPATH_BASE) test/$(VENV)/bin/python3 test/run_avocado
At least intuitively, I would have no clue what this all does. But it
doesn't matter so much, I could try to find out the day that something
doesn't work.
>
> CFLAGS = -Wall -Werror -Wextra -pedantic -std=c99
>
> @@ -131,13 +135,16 @@ big.bin:
> dd if=/dev/urandom bs=1M count=10 of=$@
>
> .PHONY: venv
> -venv:
> - $(SYSTEM_PYTHON) -m venv $(VENV)
> +venv: pull-exeter
> + $(PYTHON) -m venv $(VENV)
> $(PIP) install avocado-framework
>
> %.json: %.sh pull-exeter
> cd ..; sh test/$< --avocado > test/$@
>
> +%.json: %.py pull-exeter
> + cd ..; PYTHONPATH=$(PYPATH_BASE) $(PYTHON) test/$< --avocado > test/$@
> +
Same here.
> .PHONY: avocado
> avocado: venv $(AVOCADO_JOBS)
> $(RUN_AVOCADO) $(AVOCADO_JOBS)
> diff --git a/test/build/.gitignore b/test/build/.gitignore
> index a6c57f5f..4ef40dd0 100644
> --- a/test/build/.gitignore
> +++ b/test/build/.gitignore
> @@ -1 +1,2 @@
> *.json
> +build.exeter
> diff --git a/test/build/build.py b/test/build/build.py
> new file mode 100644
> index 00000000..79668672
> --- /dev/null
> +++ b/test/build/build.py
> @@ -0,0 +1,105 @@
> +#! /usr/bin/env python3
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# PASST - Plug A Simple Socket Transport
> +# for qemu/UNIX domain socket mode
> +#
> +# PASTA - Pack A Subtle Tap Abstraction
> +# for network namespace/tap device mode
> +#
> +# test/build/build.sh - Test build and install targets
> +#
> +# Copyright Red Hat
> +# Author: David Gibson <david@gibson.dropbear.id.au>
> +
> +import contextlib
> +import os.path
> +import shutil
> +import subprocess
> +import tempfile
> +
> +import exeter
> +
> +
> +def host_run(*cmd, **kwargs):
> + return subprocess.run(cmd, check=True, encoding='UTF-8', **kwargs)
> +
> +
> +def host_out(*cmd, **kwargs):
> + return host_run(*cmd, capture_output=True, **kwargs).stdout
A vague idea only, so far, but I guess it's fine to have some amount of
boilerplate.
> +
> +
> +@contextlib.contextmanager
> +def clone_source_tree():
> + with tempfile.TemporaryDirectory(ignore_cleanup_errors=False) as tmpdir:
> + # Make a temporary copy of the sources
> + srcfiles = host_out('git', 'ls-files').splitlines()
> + for src in srcfiles:
> + dst = os.path.join(tmpdir, src)
> + os.makedirs(os.path.dirname(dst), exist_ok=True)
> + shutil.copy(src, dst)
> + os.chdir(tmpdir)
> + yield tmpdir
This all makes sense.
Of course it would be more readable in shell script (including the trap
to remove the temporary directory on failure/interrupt), but I think
it's as clear as it can get in any other language.
> +
> +
> +def build_target(target, outputs):
> + with clone_source_tree():
> + for o in outputs:
> + assert not os.path.exists(o)
> + host_run('make', f'{target}', 'CFLAGS="-Werror"')
Compared to:
host CFLAGS="-Werror" make
I would say it's not great, but again, it makes sense, and it's as good
as it gets, I suppose.
> + for o in outputs:
> + assert os.path.exists(o)
> + host_run('make', 'clean')
> + for o in outputs:
> + assert not os.path.exists(o)
Same here,
check [ -f passt ]
check [ -h pasta ]
check [ -f qrap ]
> +
> +
> +@exeter.test
> +def test_make_passt():
> + build_target('passt', ['passt'])
> +
> +
> +@exeter.test
> +def test_make_pasta():
> + build_target('pasta', ['pasta'])
> +
> +
> +@exeter.test
> +def test_make_qrap():
> + build_target('qrap', ['qrap'])
> +
> +
> +@exeter.test
> +def test_make_all():
> + build_target('all', ['passt', 'pasta', 'qrap'])
These all make sense and look relatively readable (while not as...
writable as shell commands "everybody" is familiar with).
> +
> +@exeter.test
> +def test_make_install_uninstall():
> + with clone_source_tree():
> + with tempfile.TemporaryDirectory(ignore_cleanup_errors=False) \
> + as prefix:
> + bindir = os.path.join(prefix, 'bin')
> + mandir = os.path.join(prefix, 'share', 'man')
> + exes = ['passt', 'pasta', 'qrap']
> +
> + # Install
> + host_run('make', 'install', 'CFLAGS="-Werror"', f'prefix={prefix}')
> +
> + for t in exes:
> + assert os.path.isfile(os.path.join(bindir, t))
> + host_run('man', '-M', f'{mandir}', '-W', 'passt')
> +
> + # Uninstall
> + host_run('make', 'uninstall', f'prefix={prefix}')
> +
> + for t in exes:
> + assert not os.path.exists(os.path.join(bindir, t))
> + cmd = ['man', '-M', f'{mandir}', '-W', 'passt']
Same, up to here: it's much more readable and obvious to write in shell
script, but I don't find it impossible to grasp in Python, either.
> + exeter.assert_raises(subprocess.CalledProcessError,
> + host_run, *cmd)
This, I have no idea why. Why is it only in this loop? How does it
affect the control flow?
> +
> +
> +if __name__ == '__main__':
> + exeter.main()
> diff --git a/test/run_avocado b/test/run_avocado
> index d518b9ec..26a226ce 100755
> --- a/test/run_avocado
> +++ b/test/run_avocado
> @@ -41,7 +41,7 @@ def main():
> "resolver.references": references,
> "runner.identifier_format": "{args}",
> }
> - suite = TestSuite.from_config(config, name="static_checkers")
> + suite = TestSuite.from_config(config, name="all")
> with Job(config, [suite]) as j:
> return j.run()
>
Patch 22/22 will take me a bit longer (I'm just looking at these two
for the moment, as you suggested).
--
Stefano