[PATCH v2 0/3] mtest2make: clean up and make dependencies more precise

[PATCH v2 0/3] mtest2make: clean up and make dependencies more precise

[Paolo Bonzini]

You probably have never thought much about scripts/mtest2make.py, and in
fact it has seen only a hendful of commits in the last few years.
The idea is pretty simple: gather the list of testsuites and their
dependencies, and turn a "make check-*" invocation into calling "ninja"
first and "meson test" second.  On top of that, it magically turns
SPEED=thorough into invoking up to three suites named XYZ, XYZ-slow
and XYZ-thorough.

But even this incospicuous script can harbor a bug, or rather an "I
didn't really think too much about it" situation.  Dependencies are added
to suite XYZ independent of the speed that is used in the meson.build
file: add a dependency to a func-thorough test and "make check-func"
will build it at any requested speed.

Fixing this is a one line change with some cleanup performed on the
front, and that cleanup is patch 1.  This (at least to me) also makes
the generated Makefile.mtest file a little bit easier to read, which is
nice to have.

Paolo

v1->v2: support "make check-SUITE" where SUITE includes a speed

Paolo Bonzini (3):
  mtest2make: cleanup mtest-suites variables
  mtest2make: add dependencies to the "speed-qualified" suite
  mtest2make: do not repeat the same speed over and over

 Makefile              |  4 +++-
 scripts/mtest2make.py | 43 ++++++++++++++++++-------------------------
 2 files changed, 21 insertions(+), 26 deletions(-)

-- 
2.51.1

[PATCH v2 1/3] mtest2make: cleanup mtest-suites variables

[Paolo Bonzini]

Remove the "--suite" argument from the .*.mtest-suites variables, and
add it only when actually computing the arguments to "meson test".
This makes it possible to set ninja-cmd-goals from the set of suites,
instead of doing it via many different .ninja-goals.* variables.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile              |  4 +++-
 scripts/mtest2make.py | 40 +++++++++++++++++-----------------------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/Makefile b/Makefile
index 74c2da20372..9fb55dcf330 100644
--- a/Makefile
+++ b/Makefile
@@ -96,6 +96,8 @@ meson.stamp: config-host.mak
 
 # 3. ensure meson-generated build files are up-to-date
 
+ninja-cmd-goals =
+
 ifneq ($(NINJA),)
 Makefile.ninja: build.ninja
 	$(quiet-@){ \
@@ -150,7 +152,7 @@ NINJAFLAGS = \
           $(or $(filter -l% -j%, $(MAKEFLAGS)), \
                $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1))) \
         -d keepdepfile
-ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
+ninja-cmd-goals += $(or $(MAKECMDGOALS), all)
 ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g))
 
 makefile-targets := build.ninja ctags TAGS cscope dist clean
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 2ef375fc6fb..891037e1d66 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -14,18 +14,18 @@
 class Suite(object):
     def __init__(self):
         self.deps = set()
-        self.speeds = ['quick']
+        self.speeds = []
 
     def names(self, base):
-        return [base if speed == 'quick' else f'{base}-{speed}' for speed in self.speeds]
+        return [f'{base}-{speed}' for speed in self.speeds]
 
 
-print('''
+print(r'''
 SPEED = quick
 
-.speed.quick = $(foreach s,$(sort $(filter-out %-slow %-thorough, $1)), --suite $s)
-.speed.slow = $(foreach s,$(sort $(filter-out %-thorough, $1)), --suite $s)
-.speed.thorough = $(foreach s,$(sort $1), --suite $s)
+.speed.quick = $(sort $(filter-out %-slow %-thorough, $1))
+.speed.slow = $(sort $(filter-out %-thorough, $1))
+.speed.thorough = $(sort $1)
 
 TIMEOUT_MULTIPLIER ?= 1
 .mtestargs = --no-rebuild -t $(TIMEOUT_MULTIPLIER)
@@ -34,8 +34,10 @@ def names(self, base):
 endif
 .mtestargs += $(subst -j,--num-processes , $(filter-out -j, $(lastword -j1 $(filter -j%, $(MAKEFLAGS)))))
 
-.check.mtestargs = $(MTESTARGS) $(.mtestargs) $(if $(V),--verbose,--print-errorlogs)
-.bench.mtestargs = $(MTESTARGS) $(.mtestargs) --benchmark --verbose''')
+.check.mtestargs = $(MTESTARGS) $(.mtestargs) $(if $(V),--verbose,--print-errorlogs) \
+    $(foreach s, $(sort $(.check.mtest-suites)), --suite $s)
+.bench.mtestargs = $(MTESTARGS) $(.mtestargs) --benchmark --verbose \
+    $(foreach s, $(sort $(.bench.mtest-suites)), --suite $s)''')
 
 introspect = json.load(sys.stdin)
 
