We can get a nicer progress indication if we add the iotests
individually via the 'check' script instead of going through
the check-block.sh wrapper.

For this, we have to add some of the sanity checks that have
originally been done in the tests/check-block.sh script (whether
"bash" is available or whether CFLAGS contain -fsanitize switches)
to the meson.build file now, and add the environment variables
that have been set up by the tests/check-block.sh script before.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index e1832c90e0..5a6ccd35d8 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -1,9 +1,29 @@
-if not have_tools or targetos == 'windows'
+if not have_tools or targetos == 'windows' or \
+   config_host.has_key('CONFIG_GPROF')
   subdir_done()
 endif
 
+bash = find_program('bash', required: false)
+if not bash.found() or \
+   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
+  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
+  subdir_done()
+endif
+
+foreach cflag: config_host['QEMU_CFLAGS'].split()
+  if cflag.startswith('-fsanitize') and \
+     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
+    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
+    subdir_done()
+  endif
+endforeach
+
 qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
-qemu_iotests_env = {'PYTHON': python.full_path()}
+qemu_iotests_env = {
+  'PYTHON': python.full_path(),
+  'PYTHONUTF8': '1',
+  'QEMU_CHECK_BLOCK_AUTO': '1'
+}
 qemu_iotests_formats = {
   'qcow2': 'quick',
   'raw': 'slow',
@@ -18,16 +38,25 @@ foreach k, v : emulators
   endif
 endforeach
 
+check_script = find_program(meson.current_build_dir() / 'check')
+iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
+                      check: true).stdout().strip().replace('tests/', '').split('\n')
+
 foreach format, speed: qemu_iotests_formats
   if speed == 'quick'
     suites = 'block'
   else
     suites = ['block-' + speed, speed]
   endif
-  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
-       depends: qemu_iotests_binaries, env: qemu_iotests_env,
-       protocol: 'tap',
-       suite: suites,
-       timeout: 0,
-       is_parallel: false)
+  foreach tst: iotests
+    test('iotest-' + format + '-' + tst,
+         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
+         depends: qemu_iotests_binaries,
+         env: qemu_iotests_env + \
+              { 'TEST_DIR':
+                meson.current_build_dir() / 'scratch' / format + '-' + tst },
+         protocol: 'tap',
+         suite: suites,
+         timeout: 0)
+  endforeach
 endforeach
-- 
2.27.0

On 08.02.22 11:13, Thomas Huth wrote:
> We can get a nicer progress indication if we add the iotests
> individually via the 'check' script instead of going through
> the check-block.sh wrapper.
>
> For this, we have to add some of the sanity checks that have
> originally been done in the tests/check-block.sh script (whether
> "bash" is available or whether CFLAGS contain -fsanitize switches)
> to the meson.build file now, and add the environment variables
> that have been set up by the tests/check-block.sh script before.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>   1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> index e1832c90e0..5a6ccd35d8 100644
> --- a/tests/qemu-iotests/meson.build
> +++ b/tests/qemu-iotests/meson.build
> @@ -1,9 +1,29 @@
> -if not have_tools or targetos == 'windows'
> +if not have_tools or targetos == 'windows' or \
> +   config_host.has_key('CONFIG_GPROF')
>     subdir_done()
>   endif
>   
> +bash = find_program('bash', required: false)
> +if not bash.found() or \
> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')

Instead of me asking about where the LANG=C is, or me lamenting that we 
could test very simply for [123] before and can no longer now... Can we 
not just do `find_program('bash', required: false, version: '>= 4.0')`?

> +  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
> +  subdir_done()
> +endif
> +
> +foreach cflag: config_host['QEMU_CFLAGS'].split()
> +  if cflag.startswith('-fsanitize') and \
> +     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
> +    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
> +    subdir_done()
> +  endif
> +endforeach
> +
>   qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
> -qemu_iotests_env = {'PYTHON': python.full_path()}
> +qemu_iotests_env = {
> +  'PYTHON': python.full_path(),
> +  'PYTHONUTF8': '1',
> +  'QEMU_CHECK_BLOCK_AUTO': '1'
> +}
>   qemu_iotests_formats = {
>     'qcow2': 'quick',
>     'raw': 'slow',
> @@ -18,16 +38,25 @@ foreach k, v : emulators
>     endif
>   endforeach
>   
> +check_script = find_program(meson.current_build_dir() / 'check')
> +iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
> +                      check: true).stdout().strip().replace('tests/', '').split('\n')
> +
>   foreach format, speed: qemu_iotests_formats
>     if speed == 'quick'
>       suites = 'block'
>     else
>       suites = ['block-' + speed, speed]
>     endif
> -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
> -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
> -       protocol: 'tap',
> -       suite: suites,
> -       timeout: 0,
> -       is_parallel: false)
> +  foreach tst: iotests
> +    test('iotest-' + format + '-' + tst,
> +         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
> +         depends: qemu_iotests_binaries,
> +         env: qemu_iotests_env + \
> +              { 'TEST_DIR':
> +                meson.current_build_dir() / 'scratch' / format + '-' + tst },
> +         protocol: 'tap',
> +         suite: suites,
> +         timeout: 0)

