On Thu, Apr 21, 2022 at 01:56:19PM +0200, Peter Krempa wrote:
> On Wed, Apr 20, 2022 at 21:08:16 +0200, Victor Toso wrote:
> > This patch adds 'version' parameter to generated XML API for functions
> > and functypes.
> >
> > The 'version' metadata has been added with e0e0bf6628 by parsing .syms
> > files. This commit does not override that but it will warn if there is
> > not 'Since' metadata with new additions.
> >
> > There is not clear benefit for keeping both. For now, I've added a
> > warning in case there is a mismatch between the version provided by
> > .syms and docstring.
> >
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > ---
> > scripts/apibuild.py | 126 +++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 113 insertions(+), 13 deletions(-)
>
> Let's review the outliers and then the script which generated the rest:
>
> > diff --git a/scripts/apibuild.py b/scripts/apibuild.py
> > index b77eea0624..ec10931151 100755
> > --- a/scripts/apibuild.py
> > +++ b/scripts/apibuild.py
> > @@ -111,6 +111,73 @@ ignored_functions = {
> > "virErrorCopyNew": "private",
> > }
> >
> > +# The version in the .sym file might differnt from
> > +# the real version that the function was introduced.
> > +# This dict's value is the correct version, as it should
> > +# be in the docstrings.
> > +ignored_function_versions = {
> > + 'virDomainSetBlockThreshold': '3.2.0',
>
> We've discussed this one.
>
> > + 'virGetLastErrorMessage': '1.0.5.2',
>
> Your script seems to have picked a stable release which we did at some
> point. The v1.0.6 where the symbol is exported is correct. This
> would be also the only outlier which has a 4 digit version
> string.
>
> So the Since tag needs to be fixed and this removed.
Ack!
> > + 'virNodeDeviceCreate': '0.5.0',
>
> This is an unfortunate side-effect of grepping in the
> repository rather than looking for actual code.
>
> 'virNodeDeviceCreate' was indeed mentioned in a comment in the
> version you've added here, but was in fact implemented at the
> time it was actually exported.
Ack!
> This entry needs to be removed and the Since tag fixed.
>
>
> > + 'virAdmClientClose': '1.3.5',
> > + 'virAdmClientFree': '1.3.5',
> > + 'virAdmClientGetID': '1.3.5',
> > + 'virAdmClientGetInfo': '1.3.5',
> > + 'virAdmClientGetTimestamp': '1.3.5',
> > + 'virAdmClientGetTransport': '1.3.5',
> > + 'virAdmConnectClose': '1.2.17',
> > + 'virAdmConnectGetLibVersion': '1.3.1',
> > + 'virAdmConnectGetURI': '1.3.1',
> > + 'virAdmConnectIsAlive': '1.3.1',
> > + 'virAdmConnectListServers': '1.3.2',
> > + 'virAdmConnectLookupServer': '1.3.3',
> > + 'virAdmConnectOpen': '1.2.17',
> > + 'virAdmConnectRef': '1.2.17',
> > + 'virAdmConnectRegisterCloseCallback': '1.3.1',
> > + 'virAdmConnectUnregisterCloseCallback': '1.3.1',
> > + 'virAdmGetVersion': '1.3.0',
> > + 'virAdmServerFree': '1.3.2',
> > + 'virAdmServerGetClientLimits': '1.3.5',
> > + 'virAdmServerGetName': '1.3.2',
> > + 'virAdmServerGetThreadPoolParameters': '1.3.4',
> > + 'virAdmServerListClients': '1.3.5',
> > + 'virAdmServerLookupClient': '1.3.5',
> > + 'virAdmServerSetClientLimits': '1.3.5',
> > + 'virAdmServerSetThreadPoolParameters': '1.3.4',
>
> All of the above should be removed and the Since tag should say v2.0.0
> to match the symbol as that's the first point at which we've exported
> these.
Ack!
>
> > + 'virAdmServerUpdateTlsFiles': '6.2.0',
>
> This one is needed as it's a mistake in the symbol export.
Ack!
> > + 'virConnectFindStoragePoolSources': '0.4.6',
>
> This is a mistake in our repo. The 'v0.4.5' tag is missing but the
> function was added and exported in time of v0.4.5.
>
> Remove this entry and fix the since tag.
Yes, this was more an issue in my side as I saw no v0.4.5 tag, I
probably adjusted it myself. I'll fix it.
> > + 'virConnectNumOfDefinedDomains': '0.1.6',
> > + 'virConnectOpenAuth': '0.4.1',
>
> Same as above.
>
> > + 'virDomainBlockPeek': '0.4.4',
> > + 'virDomainMemoryPeek': '0.4.4',
>
> So in this case the code was added indeed post v0.4.2 release but the
> symbol was exported in the previous one, although it happened prior to
> v0.4.3 was released, but our repo is missing the tag.
>
> So correctly both should be 0.4.3.
Ack.
>
> > + 'virNetworkUpdate': '1.0.0',
>
> Not sure what happened here, but v0.10.2 what the script
> detects in the symbol seems to be correct.
Likely a leftover from fixing the 1.0.0 versions. I'll fix it.
> > + 'virConnectClose': '0.0.1',
> > + 'virConnectGetType': '0.0.1',
> > + 'virConnectGetVersion': '0.0.1',
> > + 'virConnectListDomains': '0.0.1',
> > + 'virConnectNumOfDomains': '0.0.1',
> > + 'virConnectOpen': '0.0.1',
> > + 'virConnectOpenReadOnly': '0.0.1',
> > + 'virDomainCreateLinux': '0.0.1',
> > + 'virDomainDestroy': '0.0.1',
> > + 'virDomainFree': '0.0.1',
> > + 'virDomainGetID': '0.0.1',
> > + 'virDomainGetInfo': '0.0.1',
> > + 'virDomainGetMaxMemory': '0.0.1',
> > + 'virDomainGetName': '0.0.1',
> > + 'virDomainGetOSType': '0.0.1',
> > + 'virDomainGetXMLDesc': '0.0.1',
> > + 'virDomainLookupByID': '0.0.1',
> > + 'virDomainLookupByName': '0.0.1',
> > + 'virDomainRestore': '0.0.2',
> > + 'virDomainResume': '0.0.1',
> > + 'virDomainSave': '0.0.2',
> > + 'virDomainSetMaxMemory': '0.0.1',
> > + 'virDomainShutdown': '0.0.1',
> > + 'virDomainSuspend': '0.0.1',
> > + 'virGetVersion': '0.0.1',
>
> Now for these you should follow again when the symbol was exported, many
> of these functions are mentioned in TODO first and implemented later.
>
> As noted previously the Since tag should be the maximum version of
> following two:
> 1) when the symbol was exported
> 2) when it was implemented.
>
> In the end I expect this list to be only contain:
>
> 'virDomainSetBlockThreshold'
> 'virAdmServerUpdateTlsFiles'
> 'virDomainBlockPeek'
> 'virDomainMemoryPeek'
Many thanks for the careful review.
I'll have all those fixes by v4.
Cheers,
Victor
Attachment:
signature.asc
Description: PGP signature