@@ -72,29 +74,21 @@ def emit_prolog(suites, prefix):
     print(f'all-{prefix}-targets = {all_targets}')
     print(f'all-{prefix}-xml = {all_xml}')
     print(f'.PHONY: {prefix} do-meson-{prefix} {prefix}-report.junit.xml $(all-{prefix}-targets) $(all-{prefix}-xml)')
-    print(f'ifeq ($(filter {prefix}, $(MAKECMDGOALS)),)')
-    print(f'.{prefix}.mtestargs += $(call .speed.$(SPEED), $(.{prefix}.mtest-suites))')
-    print(f'endif')
+    print(f'ninja-cmd-goals += $(foreach s, $(.{prefix}.mtest-suites), $(.{prefix}-$s.deps))')
     print(f'{prefix}-build: run-ninja')
     print(f'{prefix} $(all-{prefix}-targets): do-meson-{prefix}')
     print(f'do-meson-{prefix}: run-ninja; $(if $(MAKE.n),,+)$(MESON) test $(.{prefix}.mtestargs)')
     print(f'{prefix}-report.junit.xml $(all-{prefix}-xml): {prefix}-report%.junit.xml: run-ninja')
     print(f'\t$(MAKE) {prefix}$* MTESTARGS="$(MTESTARGS) --logbase {prefix}-report$*" && ln -f meson-logs/$@ .')
 
-def emit_suite_deps(name, suite, prefix):
-    deps = ' '.join(suite.deps)
-    targets = [f'{prefix}-{name}', f'{prefix}-report-{name}.junit.xml', f'{prefix}', f'{prefix}-report.junit.xml',
-               f'{prefix}-build']
-    print()
-    print(f'.{prefix}-{name}.deps = {deps}')
-    for t in targets:
-        print(f'.ninja-goals.{t} += $(.{prefix}-{name}.deps)')
-
 def emit_suite(name, suite, prefix):
-    emit_suite_deps(name, suite, prefix)
-    targets = f'{prefix}-{name} {prefix}-report-{name}.junit.xml {prefix} {prefix}-report.junit.xml'
+    deps = ' '.join(suite.deps)
+    targets = f'{prefix}-{name} {prefix}-report-{name}.junit.xml {prefix} {prefix}-report.junit.xml {prefix}-build'
+    names = ' '.join(suite.names(name))
+    print(f'.{prefix}-{name}.deps = {deps}')
     print(f'ifneq ($(filter {targets}, $(MAKECMDGOALS)),)')
-    print(f'.{prefix}.mtest-suites += ' + ' '.join(suite.names(name)))
+    # for the "base" suite possibly add FOO-slow and FOO-thorough
+    print(f".{prefix}.mtest-suites += {name} $(call .speed.$(SPEED), {names})")
     print(f'endif')
 
 targets = {t['id']: [os.path.relpath(f) for f in t['filename']]
-- 
2.51.1

Re: [PATCH v2 1/3] mtest2make: cleanup mtest-suites variables

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Remove the "--suite" argument from the .*.mtest-suites variables, and
> add it only when actually computing the arguments to "meson test".
> This makes it possible to set ninja-cmd-goals from the set of suites,
> instead of doing it via many different .ninja-goals.* variables.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 2/3] mtest2make: add dependencies to the "speed-qualified" suite

[Paolo Bonzini]

Thorough tests may have more dependencies than faster ones.
Dependencies are now looked up based on the suites being
executed, not on the suites passed as goals to the makefile.
Therefore, it is possible to limit dependencies to the
speeds that need them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/mtest2make.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 891037e1d66..692e4867ec7 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -59,13 +59,13 @@ def process_tests(test, targets, suites):
             s = s.split(':')[1]
             if s == 'slow' or s == 'thorough':
                 continue
