| Message ID | 20200908093113.47564-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | docs: add qemu-storage-daemon documentation | expand |
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 ;)) Thanks! Laszlo > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 55b58ba892..e986341997 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1,10 +1,6 @@ > # -*- Mode: Python -*- > # vim: filetype=python > > -## > -# == Block core (VM unrelated) > -## > - > { 'include': 'common.json' } > { 'include': 'crypto.json' } > { 'include': 'job.json' } > diff --git a/qapi/block.json b/qapi/block.json > index c54a393cf3..473b294a3b 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -3,6 +3,7 @@ > > ## > # = Block devices > +# == Block core (VM unrelated) > ## > > { 'include': 'block-core.json' } >
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. 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. Kevin
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. Alternatively, add just enough includes to get the order you want. > 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?
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). 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. > Alternatively, add just enough includes to get the order you want. There are orders that I can't get this way. 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". > > 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. Kevin
Am 08.09.2020 um 11:31 hat Stefan Hajnoczi geschrieben: > 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(-) storage-daemon/qapi/qapi-schema.json needs an update, too. With the series as it is, the block-core.json definitions don't get any headline at all and look as if they were part of the previous section. Maybe a nicer solution would be to keep the second-level heading where it is, but to just add a first-level one to the storage daemon qapi-schema.json. It makes sense to group block-core and block-export together even without the system emulator part, so the top-level section wouldn't be arbitrary either, but we would add a second subsection soon. Kevin
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.
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
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? > (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
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
Kevin Wolf <kwolf@redhat.com> writes: > 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: [...] >> >> > 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. You're right. Being unable to talk about something before you define it may not be all bad, though :)
Am 10.09.2020 um 07:18 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > 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: > [...] > >> >> > 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. > > You're right. > > Being unable to talk about something before you define it may not be all > bad, though :) Yes, as long as the resulting order is what we want anyway, this is not a problem. I'm just not sure if we will always be aware of the include structure without tool support so that we would always declare the dependencies first. Nobody checks the headlines in the documentation after making schema changes. Kevin
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' } ## diff --git a/qapi/block-core.json b/qapi/block-core.json index 55b58ba892..e986341997 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1,10 +1,6 @@ # -*- Mode: Python -*- # vim: filetype=python -## -# == Block core (VM unrelated) -## - { 'include': 'common.json' } { 'include': 'crypto.json' } { 'include': 'job.json' } diff --git a/qapi/block.json b/qapi/block.json index c54a393cf3..473b294a3b 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -3,6 +3,7 @@ ## # = Block devices +# == Block core (VM unrelated) ## { 'include': 'block-core.json' }
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(-)