[PATCH v2 00/34] Add 'version' to other exported types

Tue Apr 19 13:49:56 UTC 2022

On Tue, Apr 19, 2022 at 15:23:07 +0200, Victor Toso wrote:
> Hi,
> 
> On Tue, Apr 19, 2022 at 02:31:01PM +0200, Peter Krempa wrote:
> > On Thu, Apr 14, 2022 at 22:47:11 +0200, Victor Toso wrote:
> > > Hi,
> > 
> > Couple of comments which I summarize on the top level rather than doing
> > specifics since they might need a reorg of the series:
> > 
> > > 
> > > The goal of this patch series is to provide 'since' version to all
> > > exported types.
> > > 
> > > This is the non lazy version of the v1. In this series, we do
> > > change the docstrings of all exported types, to add the version
> > > metadata, in order to have scripts/apibuild.py to fetch it and add
> > > it to the appropriated XML API. This patch series also enforces
> > > that every new exported types requires a docstring with version
> > > with the proper format.
> > > 
> > >   v1: https://listman.redhat.com/archives/libvir-list/2022-April/229881.html
> > > 
> > > I've created a script that helped me out and it covered a good
> > > amound of cases. I hand fixes all the missing docstrings and
> > > corner cases that my script missed.
> > > 
> > > As mentioned in v1, I've used:
> > > 
> > >     git grep -rq $symbol $tag $includedir
> > > 
> > > This is used to find if a given $symbol exists in a given $tag or
> > > not. $tag starts with v1.0.0 and I have ignored any $tag that does
> > > not start with 'v' or has '-rc' in the name.
> > > 
> > > To help review this not so small changeset, the changes were split
> > > in the following order:
> > > 
> > >  * docs: generated: 98% work from the script. I've split it
> > >                     further, by module + group type.
> > > 
> > >  * docs: manual: -> 30% manual labor, 70% vim's macro. It was also
> > >                     split where it seems reasonable.
> > > 
> > >  * docs: (...) -> Some fixes needed in the docs.
> > > 
> > >  * scripts: -> Improvements to apibuild script.
> > > 
> > >  * syms: -> Some fixes found with the found mismatch between
> > >             docstring and sym files.
> > > 
> > > Other than that, I only caught two false positives, that is, a $symbol
> > > was present in a $tag but it was exported only at a latter $tag.
> > > 
> > > Branch  : https://gitlab.com/victortoso/libvirt/-/commits/add-since-version
> > > Green CI: https://gitlab.com/victortoso/libvirt/-/pipelines/517262280
> > > 
> > > Cheers,
> > > Victor
> > > 
> > > Victor Toso (34):
> > >   docs: Fix generated documentation of virConnectListAllNodeDeviceFlags
> > >   docs: variable: Move docstring from source to header file
> > >   docs: generated: enums: libvirt: append 'Since version' metadata
> > 
> > The tree fails to build after this commit. Per our guidelines the tree
> > must be buildable after every single commit:
> > 
> > https://www.libvirt.org/hacking.html#preparing-patches
> > 
> > This is to ensure bisectability. You'll need to reorganize the
> > fixes you've done after applying the changes automatically to
> > happen before or at the same time to ensure compliance.
> 
> Thank you. I thought I had it covered with a `git rebase -i
> --exec `. I'll double check and fix.
> 
> > 
> > >   docs: generated: enums: qemu: append 'Since version' metadata
> > >   docs: generated: enums: admin: append 'Since version' metadata
> > >   docs: generated: macros: libvirt: append 'Since version' metadata
> > >   docs: generated: macros: admin: append 'Since version' metadata
> > >   docs: generated: typedefs: libvirt: append 'Since version' metadata
> > >   docs: generated: typedefs: qemu: append 'Since version' metadata
> > >   docs: generated: typedefs: admin: append 'Since version' metadata
> > >   docs: generated: functions: libvirt: append 'Since version' metadata
> > >   docs: generated: functions: qemu: append 'Since version' metadata
> > >   docs: generated: functions: lxc: append 'Since version' metadata
> > >   docs: generated: functions: admin: append 'Since version' metadata
> > >   docs: manual: typedef: add docstring and Since metadata
> > >   docs: manual: functions: add Since metadata
> > >   docs: manual: enums: add docstring and Since metadata
> > >   docs: manual: macros: add docstring and Since metadata
> > >   docs: manual: libvirt-common: add docstring and Since metadata
> > >   docs: Fix generated documentation of virStorageVolInfoFlags
> > >   docs: Fix and append Since to virConnectListAllStoragePoolsFlags
> > >   docs: Fix and append Since to virDomainDeviceModifyFlags
> > >   docs: Fix and append Since to virDomainMemoryModFlags
> > >   docs: Fix and append Since to virDomainVcpuFlags
> > 
> > None of these actually change anything in the 'docs' directory. I'd
> > suggest to pick an unambiguous prefix such as "symbols" or something
> > similar. 
> 
> Makes sense. Would "docstrings" be preferred over "symbols" ?

Sure

> 
> > >   scripts: apibuild: parse 'Since' version for enums
> > >   scripts: apibuild: fix parsing block comments from typedef enum
> > >   scripts: apibuild: parse 'Since' for typedefs
> > >   scripts: apibuild: parse 'Since' for macros
> > 
> > After the fix mentioned below [1] the build of the tree breaks after
> > this commit:
> > 
> > FAILED: docs/libvirt-api.xml docs/libvirt-lxc-api.xml docs/libvirt-qemu-api.xml docs/libvirt-admin-api.xml
> > /home/pipo/libvirt/scripts/meson-python.sh /bin/python3 /home/pipo/libvirt/scripts/apibuild.py /home/pipo/libvirt/docs /home/pipo/build/libvirt/gcc/docs
> > Function virDomainSetBlockThreshold has symversion 3.1.0 but docstring says 3.2.0
> > Function virAdmClientClose has symversion 2.0.0 but docstring says 1.3.5
> > Function virAdmClientFree has symversion 2.0.0 but docstring says 1.3.5
> > Function virAdmClientGetID has symversion 2.0.0 but docstring says 1.3.5
> 
> Considering your comment bellow, I'll remove the docstring's
> version comparison with what is provided by .syms file.

Actually the check itself might be a good idea, because it gives you one
more possibility to catch the mistake of e.g. exposing a symbol in the
wrong version.

IMO since we are actually not striving on adding any more miss-exported
symbols I'd simply exempt the few wrong examples from the check.

> 
> My only question is, which one should be documented in the XML
> API?

I didn't look closely at the admin API changes, but in case of
'virDomainSetBlockThreshold' the API was really added in 3.2.0 even if
the symbol is exported under the 3.1.0 version, so 3.2.0 is correct in
this case.

Arguably at this point it doesn't matter too much given how old those
releases are.