So as far I understand you’d like to have meson run the iotests in 
parallel this way.  I don’t actually think that’s safely possible for 
multiple formats at once, because a test’s output is always written into 
`${build_dir}/tests/qemu-iotests/${seq}.out.bad`; so if you run e.g. 
test 001 both with raw and qcow2 simultaneously, then they can get in 
each other’s way.

(In my test branch, I have 
https://gitlab.com/hreitz/qemu/-/commit/f3110b1eeb93d02aeadc5c8b807594cfa10a6aad 
for this – maybe I should send something like this in a more refined 
form to the list some time...)

As a minor note, the `check` script has recently received a `-j` 
argument for parallel execution.  Kind of a shame that we wouldn’t be 
able to use it here, but that’s how it is, I suppose.

Hanna

On 08/02/2022 14.12, Hanna Reitz wrote:
> On 08.02.22 11:13, Thomas Huth wrote:
>> We can get a nicer progress indication if we add the iotests
>> individually via the 'check' script instead of going through
>> the check-block.sh wrapper.
>>
>> For this, we have to add some of the sanity checks that have
>> originally been done in the tests/check-block.sh script (whether
>> "bash" is available or whether CFLAGS contain -fsanitize switches)
>> to the meson.build file now, and add the environment variables
>> that have been set up by the tests/check-block.sh script before.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
>> index e1832c90e0..5a6ccd35d8 100644
>> --- a/tests/qemu-iotests/meson.build
>> +++ b/tests/qemu-iotests/meson.build
>> @@ -1,9 +1,29 @@
>> -if not have_tools or targetos == 'windows'
>> +if not have_tools or targetos == 'windows' or \
>> +   config_host.has_key('CONFIG_GPROF')
>>     subdir_done()
>>   endif
>> +bash = find_program('bash', required: false)
>> +if not bash.found() or \
>> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
> 
> Instead of me asking about where the LANG=C is, or me lamenting that we 
> could test very simply for [123] before and can no longer now... Can we not 
> just do `find_program('bash', required: false, version: '>= 4.0')`?

Oh, cool, find_program() has a version parameter, didn't know that before! 
Thanks for the hint, I'll give it a try!

>> +  foreach tst: iotests
>> +    test('iotest-' + format + '-' + tst,
>> +         python, args: [check_script.full_path(), '-tap', '-' + format, 
>> tst],
>> +         depends: qemu_iotests_binaries,
>> +         env: qemu_iotests_env + \
>> +              { 'TEST_DIR':
>> +                meson.current_build_dir() / 'scratch' / format + '-' + 
>> tst },
>> +         protocol: 'tap',
>> +         suite: suites,
>> +         timeout: 0)
> 
> So as far I understand you’d like to have meson run the iotests in parallel 
> this way.  I don’t actually think that’s safely possible for multiple 
> formats at once, because a test’s output is always written into 
> `${build_dir}/tests/qemu-iotests/${seq}.out.bad`; so if you run e.g. test 
> 001 both with raw and qcow2 simultaneously, then they can get in each 
> other’s way.

Drat, I think you're right. I was testing with "make check SPEED=slow" and 
that was still working fine, but with "make check SPEED=thorough" I'm 
getting errors, indeed.

> (In my test branch, I have 
> https://gitlab.com/hreitz/qemu/-/commit/f3110b1eeb93d02aeadc5c8b807594cfa10a6aad 
> for this – maybe I should send something like this in a more refined form to 
> the list some time...)

Thanks a lot, that fixes the problems with SPEED=thorough indeed!

  Thomas

