| Message ID | 20230905194846.169530-1-victortoso@redhat.com |
|---|---|
| Headers | show |
| Series | Validate and test qapi examples | expand |
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 <module> 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
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 <module> > 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 documentation user-def-cmd does not have documentation user-def-cmd1 does not have documentation user-def-cmd2 does not have documentation cmd-success-response does not have documentation coroutine-cmd does not have documentation guest-get-time does not have documentation guest-sync does not have documentation boxed-struct does not have documentation boxed-union does not have documentation boxed-empty does not have documentation test-flags-command does not have documentation EVENT_A does not have documentation EVENT_B does not have documentation EVENT_C does not have documentation EVENT_D does not have documentation EVENT_E does not have documentation EVENT_F does not have documentation EVENT_G does not have documentation __ORG.QEMU_X-EVENT does not have documentation __org.qemu_x-command does not have documentation test-if-union-cmd does not have documentation test-if-alternate-cmd does not have documentation test-if-cmd does not have documentation test-cmd-return-def-three does not have documentation TEST_IF_EVENT does not have documentation TEST_IF_EVENT2 does not have documentation test-features0 does not have documentation test-command-features1 does not have documentation test-command-features3 does not have documentation test-command-cond-features1 does not have documentation test-command-cond-features2 does not have documentation test-command-cond-features3 does not have documentation TEST_EVENT_FEATURES0 does not have documentation TEST_EVENT_FEATURES1 does not have documentation TEST_EVENT_FEATURES2 does not have documentation So, not sure if we should: 1. Avoid asserting when running with tests 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. Cheers, Victor
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
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
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 >
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