Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
On 6/11/21 10:40 AM, Markus Armbruster wrote:
John Snow writes:
On 5/21/21 1:35 AM, Markus Armbruster wrote:
Does not fire for qga/qapi-schema.json. Can you help?
Odd.
I did:
if self._section:
...
else:
raise QAPIWhicheverErrorItWas(...)
and then did a full build and found it to fail on QGA stuff. You may
need --enable-docs to make it happen.
It later failed on test cases, too.
PEBKAC: I tested with a --disable-docs tree. Disabled, because the
conversion to reST restored the "touch anything, rebuild everything" for
docs, which slows me down too much when I mess with the schema.
This snippet triggers the error:
##
# @GuestExec:
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
This one doesn't:
##
# @GuestExec:
#
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
The code dealing with sections is pretty impenetrable.
Yeah, that's what I thought too. I might need (or want?) to touch this
soon to do the cross-reference Sphinx stuff, so I figured I'd be coming
back here "soon".
I could make a gitlab issue for me to track to remind myself to come
back to it if you think that's acceptable.
--js
Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
John Snow writes:
> On 5/21/21 1:35 AM, Markus Armbruster wrote:
>> Does not fire for qga/qapi-schema.json. Can you help?
>
> Odd.
>
> I did:
>
> if self._section:
> ...
> else:
> raise QAPIWhicheverErrorItWas(...)
>
> and then did a full build and found it to fail on QGA stuff. You may
> need --enable-docs to make it happen.
>
> It later failed on test cases, too.
PEBKAC: I tested with a --disable-docs tree. Disabled, because the
conversion to reST restored the "touch anything, rebuild everything" for
docs, which slows me down too much when I mess with the schema.
This snippet triggers the error:
##
# @GuestExec:
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
This one doesn't:
##
# @GuestExec:
#
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
The code dealing with sections is pretty impenetrable.
Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
On 5/21/21 1:35 AM, Markus Armbruster wrote: Does not fire for qga/qapi-schema.json. Can you help? Odd. I did: if self._section: ... else: raise QAPIWhicheverErrorItWas(...) and then did a full build and found it to fail on QGA stuff. You may need --enable-docs to make it happen. It later failed on test cases, too. --js
Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
John Snow writes:
> On 5/20/21 10:42 AM, Markus Armbruster wrote:
>> First step is to find out how _end_section() can be called twice in a
>> row. It isn't in all of "make check". Hmm.
>
> Ah, maybe not twice in a *row*. It does seem to be called when we have
> an "empty section" sometimes, which arises from stuff like this:
>
> Extension error:
> /home/jsnow/src/qemu/docs/../qga/qapi-schema.json:1143:1: ending a
> totally empty section
>
> ##
> # @GuestExec:
> # @pid: pid of child process in guest OS
> #
> # Since: 2.5
> ##
> { 'struct': 'GuestExec',
> 'data': { 'pid': 'int'} }
>
> Without the newline there, it seems to get confused. There's a few
> like this that could be fixed, but then some of the test cases break
> too.
I still can't see it. I tried the obvious
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f03ba2cfec..263aeb5fc5 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -716,6 +716,7 @@ def _start_section(self, name=None, indent=0):
self.sections.append(self._section)
def _end_section(self):
+assert self._section
if self._section:
text = self._section.text = self._section.text.strip()
if self._section.name and (not text or text.isspace()):
Does not fire for qga/qapi-schema.json. Can you help?
> No appetite for barking up this tree right now.
>
> Can I fix the commit message and leave the patch in place? Maybe with
> a #FIXME comment nearby?
I'd like to understand your analysis before I answer your question.
Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
On 5/20/21 10:42 AM, Markus Armbruster wrote:
First step is to find out how _end_section() can be called twice in a
row. It isn't in all of "make check". Hmm.
Ah, maybe not twice in a *row*. It does seem to be called when we have
an "empty section" sometimes, which arises from stuff like this:
Extension error:
/home/jsnow/src/qemu/docs/../qga/qapi-schema.json:1143:1: ending a
totally empty section
##
# @GuestExec:
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
Without the newline there, it seems to get confused. There's a few like
this that could be fixed, but then some of the test cases break too.
No appetite for barking up this tree right now.
Can I fix the commit message and leave the patch in place? Maybe with a
#FIXME comment nearby?
--js
Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
John Snow writes: > It simplifies the typing to say that _section is always a > QAPIDoc.Section(). > > To accommodate this change, we must allow for this object to evaluate to > False for functions like _end_section which behave differently based on > whether or not a Section has been started. > > Signed-off-by: John Snow > > --- > > Probably a better fix is to restructure the code to prevent > _end_section() from being called twice in a row, but that seemed like > more effort, but if you have suggestions for a tactical fix, I'm open to > it. First step is to find out how _end_section() can be called twice in a row. It isn't in all of "make check". Hmm.
[PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
It simplifies the typing to say that _section is always a QAPIDoc.Section(). To accommodate this change, we must allow for this object to evaluate to False for functions like _end_section which behave differently based on whether or not a Section has been started. Signed-off-by: John Snow --- Probably a better fix is to restructure the code to prevent _end_section() from being called twice in a row, but that seemed like more effort, but if you have suggestions for a tactical fix, I'm open to it. Signed-off-by: John Snow --- scripts/qapi/parser.py | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index b3a468504fc..71e982bff57 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0): # the expected indent level of the text of this section self._indent = indent +def __bool__(self) -> bool: +return bool(self.name or self.text) + def append(self, line): # Strip leading spaces corresponding to the expected indent level # Blank lines are always OK. @@ -722,7 +725,7 @@ def _end_section(self): raise QAPIParseError( self._parser, "empty doc section '%s'" % self._section.name) -self._section = None +self._section = QAPIDoc.Section(self._parser) def _append_freeform(self, line): match = re.match(r'(@\S+:)', line) -- 2.30.2