On 2/8/22 11:13, Thomas Huth wrote:
> We can get a nicer progress indication if we add the iotests
> individually via the 'check' script instead of going through
> the check-block.sh wrapper.
> 
> For this, we have to add some of the sanity checks that have
> originally been done in the tests/check-block.sh script (whether
> "bash" is available or whether CFLAGS contain -fsanitize switches)
> to the meson.build file now, and add the environment variables
> that have been set up by the tests/check-block.sh script before.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>   1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> index e1832c90e0..5a6ccd35d8 100644
> --- a/tests/qemu-iotests/meson.build
> +++ b/tests/qemu-iotests/meson.build
> @@ -1,9 +1,29 @@
> -if not have_tools or targetos == 'windows'
> +if not have_tools or targetos == 'windows' or \
> +   config_host.has_key('CONFIG_GPROF')
>     subdir_done()
>   endif
>   
> +bash = find_program('bash', required: false)
> +if not bash.found() or \
> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
> +  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
> +  subdir_done()
> +endif
> +
> +foreach cflag: config_host['QEMU_CFLAGS'].split()
> +  if cflag.startswith('-fsanitize') and \
> +     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
> +    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
> +    subdir_done()
> +  endif
> +endforeach
> +
>   qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
> -qemu_iotests_env = {'PYTHON': python.full_path()}
> +qemu_iotests_env = {
> +  'PYTHON': python.full_path(),
> +  'PYTHONUTF8': '1',
> +  'QEMU_CHECK_BLOCK_AUTO': '1'
> +}
>   qemu_iotests_formats = {
>     'qcow2': 'quick',
>     'raw': 'slow',
> @@ -18,16 +38,25 @@ foreach k, v : emulators
>     endif
>   endforeach
>   
> +check_script = find_program(meson.current_build_dir() / 'check')
> +iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
> +                      check: true).stdout().strip().replace('tests/', '').split('\n')

The main disadvantage here is that changes to the test directory will 
not rebuild meson.build.  So when one creates a new test, meson has to 
be rerun manually.

It is probably possible to do something like:

- run "check -g auto -n" with configure_file() and store the output in a 
file

- in the Makefile, always run "check -g auto -n", like

check-block-test-list: check-block-test-list.stamp
check-block-test-list.stamp:
	./check -g auto -n > check-block-test-list.stamp; \
	if cmp check-block-test-list.stamp check-block-test-list; then \
		cp check-block-test-list.stamp check-block-test-list; \
	fi
.PHONY: check-block-test-list.stamp

check check-block: check-block-test-list.stamp

before giving control to "meson test"; but I'm not sure if that will 
also cause a rebuild of Makefile.mtest and a meson rerun.

But I'm not sure it's worth it...

Regarding the issue that Peter pointed out, that's my fault for not 
being aware of the "diff" being in the output of failing tests.  There 
are three ways to fix it:

1) reverting my patches

2) Thomas's series, but only if the above issue is fixed

