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

[Kevin Wolf <kwolf@redhat.com>]

Am 09.09.2020 um 17:28 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > 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.
> 
> Correct.  The use for controlling doc order is a bit of a hack.
> 
> >> > 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.
> 
> Splitting block-core.json can help only if other modules then use less
> than all the parts.  In particular, as long as block.json includes the
> same stuff, it'll surely still include jobs.json.  Could it include
> less?

Not if the documentation wants to have a single chapter for the block
layer instead of many small block related top-level chapters.

Otherwise we could include some things directly from qapi-schema.json,
but obviously, that would still have to be after job.json for some
parts.

Kevin