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
