Re: [Qemu-devel] [PATCH v4 2/3] qapi: Add a primitive to include other files from a QAPI schema file

[Markus Armbruster <armbru@redhat.com>]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Adds the "include(...)" primitive to the syntax of QAPI schema files.
>>> 
>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>> ---
>>> docs/qapi-code-gen.txt |    8 ++++++++
>>> scripts/qapi.py        |   36 ++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 42 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>>> index 2e9f036..e007807 100644
>>> --- a/docs/qapi-code-gen.txt
>>> +++ b/docs/qapi-code-gen.txt
>>> @@ -40,6 +40,14 @@ enumeration types and union types.
>>> Generally speaking, types definitions should always use CamelCase
>>> for the type
>>> names. Command names should be all lower case with words separated
>>> by a hyphen.
>>> 
>>> +The QAPI schema definitions can be modularized using the 'include'
>>> directive:
>>> +
>>> + include("sub-system/qapi.json")
>
>> And now it isn't JSON anymore.
>
>> To keep it JSON, use syntax like
>
>>     { "include": "sub-system/qapi.json" }
>
>> If you absolutely must make it non-JSON, you better rename the .json
>> files.
>
>> Hmm, we already are non-JSON, because we use ' instead of " for no sane
>> reason.
>
>> Our JSON parser accepts ' as an extension, to save us quoting in C
>> strings.  That reason doesn't apply to .json files.
>
> Is it a problem if they are not pure JSON? In the end, they are parsed by
> qapi.py (which already knows about file syntax), and having a separate syntax
> for includes makes it somewhat easier to spot when that happens.

I don't particularly care whether schema syntax is pure JSON, some
bastardized variation of JSON, or something else entirely.  But as long
as we advertize schema files it as .json, they better contain JSON.  If
they contain something else, they should be called something else.

>>> +
>>> +All paths are interpreted as relative to the initial input file
>>> passed to the
>>> +QAPI parsing scripts.
>
>> Really?
>
>> Consider foo.json includes lib/a.json, which wants to include
>> lib/b.json.
>
>> foo.json:       include("lib/a.json")
>> lib/a.json:     include("lib/b.json")   # relative to foo.json's directory
>
>> Now throw in bar/bar.json including lib/a.json:
>
>> bar/bar.json:   include("../lib/a.json")
>> lib/a.json:     include("lib/b.json")   # relative to bar/ -> ENOENT
>
>> Make it relative to the file with the include directive.
>
> Right, sorry. The documentation was not updated after removing the explicit
> include directory argument. What I'm not sure though is what would be a better
> option, having current-file-relative includes, or resuscitating the old explicit
> include argument.

Include relative to the including file is simple.  If we needed
not-so-simple semantics, I suspect we'd be better off piping through
cpp.

>>> +
>>> +
>>> === Complex types ===
>>> 
>>> A complex type is a dictionary containing a single key whose value is a
>> [...]
>
>> Are you aware of Wenchao Xia's "[PATCH V8 00/10] qapi script: support
>> enum as discriminator and better enum name"?  I'm afraid there's a
>> (semantic?) conflict.  With include files, "[PATCH V8 03/10] qapi
>> script: remember line number in schema parsing" needs to remember the
>> source file, too.
>
>> Wenchao's series is likely go in first.  Perhaps you want to base on it
>> now.
>
> I was not aware of that. Since I'm managing multiple series on a single branch
> with stgit (and extracting that is somewhat tedious), I will redo this series
> once Xia's is merged upstream. Is the merge going to happen anytime soon?

I reviewed v7, and it looked almost ready to me.  If v8 is ready, it'll
hopefully get merged fairly quickly (days, not weeks).