Re: [PATCH v1 0/9] qapi-go: add generator for Golang interface

[Victor Toso]

Hi,

On Thu, Sep 28, 2023 at 02:40:27PM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 27, 2023 at 01:25:35PM +0200, Victor Toso wrote:
> > Hi, long time no see!
> > 
> > This patch series intent is to introduce a generator that produces a Go
> > module for Go applications to interact over QMP with QEMU.
> 
> snip
>  
> > Victor Toso (9):
> >   qapi: golang: Generate qapi's enum types in Go
> >   qapi: golang: Generate qapi's alternate types in Go
> >   qapi: golang: Generate qapi's struct types in Go
> >   qapi: golang: structs: Address 'null' members
> >   qapi: golang: Generate qapi's union types in Go
> >   qapi: golang: Generate qapi's event types in Go
> >   qapi: golang: Generate qapi's command types in Go
> >   qapi: golang: Add CommandResult type to Go
> >   docs: add notes on Golang code generator
> > 
> >  docs/devel/qapi-golang-code-gen.rst |  341 +++++++++
> >  scripts/qapi/golang.py              | 1047 +++++++++++++++++++++++++++
> >  scripts/qapi/main.py                |    2 +
> >  3 files changed, 1390 insertions(+)
> >  create mode 100644 docs/devel/qapi-golang-code-gen.rst
> >  create mode 100644 scripts/qapi/golang.py
> 
> So the formatting of the code is kinda all over the place eg
> 
> func (s *StrOrNull) ToAnyOrAbsent() (any, bool) {
>     if s != nil {
>         if s.IsNull {
>             return nil, false
>     } else if s.S != nil {
>         return *s.S, false
>         }
>     }
> 
>     return nil, true
> }
> 
> 
> ought to be
> 
> func (s *StrOrNull) ToAnyOrAbsent() (any, bool) {
>         if s != nil {
>                 if s.IsNull {
>                         return nil, false
>                 } else if s.S != nil {
>                         return *s.S, false
>                 }
>         }
> 
>         return nil, true
> }
> 
> I'd say we should add a 'make check-go' target, wired into 'make check'
> that runs 'go fmt' on the generated code to validate that we generated
> correct code. Then fix the generator to actually emit the reqired
> format.

As mentioned in another thread, my main concern with this is
requiring go binaries in the make script. Might be fine if the
scope is only when a release is done, but shouldn't be a default
requirement.

> Having said that, this brings up the question of how we expect apps to
> consume the Go code. Generally an app would expect to just add the
> module to their go.mod file, and have the toolchain download it on
> the fly during build.
> 
> If we assume a go namespace under qemu.org, we'll want one or more
> modules. Either we have one module, containing APIs for all of QEMU,
> QGA, and QSD, or we have separate go modules for each. I'd probably
> tend towards the latter, since it means we can version them separately
> which might be useful if we're willing to break API in one of them,
> but not the others.
> 
> IOW, integrating this directly into qemu.git as a build time output
> is not desirable in this conext though, as 'go build' can't consume
> that.
> 
> IOW, it would push towards
> 
>    go-qemu.git
>    go-qga.git
>    go-qsd.git
> 
> and at time of each QEMU release, we would need to invoke the code
> generator, and store its output in the respective git modules.

In which point, I think it is fair to run the gofmt and goimports.
Still, if you think it isn't a problem to add such make check-go
target with tooling specific to go code in them, I'll add that to
next iteration.

> This would also need the generator application to be a
> standalone tool, separate from the C QAPI generator.

It is. I mean, both run together now but that can be improved.

> Finally Go apps would want to do
> 
>    import (
>        qemu.org/go/qemu
>        qemu.org/go/qga
>        qemu.org/go/gsd
>    )
> 
> and would need us to create the "https://qemu.org/go/qemu"; page
> containing dummy HTML content with 
> 
>     <meta name="go-import" content="qemu.org/go/qemu git 
> https://gitlab.com/qemu-project/go-qemu.git@/>

Neat. I didn't know this. Yes, we want that, but with different
name for the git [0]. Perhaps just another folder:

    https://gitlab.com/qemu-project/go/qemu.git
    https://gitlab.com/qemu-project/go/qga.git
    https://gitlab.com/qemu-project/go/gsd.git

> and likewise for the other modules.

[0] https://github.com/digitalocean/go-qemu

Thanks again for the reviews!

Cheers,
Victor

signature.asc
Description: PGP signature