Re: [PATCH v7 10/11] iotests: rewrite check into python

[Kevin Wolf]

Am 23.01.2021 um 16:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.01.2021 19:08, Kevin Wolf wrote:
> > Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Just use classes introduced in previous three commits. Behavior
> > > difference is described in these three commits.
> > > 
> > > Drop group file, as it becomes unused.
> > > 
> > > Drop common.env: now check is in python, and for tests we use same
> > > python interpreter that runs the check itself. Use build environment
> > > PYTHON in check-block instead, to keep "make check" use the same
> > > python.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > 
> > > diff --git a/tests/check-block.sh b/tests/check-block.sh
> > > index fb4c1baae9..26eb1c0a9b 100755
> > > --- a/tests/check-block.sh
> > > +++ b/tests/check-block.sh
> > > @@ -69,7 +69,7 @@ export QEMU_CHECK_BLOCK_AUTO=1
> > >   ret=0
> > >   for fmt in $format_list ; do
> > > -    ./check -makecheck -$fmt $group || ret=1
> > > +    ${PYTHON} ./check -makecheck -$fmt $group || ret=1
> > >   done
> > 
> > When I add an echo to print that command line, it seems that ${PYTHON}
> > is empty for me. Is this expected?
> 
> It seems to be defined defined when called from make check. Did you
> just call check-block directly?
D> 
> It's not intentional, but I think it's OK: if PYTHON is not defined
> let's just execute check as self-executable. And for make-check PYTHON
> is defined and correct python is used.

Hm, where does that happen in 'make check'? It seems the old makefiles
were quite readable in comparison to what we have now...

Anyway, I think 'make check-block' should run just the block-specific
subset of 'make check', without changing the behaviour of the remaining
tests. Anything that can be started through make should respect the
configured Python interpreter.

> > >   exit $ret
> > > diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> > > index 952762d5ed..914321806a 100755
> > > --- a/tests/qemu-iotests/check
> > > +++ b/tests/qemu-iotests/check
> 
> [..]
> 
> > > -            if [ -x "$binary" ]
> > > -            then
> > > -                export QEMU_PROG="$build_root/$binary"
> > > -                break
> > > -            fi
> > > -        done
> > > -        popd > /dev/null
> > > -        [ "$QEMU_PROG" = "" ] && _init_error "qemu not found"
> > > -    fi
> > 
> > I think this else branch is kind of important (if there is no system
> > emulator binary for the host architecture, find _any_ system emulator
> > binary that was built). I can't find its equivalent in the new code.
> 
> Hmm, I decided testing "first found" emulator is strange.. It seems
> like we have several emulators and user don't care which would be
> tested?

Remember that we're mainly testing the block layer, which is the same in
all qemu-system-* binaries anyway. So yes, any system emulator binary is
good enough for many test cases, and certainly better than having no
system emulator. Differences are only in the supported guest devices,
which may cause some tests to be skipped.

If there are multiple binaries that we could use, we could change the
way to select one instead of just the first one, e.g. by trying x86_64
first because this is what enables the largest set of tests.

But anything is better than failing with "qemu not found".

> Probably we should instead used qemu-system-* binary only if there is
> only one matching binary. And fail if there are many.

No, 'make check' shouldn't fail because I built arm and ppc emulators on
my x86_64 machine without also building a x86_64 emulator. (And I think
this is a case that fails both with the actual patch under review and
with your suggested change.)

Kevin