3) shipping the tests/qemu-iotests/*.bad files as artifacts

4) not using -tap (either reverting commit d316859f4e, "check-block: 
replace -makecheck with TAP output", or just removing -tap from 
tests/qemu-iotests/meson.build).

My preference is 4, then 1 or 2 (depending on the complexity), then 3.

Paolo

>   foreach format, speed: qemu_iotests_formats
>     if speed == 'quick'
>       suites = 'block'
>     else
>       suites = ['block-' + speed, speed]
>     endif
> -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
> -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
> -       protocol: 'tap',
> -       suite: suites,
> -       timeout: 0,
> -       is_parallel: false)
> +  foreach tst: iotests
> +    test('iotest-' + format + '-' + tst,
> +         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
> +         depends: qemu_iotests_binaries,
> +         env: qemu_iotests_env + \
> +              { 'TEST_DIR':
> +                meson.current_build_dir() / 'scratch' / format + '-' + tst },
> +         protocol: 'tap',
> +         suite: suites,
> +         timeout: 0)
> +  endforeach
>   endforeach

On 08/02/2022 13.36, Paolo Bonzini wrote:
> On 2/8/22 11:13, Thomas Huth wrote:
>> We can get a nicer progress indication if we add the iotests
>> individually via the 'check' script instead of going through
>> the check-block.sh wrapper.
>>
>> For this, we have to add some of the sanity checks that have
>> originally been done in the tests/check-block.sh script (whether
>> "bash" is available or whether CFLAGS contain -fsanitize switches)
>> to the meson.build file now, and add the environment variables
>> that have been set up by the tests/check-block.sh script before.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
>> index e1832c90e0..5a6ccd35d8 100644
>> --- a/tests/qemu-iotests/meson.build
>> +++ b/tests/qemu-iotests/meson.build
>> @@ -1,9 +1,29 @@
>> -if not have_tools or targetos == 'windows'
>> +if not have_tools or targetos == 'windows' or \
>> +   config_host.has_key('CONFIG_GPROF')
>>     subdir_done()
>>   endif
>> +bash = find_program('bash', required: false)
>> +if not bash.found() or \
>> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
>> +  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
>> +  subdir_done()
>> +endif
>> +
>> +foreach cflag: config_host['QEMU_CFLAGS'].split()
>> +  if cflag.startswith('-fsanitize') and \
>> +     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
>> +    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
>> +    subdir_done()
>> +  endif
>> +endforeach
>> +
>>   qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
>> -qemu_iotests_env = {'PYTHON': python.full_path()}
>> +qemu_iotests_env = {
>> +  'PYTHON': python.full_path(),
>> +  'PYTHONUTF8': '1',
>> +  'QEMU_CHECK_BLOCK_AUTO': '1'
>> +}
>>   qemu_iotests_formats = {
>>     'qcow2': 'quick',
>>     'raw': 'slow',
>> @@ -18,16 +38,25 @@ foreach k, v : emulators
>>     endif
>>   endforeach
>> +check_script = find_program(meson.current_build_dir() / 'check')
>> +iotests = run_command(python, [check_script.full_path(), '-g', 'auto', 
>> '-n'],
>> +                      check: true).stdout().strip().replace('tests/', 
>> '').split('\n')
> 
> The main disadvantage here is that changes to the test directory will not 
> rebuild meson.build.  So when one creates a new test, meson has to be rerun 
> manually.
> 
> It is probably possible to do something like:
> 
> - run "check -g auto -n" with configure_file() and store the output in a file
> 
> - in the Makefile, always run "check -g auto -n", like
> 
> check-block-test-list: check-block-test-list.stamp
> check-block-test-list.stamp:
>      ./check -g auto -n > check-block-test-list.stamp; \
>      if cmp check-block-test-list.stamp check-block-test-list; then \
>          cp check-block-test-list.stamp check-block-test-list; \
>      fi
> .PHONY: check-block-test-list.stamp
> 
> check check-block: check-block-test-list.stamp
> 
> before giving control to "meson test"; but I'm not sure if that will also 
> cause a rebuild of Makefile.mtest and a meson rerun.
> 
> But I'm not sure it's worth it...

Before we go down that road, I think it would be better to get rid of the 
"auto" group and add the list of the iotests that should get run during 
"make check" to the meson.build file directly. That's easier to understand 
and less confusing.

> Regarding the issue that Peter pointed out, that's my fault for not being 
> aware of the "diff" being in the output of failing tests.  There are three 
> ways to fix it:
> 
> 1) reverting my patches
> 
> 2) Thomas's series, but only if the above issue is fixed
> 
> 3) shipping the tests/qemu-iotests/*.bad files as artifacts
> 
> 4) not using -tap (either reverting commit d316859f4e, "check-block: replace 
> -makecheck with TAP output", or just removing -tap from 
> tests/qemu-iotests/meson.build).
> 
> My preference is 4, then 1 or 2 (depending on the complexity), then 3.

What about simply printing the diff to stderr instead of stdout?
Something like this seems to work, at least for a quick glance:

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0eace147b8..d45a2688a0 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -404,7 +404,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> 
bool:
              if res.status == 'fail':
                  failed.append(name)
                  if res.diff:
-                    print('\n'.join(res.diff))
+                    print('\n'.join(res.diff), file=sys.stderr)
              elif res.status == 'not run':
                  notrun.append(name)
              elif res.status == 'pass':

  Thomas

On 2/8/22 16:10, Thomas Huth wrote:
> Before we go down that road, I think it would be better to get rid of
> the "auto" group and add the list of the iotests that should get run
> during "make check" to the meson.build file directly. That's easier to
> understand and less confusing.

There are other groups than "auto", though.  It's a bit contrarian to 
the aim that changing one aspect of the build should only touch a single 
place (see commit 32fcc6244c, "contrib/vhost-user-input: convert to 
meson", for a reminder of what it was like to add an executable with 
Makefiles!).

> diff --git a/tests/qemu-iotests/testrunner.py 
> b/tests/qemu-iotests/testrunner.py
> index 0eace147b8..d45a2688a0 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -404,7 +404,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) 
> -> bool:
>               if res.status == 'fail':
>                   failed.append(name)
>                   if res.diff:
> -                    print('\n'.join(res.diff))
> +                    print('\n'.join(res.diff), file=sys.stderr)
>               elif res.status == 'not run':
>                   notrun.append(name)
>               elif res.status == 'pass':

Interesting.  But it should be done only if self.tap is true.

Paolo