Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 09.09.2020 um 14:10 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> >> Hi Stefan,
> >> >> 
> >> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> >> > block-core.json is included from several places. It has no way of
> >> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> >> >> > errors when it encounters an h2 header where it expects an h1 header.
> >> >> > This issue prevents the next patch from generating documentation for
> >> >> > qemu-storage-daemon QMP commands.
> >> >> > 
> >> >> > Move the header into parents so that the correct header level can be
> >> >> > used. Note that transaction.json is not updated since it doesn't seem 
> >> >> > to
> >> >> > need a header.
> >> >> > 
> >> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> >> > ---
> >> >> >  docs/interop/firmware.json | 4 ++++
> >> >> >  qapi/block-core.json       | 4 ----
> >> >> >  qapi/block.json            | 1 +
> >> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
> >> >> > 
> >> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> >> >> > index 989f10b626..48af327f98 100644
> >> >> > --- a/docs/interop/firmware.json
> >> >> > +++ b/docs/interop/firmware.json
> >> >> > @@ -15,6 +15,10 @@
> >> >> >  ##
> >> >> >  
> >> >> >  { 'include' : 'machine.json' }
> >> >> > +
> >> >> > +##
> >> >> > +# == Block devices
> >> >> > +##
> >> >> >  { 'include' : 'block-core.json' }
> >> >> >  
> >> >> >  ##
> >> >> 
> >> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> >> "transaction.json".
> >> >> 
> >> >> It's been a long time since I last looked at a rendered view of
> >> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
> >> >> it can refer to some block-related types (@BlockdevDriver seems like the
> >> >> main, or only, one).
> >> >> 
> >> >> I wouldn't expect the rendered view of "firmware.json" to have a section
> >> >> header saying "Block devices".
> >> >> 
> >> >> I think it should be fine to drop this hunk (and my CC along with it ;))
> >> >
> >> > I think this is actually a more general problem with the way we generate
> >> > the documentation. For example, the "Background jobs" documentation ends
> >> > up under "Block Devices" just because qapi-schema.json includes
> >> > block-core.json before job.json and block-core.json includes job.json to
> >> > be able to access some types.
> >> 
> >> The doc generator is stupid and greedy (which also makes it
> >> predictable): a module's documentation is emitted where it is first
> >> included.
> >> 
> >> For full control of the order, have the main module include all
> >> sub-modules in the order you want.
> >
> > This works as long as the order that we want is consistent with the
> > requirement that every file must be included by qapi-schea.json before
> > it is included by any other file (essentially making the additional
> > includes in other files useless).
> 
> These other includes are not useless: they are essential for generating
> self-contained headers.
> 
> When MOD.json includes SUBMOD.json, then the generated qapi-FOO-MOD.h
> include qapi-FOO-SUBMOD.h.  When every module pulls in the modules it
> requires, so do the generated headers.  When a module doesn't, its
> generated headers won't compile unless you manually include the missing
> generated headers it requires.

Hm, right. So we're using includes for two different purposes, but just
from looking at the include line, you can't know which one it is.

> > Is this the order that we want?
> >
> > If so, we could support following the rule consistently by making double
> > include of a file an error.
> 
> Breaks our simple & stupid way to generate self-contained headers.
> 
> >> Alternatively, add just enough includes to get the order you want.
> >
> > There are orders that I can't get this way.
> 
> You're right, ordering by first include is not completely general.
> 
> >                                             For example, if I want to
> > have "Block devices" documented before "Background jobs", there is no
> > way to add includes to actually get this order without having
> > "Background jobs" as a subsection in "Block devices".
> 
> "Background jobs" is job.json.
> 
> "Block devices" is block.json, which includes block-core.json, which
> includes job.json.
> 
> To be able to put "Block devices" first, you'd have to break the chain
> from block.json to job.json.  Which might even be an improvement:
> 
> $ wc -l qapi/*.json | awk '$1 >= 1000 { print }'
>   5527 qapi/block-core.json
>   1722 qapi/migration.json
>   1617 qapi/misc.json
>   1180 qapi/ui.json
>  17407 total
> 
> Could we split block-core.json into several cohesive parts?

Possibly. However, while it would be an improvement generally speaking,
how does this change the specific problem? All of the smaller files will
be included by block.json (or whatever file provides the "Block devices"
chapter in the documentation) and at least one of them will still have
to include job.json.

(As it happens, the block export series splits off a block-export QAPI
module, but it's probably small enough to be barely noticable in this
comparison.)

Kevin