Re: [PATCH v6 08/11] iotests: add testenv.py

[Vladimir Sementsov-Ogievskiy]

15.01.2021 15:45, Kevin Wolf wrote:
> Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 15.01.2021 14:18, Kevin Wolf wrote:
> > > Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > Add TestEnv class, which will handle test environment in a new python
> > > > iotests running framework.
> > > >
> > > > Difference with current ./check interface:
> > > > - -v (verbose) option dropped, as it is unused
> > > >
> > > > - -xdiff option is dropped, until somebody complains that it is needed
> > > > - same for -n option
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > ---
> > > >    tests/qemu-iotests/testenv.py | 328 ++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 328 insertions(+)
> > > >    create mode 100755 tests/qemu-iotests/testenv.py

[..]

> > > > +    def init_binaries(self):
> > > > +        """Init binary path variables:
> > > > +             PYTHON (for bash tests)
> > > > +             QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
> > > > +             SOCKET_SCM_HELPER
> > > > +        """
> > > > +        self.python = '/usr/bin/python3 -B'
> > >
> > > This doesn't look right, we need to respect the Python binary set in
> > > configure (which I think we get from common.env)
> >
> > Oh, I missed the change. Then I should just drop this self.python.
>
> Do we still get the value from elsewhere or do we need to manually parse
> common.env?

Hmm.. Good question. We have either parse common.env, and still create 
self.python variable.

Or drop it, and include common.env directly to bash tests. For this we'll need 
to export

BUILD_IOTESTS, and do
 . $BUILD_IOTESTS/common.env

in common.rc..


> > > > +        def root(*names):
> > > > +            return os.path.join(self.build_root, *names)
> > > > +
> > > > +        arch = os.uname().machine
> > > > +        if 'ppc64' in arch:
> > > > +            arch = 'ppc64'
> > > > +
> > > > +        self.qemu_prog = os.getenv('QEMU_PROG', root(f'qemu-system-{arch}'))
> > > > +        self.qemu_img_prog = os.getenv('QEMU_IMG_PROG', root('qemu-img'))
> > > > +        self.qemu_io_prog = os.getenv('QEMU_IO_PROG', root('qemu-io'))
> > > > +        self.qemu_nbd_prog = os.getenv('QEMU_NBD_PROG', root('qemu-nbd'))
> > > > +        self.qsd_prog = os.getenv('QSD_PROG', root('storage-daemon',
> > > > +                                                   'qemu-storage-daemon'))
> > > > +
> > > > +        for b in [self.qemu_img_prog, self.qemu_io_prog, self.qemu_nbd_prog,
> > > > +                  self.qemu_prog, self.qsd_prog]:
> > > > +            if not os.path.exists(b):
> > > > +                exit('Not such file: ' + b)
> > > > +            if not os.access(b, os.X_OK):
> > > > +                exit('Not executable: ' + b)
> > > > +
> > > > +        helper_path = os.path.join(self.build_iotests, 'socket_scm_helper')
> > > > +        if os.access(helper_path, os.X_OK):
> > > > +            self.socket_scm_helper = helper_path  # SOCKET_SCM_HELPER
> > > > +
> > > > +    def __init__(self, argv: List[str]) -> None:
> > > > +        """Parse args and environment"""
> > > > +
> > > > +        # Initialize generic paths: build_root, build_iotests, source_iotests,
> > > > +        # which are needed to initialize some environment variables. They are
> > > > +        # used by init_*() functions as well.
> > > > +
> > > > +
> > > > +        if os.path.islink(sys.argv[0]):
> > > > +            # called from the build tree
> > > > +            self.source_iotests = os.path.dirname(os.readlink(sys.argv[0]))
> > > > +            self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0]))
> > > > +        else:
> > > > +            # called from the source tree
> > > > +            self.source_iotests = os.getcwd()
> > > > +            self.build_iotests = self.source_iotests
> > > > +
> > > > +        self.build_root = os.path.join(self.build_iotests, '..', '..')
> > > > +
> > > > +        self.init_handle_argv(argv)
> > > > +        self.init_directories()
> > > > +        self.init_binaries()
> > > > +
> > > > +        # QEMU_OPTIONS
> > > > +        self.qemu_options = '-nodefaults -display none -accel qtest'
> > > > +        machine_map = (
> > > > +            (('arm', 'aarch64'), 'virt'),
> > >
> > > How does this work? Won't we check for "qemu-system-('arm', 'aarch64')"
> > > below, which we'll never find?
> >
> > Hmm. I just took existing logic from check:
> >
> > [..]
> >    case "$QEMU_PROG" in
> >        *qemu-system-arm|*qemu-system-aarch64)
> >            export QEMU_OPTIONS="$QEMU_OPTIONS -machine virt"
> >            ;;
> > [..]
>
> What I mean is that the code below doesn't look like it's prepared to
> interpret a tuple like ('arm', 'aarch64'), it expects a single string:
>
> > > > +            ('avr', 'mega2560'),
> > > > +            ('rx', 'gdbsim-r5f562n8'),
> > > > +            ('tricore', 'tricore_testboard')
> > > > +        )
> > > > +        for suffix, machine in machine_map:
> > > > +            if self.qemu_prog.endswith(f'qemu-system-{suffix}'):
>
> Here we get effectively:
>
>      suffix: Tuple[str, str] = ('arm', 'aarch64')
>
> The formatted string uses str(suffix), which makes the result
> "qemu-system-('arm', 'aarch64')".
>
> Or am I misunderstanding something here?

Ah, you are right:) Will fix. I interpreted your

  Won't we check for "qemu-system-('arm', 'aarch64')"

as
  
  Won't we check for "qemu-system-arm" or "qemu-system-aarch64"



--
Best regards,
Vladimir