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/ Since you leave the front end alone, you don't need the first kind. The other two kinds are less clear. >> 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.