Re: [PATCH 1/2] python: introduce qmp-shell-wrap convenience tool

[John Snow <jsnow@redhat.com>]

On Tue, Jan 18, 2022 at 5:06 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jan 17, 2022 at 06:27:24PM -0500, John Snow wrote:
> > On Mon, Jan 17, 2022 at 9:11 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > With the current 'qmp-shell' tool developers must first spawn QEMU with
> > > a suitable -qmp arg and then spawn qmp-shell in a separate terminal
> > > pointing to the right socket.
> > >
> > > With 'qmp-shell-wrap' developers can ignore QMP sockets entirely and
> > > just pass the QEMU command and arguments they want. The program will
> > > listen on a UNIX socket and tell QEMU to connect QMP to that.
> > >
> > > For example, this:
> > >
> > >  # qmp-shell-wrap -- qemu-system-x86_64 -display none
> > >
> > > Is roughly equivalent of running:
> > >
> > >  # qemu-system-x86_64 -display none -qmp qmp-shell-1234 &
> > >  # qmp-shell qmp-shell-1234
> > >
> > > Except that 'qmp-shell-wrap' switches the socket peers around so that
> > > it is the UNIX socket server and QEMU is the socket client. This makes
> > > QEMU reliably go away when qmp-shell-wrap exits, closing the server
> > > socket.
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  python/qemu/qmp/qmp_shell.py | 61 +++++++++++++++++++++++++++++++++---
> > >  scripts/qmp/qmp-shell-wrap   | 11 +++++++
> > >  2 files changed, 68 insertions(+), 4 deletions(-)
> > >  create mode 100755 scripts/qmp/qmp-shell-wrap
> > >
> > > diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
> > > index e7d7eb18f1..12f7d28afc 100644
> > > --- a/python/qemu/qmp/qmp_shell.py
> > > +++ b/python/qemu/qmp/qmp_shell.py
> > > @@ -86,6 +86,7 @@
> > >  import os
> > >  import re
> > >  import readline
> > > +from subprocess import Popen
> > >  import sys
> > >  from typing import (
> > >      Iterator,
> > > @@ -162,8 +163,10 @@ class QMPShell(qmp.QEMUMonitorProtocol):
> > >      :param verbose: Echo outgoing QMP messages to console.
> > >      """
> > >      def __init__(self, address: qmp.SocketAddrT,
> > > -                 pretty: bool = False, verbose: bool = False):
> > > -        super().__init__(address)
> > > +                 pretty: bool = False,
> > > +                 verbose: bool = False,
> > > +                 server: bool = False):
> > > +        super().__init__(address, server=server)
> > >          self._greeting: Optional[QMPMessage] = None
> > >          self._completer = QMPCompleter()
> > >          self._transmode = False
> > > @@ -404,8 +407,10 @@ class HMPShell(QMPShell):
> > >      :param verbose: Echo outgoing QMP messages to console.
> > >      """
> > >      def __init__(self, address: qmp.SocketAddrT,
> > > -                 pretty: bool = False, verbose: bool = False):
> > > -        super().__init__(address, pretty, verbose)
> > > +                 pretty: bool = False,
> > > +                 verbose: bool = False,
> > > +                 server: bool = False):
> > > +        super().__init__(address, pretty, verbose, server)
> > >          self._cpu_index = 0
> > >
> > >      def _cmd_completion(self) -> None:
> > > @@ -529,6 +534,54 @@ def main() -> None:
> > >          for _ in qemu.repl():
> > >              pass
> > >
> > > +def main_wrap() -> None:
> > > +    """
> > > +    qmp-shell-wrap entry point: parse command line arguments and start the REPL.
> > > +    """
> > > +    parser = argparse.ArgumentParser()
> > > +    parser.add_argument('-H', '--hmp', action='store_true',
> > > +                        help='Use HMP interface')
> > > +    parser.add_argument('-v', '--verbose', action='store_true',
> > > +                        help='Verbose (echo commands sent and received)')
> > > +    parser.add_argument('-p', '--pretty', action='store_true',
> > > +                        help='Pretty-print JSON')
> > > +
> > > +    parser.add_argument('command', nargs=argparse.REMAINDER,
> > > +                        help='QEMU command line to invoke')
> > > +
> > > +    args = parser.parse_args()
> > > +
> > > +    cmd = args.command
> > > +    if len(cmd) != 0 and cmd[0] == '--':
> > > +        cmd = cmd[1:]
> > > +    if len(cmd) == 0:
> > > +        cmd = "qemu-system-x86_64"
> > > +
> > > +    sockpath = "qmp-shell-wrap-%d" % os.getpid()
> > > +    cmd += ["-qmp", "unix:%s" % sockpath]
> > > +
> > > +    shell_class = HMPShell if args.hmp else QMPShell
> > > +
> > > +    try:
> > > +        address = shell_class.parse_address(sockpath)
> > > +    except qmp.QMPBadPortError:
> > > +        parser.error(f"Bad port number: {socketpath}")
> > > +        return  # pycharm doesn't know error() is noreturn
> > > +
> > > +    with shell_class(address, args.pretty, args.verbose, True) as qemu:
> > > +        qemuproc = Popen(cmd)
> > > +
> > > +        try:
> > > +            qemu.accept()
> > > +        except qmp.QMPConnectError:
> > > +            die("Didn't get QMP greeting message")
> > > +        except qmp.QMPCapabilitiesError:
> > > +            die("Couldn't negotiate capabilities")
> > > +        except OSError as err:
> > > +            die(f"Couldn't connect to {sockpath}: {err!s}")
> > > +
> > > +        for _ in qemu.repl():
> > > +            pass
> > >
> > >  if __name__ == '__main__':
> > >      main()
> > > diff --git a/scripts/qmp/qmp-shell-wrap b/scripts/qmp/qmp-shell-wrap
> > > new file mode 100755
> > > index 0000000000..9e94da114f
> > > --- /dev/null
> > > +++ b/scripts/qmp/qmp-shell-wrap
> > > @@ -0,0 +1,11 @@
> > > +#!/usr/bin/env python3
> > > +
> > > +import os
> > > +import sys
> > > +
> > > +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> > > +from qemu.qmp import qmp_shell
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > +    qmp_shell.main_wrap()
> > > --
> > > 2.33.1
> > >
> >
> > Adds some new failures to the python linters; try "make check-dev" in
> > the python sub-dir.
>
> It would be nice to just have this integrated into 'make check' so we
> don't need to remember to run a special command.

