Hi, On Mon, Jun 27, 2022 at 05:29:26PM +0200, Markus Armbruster wrote: > Victor Toso <victort...@redhat.com> writes: > > > Hi Markus, > > > > On Mon, Jun 27, 2022 at 09:15:53AM +0200, Markus Armbruster wrote: > >> Victor Toso <victort...@redhat.com> writes: > >> > >> > Hi, > >> > > >> > This is the second iteration of RFC v1: > >> > https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00226.html > >> > > >> > > >> > # What this is about? > >> > > >> > To generate a simple Golang interface that could communicate with QEMU > >> > over QMP. The Go code that is generated is meant to be used as the bare > >> > bones to exchange QMP messages. > >> > > >> > The goal is to have this as a Go module in QEMU gitlab namespace, > >> > similar to what have been done to pyhon-qemu-qmp > >> > https://gitlab.com/qemu-project/python-qemu-qmp > >> > >> Aspects of review: > >> > >> (1) Impact on common code, if any > >> > >> I care, because any messes made there are likely to affect me down > >> the road. > > > > For the first version of the Go generated interface, my goal is > > to have something that works and can be considered alpha to other > > Go projects. > > > > With this first version, I don't want to bring huge changes to > > the python library or to the QAPI spec and its usage in QEMU. > > Unless someone finds that a necessity. > > > > So I hope (1) is simple :) > > > >> (2) The generated Go code > >> > >> Is it (close to) what we want long term? If not, is it good enough > >> short term, and how could we make necessary improvements? > >> > >> I'd prefer to leave this to folks who actually know their Go. > >> (3) General Python sanity > >> > >> We need eyes, but not necessarily mine. Any takers? > >> > >> [...] > >> > >> > scripts/qapi/golang.py | 765 +++++++++++++++++++++++++++++++++++++++++ > >> > scripts/qapi/main.py | 2 + > >> > 2 files changed, 767 insertions(+) > >> > create mode 100644 scripts/qapi/golang.py > >> > >> This adds a new generator and calls it from generate(), i.e. > >> review aspect (1) is empty. "Empty" is a quick & easy way to > >> get my ACK! > >> > >> No tests? > > > > I've added tests but on the qapi-go module, those are the files > > with _test.go prefix on them. Example for commands: > > > > > > https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/commands_test.go > > > > Should the generator itself have tests or offloading that to the > > qapi-go seems reasonable? > > Offloading may be reasonable, but how am I to run the tests then? > Documentation should tell me. > > We have roughly three kinds of tests so far: > > 1. Front end tests in tests/qapi-schema > > 2. Unit tests in tests/unit/ > > To find them: > > $ git-grep '#include ".*qapi-.*\.h"' tests/unit/ > > 3. Many tests talking QMP in tests/qtest/
I'm thinking on the tests in QEMU side. Perhaps adding something with Avocado that generates the qapi-go and communicates with a running QEMU with that generated Go module? One thing that I try to keep in mind is to not add Go dependencies in QEMU and this Golang work is not internal to QEMU itself. > Since you leave the front end alone, you don't need the first > kind. > > The other two kinds are less clear. I'm open for suggestions. I thought that, if we have a qapi-go Go module in Gitlab's qemu-project namespace, we could leverage most of the tests to the consumer of the actual generated code but I agree that it is necessary to have something in qemu too. > >> No documentation? > > > > Yes, this iteration removed the documentation of the generated > > types. I'm a bit sad about that. I want to consume the > > documentation in the QAPI files to provide the latest info from > > types/fields but we can't 'copy' it, the only solution is 'refer' > > to it with hyperlink, which I haven't done yet. > > Two kinds of documentation: generated documentation for the generated Go > code, and documentation about the generator. I was thinking of the > latter. Specifically, docs/devel/qapi-code-gen.rst section "Code > generation". Opinions on its usefulness differ. I like it. Me too. I'll add documentation for the next iteration, thanks for pointing it out. Cheers, Victor
signature.asc
Description: PGP signature