+        suites[s].deps.update(deps)
         if s.endswith('-slow'):
             s = s[:-5]
             suites[s].speeds.append('slow')
         if s.endswith('-thorough'):
             s = s[:-9]
             suites[s].speeds.append('thorough')
-        suites[s].deps.update(deps)
 
 def emit_prolog(suites, prefix):
     all_targets = ' '.join((f'{prefix}-{k}' for k in suites.keys()))
-- 
2.51.1

Re: [PATCH v2 2/3] mtest2make: add dependencies to the "speed-qualified" suite

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Thorough tests may have more dependencies than faster ones.
> Dependencies are now looked up based on the suites being
> executed, not on the suites passed as goals to the makefile.
> Therefore, it is possible to limit dependencies to the
> speeds that need them.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  scripts/mtest2make.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
> index 891037e1d66..692e4867ec7 100644
> --- a/scripts/mtest2make.py
> +++ b/scripts/mtest2make.py
> @@ -59,13 +59,13 @@ def process_tests(test, targets, suites):
>              s = s.split(':')[1]
>              if s == 'slow' or s == 'thorough':
>                  continue
> +        suites[s].deps.update(deps)
>          if s.endswith('-slow'):
>              s = s[:-5]
>              suites[s].speeds.append('slow')
>          if s.endswith('-thorough'):
>              s = s[:-9]
>              suites[s].speeds.append('thorough')
> -        suites[s].deps.update(deps)
>  
>  def emit_prolog(suites, prefix):
>      all_targets = ' '.join((f'{prefix}-{k}' for k in suites.keys()))

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 3/3] mtest2make: do not repeat the same speed over and over

[Paolo Bonzini]

There are just two of them (slow and thorough; quick is simply the
default).  Avoid repeating them for as many times as there are tests.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/mtest2make.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 692e4867ec7..19c11ed9352 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -14,7 +14,7 @@
 class Suite(object):
     def __init__(self):
         self.deps = set()
-        self.speeds = []
+        self.speeds = set()
 
     def names(self, base):
         return [f'{base}-{speed}' for speed in self.speeds]
@@ -62,10 +62,10 @@ def process_tests(test, targets, suites):
         suites[s].deps.update(deps)
         if s.endswith('-slow'):
             s = s[:-5]
-            suites[s].speeds.append('slow')
+            suites[s].speeds.add('slow')
         if s.endswith('-thorough'):
             s = s[:-9]
-            suites[s].speeds.append('thorough')
+            suites[s].speeds.add('thorough')
 
 def emit_prolog(suites, prefix):
     all_targets = ' '.join((f'{prefix}-{k}' for k in suites.keys()))
-- 
2.51.1

Re: [PATCH v2 3/3] mtest2make: do not repeat the same speed over and over

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> There are just two of them (slow and thorough; quick is simply the
> default).  Avoid repeating them for as many times as there are tests.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/3] mtest2make: clean up and make dependencies more precise

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> You probably have never thought much about scripts/mtest2make.py, and in
> fact it has seen only a hendful of commits in the last few years.
> The idea is pretty simple: gather the list of testsuites and their
> dependencies, and turn a "make check-*" invocation into calling "ninja"
> first and "meson test" second.  On top of that, it magically turns
> SPEED=thorough into invoking up to three suites named XYZ, XYZ-slow
> and XYZ-thorough.
>
> But even this incospicuous script can harbor a bug, or rather an "I
> didn't really think too much about it" situation.  Dependencies are added
> to suite XYZ independent of the speed that is used in the meson.build
> file: add a dependency to a func-thorough test and "make check-func"
> will build it at any requested speed.
>
> Fixing this is a one line change with some cleanup performed on the
> front, and that cleanup is patch 1.  This (at least to me) also makes
> the generated Makefile.mtest file a little bit easier to read, which is
> nice to have.

Have a:

Tested-by: Alex Bennée <alex.bennee@linaro.org>

for the series.

>
> Paolo
>
> v1->v2: support "make check-SUITE" where SUITE includes a speed
>
> Paolo Bonzini (3):
>   mtest2make: cleanup mtest-suites variables
>   mtest2make: add dependencies to the "speed-qualified" suite
>   mtest2make: do not repeat the same speed over and over
>
>  Makefile              |  4 +++-
>  scripts/mtest2make.py | 43 ++++++++++++++++++-------------------------
>  2 files changed, 21 insertions(+), 26 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro