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

[Markus Armbruster]

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.

> 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?

>> > Maybe we should always look for the least nested include directive to
>> > figure out where the documentation should be placed. Then things
>> > directly referenced by qapi-schema.json would always be on the top
>> > level.
>> >
>> > Possibly we would then want to remove some includes from
>> > qapi-schema.json and include them only from some other file to group
>> > documentation sections that actually make sense to be grouped together.
>> 
>> I doubt implementing this feature would pay back the invested time.
>> Manually controlling the order like I described above is not much of a
>> burden, isn't it?
>
> Depends on whether we are okay with the limitations of the tool
> dictating the order of sections in our documentation.

Fair enough.