Re: [PATCH v1 0/7] Validate and test qapi examples
Hi,
On Fri, Sep 08, 2023 at 09:51:35AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/9/23 20:17, Victor Toso wrote:
> > Hi,
>
> > >File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line
> > > 118, in parse_examples_of
> > > assert((obj.doc is not None))
> > > ^^^
> > > AssertionError
> > > ninja: build stopped: subcommand failed.
> > >
> > > not sure if that's related to the examples that still need fixing or not ?
> >
> > This is related to the script being fed with data without
> > documentation. In general, asserting should be the right approach
> > because we don't want API without docs but this failure comes
> > from the tests, that is, adding the following diff:
> >
> > diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
> > index c14ed11774..a961c0575d 100644
> > --- a/scripts/qapi/dumpexamples.py
> > +++ b/scripts/qapi/dumpexamples.py
> > @@ -115,6 +115,10 @@ def parse_examples_of(self:
> > QAPISchemaGenExamplesVisitor,
> >
> > assert(name in self.schema._entity_dict)
> > obj = self.schema._entity_dict[name]
> > +if obj.doc is None:
> > +print(f"{name} does not have documentation")
> > +return
> > +
> > assert((obj.doc is not None))
> > module_name = obj._module.name
> >
> > gives:
> >
> > user-def-cmd0 does not have documentation
> > user-def-cmd does not have documentation
> [...]
>
> > So, not sure if we should:
> > 1. Avoid asserting when running with tests
>
> This seems the most sensible option, adding an argument to
> the 'command' invoked by meson's test_qapi_files() target in
> tests/meson.build.
In a offline discussion with Markus, he pointed out to me the
existence of 'pragma': { 'doc-required': true }, which means we
can possibly avoid extra flags and changes in meson and handle
this in the generator. I'll update the code and submit, probably
Tomorrow :)
Cheers,
Victor
> > 2. Avoid running this generator with tests
> > 3. Add some minimal docs to the tests
> >
> > Both (1) and (2) are quite simple. Not sure if there is real
> > benefit in (3). If we should tweak qemu tests with this, should
> > be related to using the JSON output itself, to keep examples
> > correct.
>
> IMO (3) is a waste of time.
>
> Regards,
>
> Phil.
>
> > Cheers,
> > Victor
>
signature.asc
Description: PGP signature
Re: [PATCH v1 0/7] Validate and test qapi examples
On Fri, Sep 08, 2023 at 09:51:35AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/9/23 20:17, Victor Toso wrote:
> > Hi,
>
> > >File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line
> > > 118, in parse_examples_of
> > > assert((obj.doc is not None))
> > > ^^^
> > > AssertionError
> > > ninja: build stopped: subcommand failed.
> > >
> > > not sure if that's related to the examples that still need fixing or not ?
> >
> > This is related to the script being fed with data without
> > documentation. In general, asserting should be the right approach
> > because we don't want API without docs but this failure comes
> > from the tests, that is, adding the following diff:
> >
> > diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
> > index c14ed11774..a961c0575d 100644
> > --- a/scripts/qapi/dumpexamples.py
> > +++ b/scripts/qapi/dumpexamples.py
> > @@ -115,6 +115,10 @@ def parse_examples_of(self:
> > QAPISchemaGenExamplesVisitor,
> >
> > assert(name in self.schema._entity_dict)
> > obj = self.schema._entity_dict[name]
> > +if obj.doc is None:
> > +print(f"{name} does not have documentation")
> > +return
> > +
> > assert((obj.doc is not None))
> > module_name = obj._module.name
> >
> > gives:
> >
> > user-def-cmd0 does not have documentation
> > user-def-cmd does not have documentation
> [...]
>
> > So, not sure if we should:
> > 1. Avoid asserting when running with tests
>
> This seems the most sensible option, adding an argument to
> the 'command' invoked by meson's test_qapi_files() target in
> tests/meson.build.
>
> > 2. Avoid running this generator with tests
> > 3. Add some minimal docs to the tests
> >
> > Both (1) and (2) are quite simple. Not sure if there is real
> > benefit in (3). If we should tweak qemu tests with this, should
> > be related to using the JSON output itself, to keep examples
> > correct.
>
> IMO (3) is a waste of time.
Agreed, I think we shouuld just skip the docs check as this is not
loading a real QAPI schema, just a dummy test one.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v1 0/7] Validate and test qapi examples
On 7/9/23 20:17, Victor Toso wrote:
Hi,
File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118,
in parse_examples_of
assert((obj.doc is not None))
^^^
AssertionError
ninja: build stopped: subcommand failed.
not sure if that's related to the examples that still need fixing or not ?
This is related to the script being fed with data without
documentation. In general, asserting should be the right approach
because we don't want API without docs but this failure comes
from the tests, that is, adding the following diff:
diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
index c14ed11774..a961c0575d 100644
--- a/scripts/qapi/dumpexamples.py
+++ b/scripts/qapi/dumpexamples.py
@@ -115,6 +115,10 @@ def parse_examples_of(self:
QAPISchemaGenExamplesVisitor,
assert(name in self.schema._entity_dict)
obj = self.schema._entity_dict[name]
+if obj.doc is None:
+print(f"{name} does not have documentation")
+return
+
assert((obj.doc is not None))
module_name = obj._module.name
gives:
user-def-cmd0 does not have documentation
user-def-cmd does not have documentation
[...]
So, not sure if we should:
1. Avoid asserting when running with tests
This seems the most sensible option, adding an argument to
the 'command' invoked by meson's test_qapi_files() target in
tests/meson.build.
2. Avoid running this generator with tests
3. Add some minimal docs to the tests
Both (1) and (2) are quite simple. Not sure if there is real
benefit in (3). If we should tweak qemu tests with this, should
be related to using the JSON output itself, to keep examples
correct.
IMO (3) is a waste of time.
Regards,
Phil.
Cheers,
Victor
Re: [PATCH v1 0/7] Validate and test qapi examples
Hi,
Thanks for the quick review Daniel!
On Wed, Sep 06, 2023 at 10:17:04AM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 05, 2023 at 09:48:39PM +0200, Victor Toso wrote:
> > Hi,
> >
> > This is a follow up from the RFC sent in the end of 08-2022:
> > https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04525.html
> >
> > The generator code was rebased, without conflicts. The commit log was
> > improved as per Markus suggestion [0], altough I'm sure it can be
> > improved further.
> >
> > To clarify, consuming the Examples as data for testing the qapi-go
> > work has been very very helpful. I'm positive it can be of use for other
> > bindings in the future, besides keeping the examples functional
> >
> > Cheers,
> >
> > [0] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04682.html
> >
> > Victor Toso (7):
> > qapi: scripts: add a generator for qapi's examples
> > qapi: fix example of get-win32-socket command
> > qapi: fix example of dumpdtb command
> > qapi: fix example of cancel-vcpu-dirty-limit command
> > qapi: fix example of set-vcpu-dirty-limit command
> > qapi: fix example of calc-dirty-rate command
> > qapi: fix example of NETDEV_STREAM_CONNECTED event
> >
> > qapi/machine.json| 2 +-
> > qapi/migration.json | 6 +-
> > qapi/misc.json | 2 +-
> > qapi/net.json| 6 +-
> > scripts/qapi/dumpexamples.py | 194 +++
> > scripts/qapi/main.py | 2 +
> > 6 files changed, 204 insertions(+), 8 deletions(-)
> > create mode 100644 scripts/qapi/dumpexamples.py
>
> After applying this series, aside from the extra broken examples
> mentioned in my patch 1 comments, I also see a test suite failure
> during build
My bad.
> FAILED: tests/qapi-builtin-types.c tests/qapi-builtin-types.h
> tests/qapi-builtin-visit.c tests/qapi-builtin-visit.h
> tests/test-qapi-commands-sub-sub-module.c
> tests/test-qapi-commands-sub-sub-module.h tests/test-qapi-commands.c
> tests/test-qapi-commands.h tests/test-qapi-emit-events.c
> tests/test-qapi-emit-events.h tests/test-qapi-events-sub-sub-module.c
> tests/test-qapi-events-sub-sub-module.h tests/test-qapi-events.c
> tests/test-qapi-events.h tests/test-qapi-init-commands.c
> tests/test-qapi-init-commands.h tests/test-qapi-introspect.c
> tests/test-qapi-introspect.h tests/test-qapi-types-sub-sub-module.c
> tests/test-qapi-types-sub-sub-module.h tests/test-qapi-types.c
> tests/test-qapi-types.h tests/test-qapi-visit-sub-sub-module.c
> tests/test-qapi-visit-sub-sub-module.h tests/test-qapi-visit.c
> tests/test-qapi-visit.h
> /home/berrange/src/virt/qemu/build/pyvenv/bin/python3
> /home/berrange/src/virt/qemu/scripts/qapi-gen.py -o
> /home/berrange/src/virt/qemu/build/tests -b -p test-
> ../tests/qapi-schema/qapi-schema-test.json --suppress-tracing
> Traceback (most recent call last):
> File "/home/berrange/src/virt/qemu/scripts/qapi-gen.py", line 19, in
>
> sys.exit(main.main())
> ^^^
> File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 96, in main
> generate(args.schema,
> File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 58, in
> generate
> gen_examples(schema, output_dir, prefix)
> File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 40,
> in gen_examples
> schema.visit(vis)
> File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 1227, in
> visit
> mod.visit(visitor)
> File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 209, in
> visit
> entity.visit(visitor)
> File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 857, in
> visit
> visitor.visit_command(
> File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 184,
> in visit_command
> parse_examples_of(self, name)
> File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118,
> in parse_examples_of
> assert((obj.doc is not None))
> ^^^
> AssertionError
> ninja: build stopped: subcommand failed.
>
> not sure if that's related to the examples that still need fixing or not ?
This is related to the script being fed with data without
documentation. In general, asserting should be the right approach
because we don't want API without docs but this failure comes
from the tests, that is, adding the following diff:
diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
index c14ed11774..a961c0575d 100644
--- a/scripts/qapi/dumpexamples.py
+++ b/scripts/qapi/dumpexamples.py
@@ -115,6 +115,10 @@ def parse_examples_of(self:
QAPISchemaGenExamplesVisitor,
assert(name in self.schema._entity_dict)
obj = self.schema._entity_dict[name]
+if obj.doc is None:
+print(f"{name} does not have documentation")
+return
+
assert((obj.doc is not None))
module_name = obj._module.name
gives:
user-def-cmd0 does not have
Re: [PATCH v1 0/7] Validate and test qapi examples
On Tue, Sep 05, 2023 at 09:48:39PM +0200, Victor Toso wrote: > Hi, > > This is a follow up from the RFC sent in the end of 08-2022: > https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04525.html > > The generator code was rebased, without conflicts. The commit log was > improved as per Markus suggestion [0], altough I'm sure it can be > improved further. > > To clarify, consuming the Examples as data for testing the qapi-go > work has been very very helpful. I'm positive it can be of use for other > bindings in the future, besides keeping the examples functional > > Cheers, > > [0] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04682.html > > Victor Toso (7): > qapi: scripts: add a generator for qapi's examples > qapi: fix example of get-win32-socket command > qapi: fix example of dumpdtb command > qapi: fix example of cancel-vcpu-dirty-limit command > qapi: fix example of set-vcpu-dirty-limit command > qapi: fix example of calc-dirty-rate command > qapi: fix example of NETDEV_STREAM_CONNECTED event > > qapi/machine.json| 2 +- > qapi/migration.json | 6 +- > qapi/misc.json | 2 +- > qapi/net.json| 6 +- > scripts/qapi/dumpexamples.py | 194 +++ > scripts/qapi/main.py | 2 + > 6 files changed, 204 insertions(+), 8 deletions(-) > create mode 100644 scripts/qapi/dumpexamples.py After applying this series, aside from the extra broken examples mentioned in my patch 1 comments, I also see a test suite failure during build FAILED: tests/qapi-builtin-types.c tests/qapi-builtin-types.h tests/qapi-builtin-visit.c tests/qapi-builtin-visit.h tests/test-qapi-commands-sub-sub-module.c tests/test-qapi-commands-sub-sub-module.h tests/test-qapi-commands.c tests/test-qapi-commands.h tests/test-qapi-emit-events.c tests/test-qapi-emit-events.h tests/test-qapi-events-sub-sub-module.c tests/test-qapi-events-sub-sub-module.h tests/test-qapi-events.c tests/test-qapi-events.h tests/test-qapi-init-commands.c tests/test-qapi-init-commands.h tests/test-qapi-introspect.c tests/test-qapi-introspect.h tests/test-qapi-types-sub-sub-module.c tests/test-qapi-types-sub-sub-module.h tests/test-qapi-types.c tests/test-qapi-types.h tests/test-qapi-visit-sub-sub-module.c tests/test-qapi-visit-sub-sub-module.h tests/test-qapi-visit.c tests/test-qapi-visit.h /home/berrange/src/virt/qemu/build/pyvenv/bin/python3 /home/berrange/src/virt/qemu/scripts/qapi-gen.py -o /home/berrange/src/virt/qemu/build/tests -b -p test- ../tests/qapi-schema/qapi-schema-test.json --suppress-tracing Traceback (most recent call last): File "/home/berrange/src/virt/qemu/scripts/qapi-gen.py", line 19, in sys.exit(main.main()) ^^^ File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 96, in main generate(args.schema, File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 58, in generate gen_examples(schema, output_dir, prefix) File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 40, in gen_examples schema.visit(vis) File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 1227, in visit mod.visit(visitor) File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 209, in visit entity.visit(visitor) File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 857, in visit visitor.visit_command( File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 184, in visit_command parse_examples_of(self, name) File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118, in parse_examples_of assert((obj.doc is not None)) ^^^ AssertionError ninja: build stopped: subcommand failed. not sure if that's related to the examples that still need fixing or not ? With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH v1 0/7] Validate and test qapi examples
Hi, This is a follow up from the RFC sent in the end of 08-2022: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04525.html The generator code was rebased, without conflicts. The commit log was improved as per Markus suggestion [0], altough I'm sure it can be improved further. To clarify, consuming the Examples as data for testing the qapi-go work has been very very helpful. I'm positive it can be of use for other bindings in the future, besides keeping the examples functional. Cheers, [0] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04682.html Victor Toso (7): qapi: scripts: add a generator for qapi's examples qapi: fix example of get-win32-socket command qapi: fix example of dumpdtb command qapi: fix example of cancel-vcpu-dirty-limit command qapi: fix example of set-vcpu-dirty-limit command qapi: fix example of calc-dirty-rate command qapi: fix example of NETDEV_STREAM_CONNECTED event qapi/machine.json| 2 +- qapi/migration.json | 6 +- qapi/misc.json | 2 +- qapi/net.json| 6 +- scripts/qapi/dumpexamples.py | 194 +++ scripts/qapi/main.py | 2 + 6 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 scripts/qapi/dumpexamples.py -- 2.41.0