The CI will run it, but 'make check' doesn't. To add it to make check,
I need to figure out how to insert a venv-building step into 'make
check' such that the venv gets deposited into the build dir instead of
the source dir.
I think I may also need yet another set of package dependencies that
pin on precise dependencies for testing purposes to prevent random
regressions during 'make check' when nobody has touched the Python
code.

Overall, I felt like maybe it was more hassle than it was worth if I
can just nudge people touching the python to run a 'make check-dev'
every so often.

Patches welcome, etc. My overall strategy with the python tests so far has been:

(1) Keep python tests fully separate from the QEMU build system to
allow them to be split out into new repositories easily.
(2) Use the pipenv test to lock the very oldest dependencies the code
and tests support, using the very oldest python we support (3.6) This
test is used as the gating test in GitLab CI, as it is very repeatable
and the GitLab CI setup ensures I can always have the exact Python
packages it requires available.
(3) Use the tox test to test against a wide variety of Python
interpreters (3.6 through 3.10 inclusive) using the very latest python
packages to detect regressions on cutting-edge environments
(4) Use the widest possible range of versions for dependent packages
in setup.cfg such that QEMU packages are unlikely to cause versioning
conflicts in environments that decide to integrate our code.

Overall, I test on 3.6 through 3.10, and against the "oldest" and
"newest" dependencies. It's a good, wide matrix.

However, It's #4 there that runs me into trouble with tests that are
guaranteed to pass -- the linters update all the time and cause new
problems. I use pipenv to lock to specific versions, but that tool
wants to run against Python 3.6 *explicitly*, so it isn't suitable for
a generic purpose 'make check' because not everyone will have a Python
3.6 interpreter available. I need something kind of halfway between,
where I can lock against specific versions but not against the Python
interpreter version, and that's what could be used for a decent 'make
check' test.

Of course, I don't want to maintain like 10 versions of a dependent
packages list, either.

(I really, really wish pip had an --use-oldest flag for dependency
resolution, but it doesn't.)

>
> > ... Though, due to a bug in avocado, this helpfully doesn't actually
> > show you the failure output right now ...
>
> Urgh.
>

It regressed during winter vacation. I sent a small patch to enable
the new runner [1], and I have been discussing with Beraldo how to
re-enable the Coverage.py tests under the new Avocado.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg03614.html