Hi,

This series is an extension of what Daniel did in e0e0bf6628 "scripts:
include function versions in API definition".

The main motivation behind this is to help code generators to consider
if a given symbol is present in a given version of libvirt that they are
either running/building against, more specifically:

    https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7

As headers are already a great source of documentation for developers,
I'm suggesting to add a specific comment to each of this exported types:

    /* ... Since <version> */

For the use case I mentioned above, I'm adding small parsing function in
apibuild.py to fetch the above metadata and included it on the generated
XML API.

To avoid adding too much noise in the githistory, I'm proposing the
addition of symbols.versions.allowlist file, that apibuild.py can use to
fetch the first git tag that a given symbol appeared in. I did a small
script to generate it, but it sums up to calling the bellow command for
any tag that starts with 'v' and has not '-rc'.

    git grep $symbol $tag ./include

I'm trying the simples approach I could find but I'm happy to improve
this further based on your suggestions.

Ah, the diff in the generated XMLs, per patch (sadly, pasteben drops it
after a single day). The initial reference is current master 67c77744d7
'tests: Fixing compiler warning in cputest'

 0001 (enums) ... https://paste.centos.org/view/e9137ef0
 0002 (typedefs)  https://paste.centos.org/view/76f0b397
 0003 (macros) .. https://paste.centos.org/view/b68fb03a
 0004 (variables) https://paste.centos.org/view/4a20d9bb

Cheers,
Victor

Victor Toso (4):
  scripts: apibuild: parse 'Since' version for enums
  scripts: apibuild: parse 'Since' for typedefs
  scripts: apibuild: parse 'Since' for macros
  scripts: apibuild: add 'version' to variables

 scripts/apibuild.py        |   68 +-
 symbols.versions.allowlist | 2144 ++++++++++++++++++++++++++++++++++++
 2 files changed, 2199 insertions(+), 13 deletions(-)
 create mode 100644 symbols.versions.allowlist

-- 
2.35.1

On 4/5/22 13:43, Victor Toso wrote:
> Hi,
> 
> This series is an extension of what Daniel did in e0e0bf6628 "scripts:
> include function versions in API definition".
> 
> The main motivation behind this is to help code generators to consider
> if a given symbol is present in a given version of libvirt that they are
> either running/building against, more specifically:
> 
>     https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> 
> As headers are already a great source of documentation for developers,
> I'm suggesting to add a specific comment to each of this exported types:
> 
>     /* ... Since <version> */
> 
> For the use case I mentioned above, I'm adding small parsing function in
> apibuild.py to fetch the above metadata and included it on the generated
> XML API.
> 
> To avoid adding too much noise in the githistory, I'm proposing the
> addition of symbols.versions.allowlist file, that apibuild.py can use to
> fetch the first git tag that a given symbol appeared in. I did a small
> script to generate it, but it sums up to calling the bellow command for
> any tag that starts with 'v' and has not '-rc'.
> 
>     git grep $symbol $tag ./include

This gives you the latest tag that a symbol was touched in. You need to
look at the very first commit that introduced the symbol. For instance,
in your allowlist file you have:

  virConnectDomainEventDeregister v0.10.0

but src/libvirt_public.syms lists this symbol in 0.5.0 release:

LIBVIRT_0.5.0 {
    global:
        ...
        virConnectDomainEventDeregister;

> 
> I'm trying the simples approach I could find but I'm happy to improve
> this further based on your suggestions.
> 
> Ah, the diff in the generated XMLs, per patch (sadly, pasteben drops it
> after a single day). The initial reference is current master 67c77744d7
> 'tests: Fixing compiler warning in cputest'
> 
>  0001 (enums) ... https://paste.centos.org/view/e9137ef0
>  0002 (typedefs)  https://paste.centos.org/view/76f0b397
>  0003 (macros) .. https://paste.centos.org/view/b68fb03a
>  0004 (variables) https://paste.centos.org/view/4a20d9bb
> 
> Cheers,
> Victor
> 
> Victor Toso (4):
>   scripts: apibuild: parse 'Since' version for enums
>   scripts: apibuild: parse 'Since' for typedefs
>   scripts: apibuild: parse 'Since' for macros
>   scripts: apibuild: add 'version' to variables
> 
>  scripts/apibuild.py        |   68 +-
>  symbols.versions.allowlist | 2144 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 2199 insertions(+), 13 deletions(-)
>  create mode 100644 symbols.versions.allowlist
> 

I like this. It's not only for Golang bindings, but other bindings might
benefit from this as well. And also developers reading the docs (they
see immediately what version was the symbol they are looking at
introduced in).

Having said that, maybe we should just add 'Since ...' to every symbol
in include/**\.h instead of having it on a side then? If not, then I
think scripts/ or docs/ is better location for the allowlist file.

Moreover, the allowlist file now contains

Michal

On Tue, Apr 05, 2022 at 04:52:10PM +0200, Michal Prívozník wrote:
> On 4/5/22 13:43, Victor Toso wrote:
> > As headers are already a great source of documentation for developers,
> > I'm suggesting to add a specific comment to each of this exported types:
> >
> >     /* ... Since <version> */
> >
> > For the use case I mentioned above, I'm adding small parsing function in
> > apibuild.py to fetch the above metadata and included it on the generated
> > XML API.
> >
> > To avoid adding too much noise in the githistory, I'm proposing the
> > addition of symbols.versions.allowlist file, that apibuild.py can use to
> > fetch the first git tag that a given symbol appeared in.
>
> I like this. It's not only for Golang bindings, but other bindings might
> benefit from this as well. And also developers reading the docs (they
> see immediately what version was the symbol they are looking at
> introduced in).
>
> Having said that, maybe we should just add 'Since ...' to every symbol
> in include/**\.h instead of having it on a side then? If not, then I
> think scripts/ or docs/ is better location for the allowlist file.

Personally, I think that "since" information should be parsed from
the docstring.

Even the thing that we currently do for functions, where we look into
the .syms file and derive the version number from there, should only
be used as a way to validate the "since" information contained in the
corresponding docstring.

The "allowfile" you've generated could be a first step towards
reaching the goal, but I don't think it should be something that ends
up sticking around.

Adding "Since" tags everywhere by hand would be an insane amount of
work of course, but we should be able to hack together a script that
does something along the lines of

  comment = get_comment_for(sym)
  version = grab_version_from_allowfile(sym)
  replace(comment, f"{comment} (Since {version})")

and get like 90% of the way there. We could then make manual
adjustments to the stuff that looks off.

Ideally, the "since" information would also be included in the HTML
documentation. GLib does that[1] and it's very useful.

To avoid cluttering things too much, we could decide to only show
"since" information for symbols that have been introduced after
1.0.0.

If we wanted to get *really* fancy, we could have some CSS or
JavaScript thingy that allows you to select the libvirt version
you're targeting and hides or marks in some way the symbols that are
newer than that to let you know that you can't use them.

I'm not saying that we should seriously work on that last part, it's
just the kind of thing that you can unlock once you have full version
information for every symbol in the API :)


[1] https://docs.gtk.org/glib/func.aligned_alloc0.html
-- 
Andrea Bolognani / Red Hat / Virtualization

Hi,

On Tue, Apr 05, 2022 at 05:01:54PM +0000, Andrea Bolognani wrote:
> On Tue, Apr 05, 2022 at 04:52:10PM +0200, Michal Prívozník wrote:
> > On 4/5/22 13:43, Victor Toso wrote:
> > > As headers are already a great source of documentation for developers,
> > > I'm suggesting to add a specific comment to each of this exported types:
> > >
> > >     /* ... Since <version> */
> > >
> > > For the use case I mentioned above, I'm adding small parsing function in
> > > apibuild.py to fetch the above metadata and included it on the generated
> > > XML API.
> > >
> > > To avoid adding too much noise in the githistory, I'm proposing the
> > > addition of symbols.versions.allowlist file, that apibuild.py can use to
> > > fetch the first git tag that a given symbol appeared in.
> >
> > I like this. It's not only for Golang bindings, but other bindings might
> > benefit from this as well. And also developers reading the docs (they
> > see immediately what version was the symbol they are looking at
> > introduced in).
> >
> > Having said that, maybe we should just add 'Since ...' to every symbol
> > in include/**\.h instead of having it on a side then? If not, then I
> > think scripts/ or docs/ is better location for the allowlist file.

Sorry, missed this part when first read your reply Michal.

> Personally, I think that "since" information should be parsed
> from the docstring.
> 
> Even the thing that we currently do for functions, where we look into
> the .syms file and derive the version number from there, should only
> be used as a way to validate the "since" information contained in the
> corresponding docstring.
> 
> The "allowfile" you've generated could be a first step towards
> reaching the goal, but I don't think it should be something that ends
> up sticking around.
> 
> Adding "Since" tags everywhere by hand would be an insane amount of
> work of course, but we should be able to hack together a script that
> does something along the lines of
> 
>   comment = get_comment_for(sym)
>   version = grab_version_from_allowfile(sym)
>   replace(comment, f"{comment} (Since {version})")
> 
> and get like 90% of the way there. We could then make manual
> adjustments to the stuff that looks off.

Yes, I don't think it is too hard. IMHO, just playing with
strings at this point. My main concern was the noise it could
generate over git history. I'm the kind of person that does git
blame a lot to get the context of a something I'm reading, so
affecting that is naturally a concern of mine. If it is fine for
you all, I'm 100% okay with that. It'll take a bit longer but not
that much. (famous last words)

> Ideally, the "since" information would also be included in the HTML
> documentation. GLib does that[1] and it's very useful.
>
> To avoid cluttering things too much, we could decide to only
> show "since" information for symbols that have been introduced
> after 1.0.0.

That's 1235 out of 2144. Sounds good to me too.

> If we wanted to get *really* fancy, we could have some CSS or
> JavaScript thingy that allows you to select the libvirt version
> you're targeting and hides or marks in some way the symbols
> that are newer than that to let you know that you can't use
> them.

Where would you like to show that?

> I'm not saying that we should seriously work on that last part,
> it's just the kind of thing that you can unlock once you have
> full version information for every symbol in the API :)

There is always room for improvement, that's for sure. At the
moment, I want to unblock the code generation thing in
libvirt-go-module, somewhat my primary goal. This series is a
part of that quest 0:-)

> [1] https://docs.gtk.org/glib/func.aligned_alloc0.html
> -- 
> Andrea Bolognani / Red Hat / Virtualization

Thanks again for the reviews,
Victor

On Tue, Apr 05, 2022 at 08:24:01PM +0200, Victor Toso wrote:
> On Tue, Apr 05, 2022 at 05:01:54PM +0000, Andrea Bolognani wrote:
> > Adding "Since" tags everywhere by hand would be an insane amount of
> > work of course, but we should be able to hack together a script that
> > does something along the lines of
> >
> >   comment = get_comment_for(sym)
> >   version = grab_version_from_allowfile(sym)
> >   replace(comment, f"{comment} (Since {version})")
> >
> > and get like 90% of the way there. We could then make manual
> > adjustments to the stuff that looks off.
>
> Yes, I don't think it is too hard. IMHO, just playing with
> strings at this point. My main concern was the noise it could
> generate over git history. I'm the kind of person that does git
> blame a lot to get the context of a something I'm reading, so
> affecting that is naturally a concern of mine. If it is fine for
> you all, I'm 100% okay with that. It'll take a bit longer but not
> that much. (famous last words)

A lot of the docstrings are multi-line, in which case simply adding a
line will not affect your ability to git blame at all. It's going to
be more disruptive for enum values, but personally I think it's still
a good trade-off: those are usually added once and never touched
afterwards, so you're just adding a single extra step. Note that I
can't guarantee that other developers feel the same way ;)

> > If we wanted to get *really* fancy, we could have some CSS or
> > JavaScript thingy that allows you to select the libvirt version
> > you're targeting and hides or marks in some way the symbols
> > that are newer than that to let you know that you can't use
> > them.
>
> Where would you like to show that?

I literally didn't think that far :) It was just a pie-in-the-sky
idea. Our focus at the moment should be getting accurate "since"
information into the XML API description and HTML API documentation.

-- 
Andrea Bolognani / Red Hat / Virtualization

Hi,

On Tue, Apr 05, 2022 at 04:52:10PM +0200, Michal Prívozník wrote:
> On 4/5/22 13:43, Victor Toso wrote:
> > Hi,
> > 
> > This series is an extension of what Daniel did in e0e0bf6628 "scripts:
> > include function versions in API definition".
> > 
> > The main motivation behind this is to help code generators to consider
> > if a given symbol is present in a given version of libvirt that they are
> > either running/building against, more specifically:
> > 
> >     https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> > 
> > As headers are already a great source of documentation for developers,
> > I'm suggesting to add a specific comment to each of this exported types:
> > 
> >     /* ... Since <version> */
> > 
> > For the use case I mentioned above, I'm adding small parsing function in
> > apibuild.py to fetch the above metadata and included it on the generated
> > XML API.
> > 
> > To avoid adding too much noise in the githistory, I'm proposing the
> > addition of symbols.versions.allowlist file, that apibuild.py can use to
> > fetch the first git tag that a given symbol appeared in. I did a small
> > script to generate it, but it sums up to calling the bellow command for
> > any tag that starts with 'v' and has not '-rc'.
> > 
> >     git grep $symbol $tag ./include
> 
> This gives you the latest tag that a symbol was touched in. You
> need to look at the very first commit that introduced the
> symbol.

Looking to each commit takes a few hours, that's why I took the
approach at looking per tag instead.

> For instance, in your allowlist file you have:
> 
>   virConnectDomainEventDeregister v0.10.0
> 
> but src/libvirt_public.syms lists this symbol in 0.5.0 release:
> 
> LIBVIRT_0.5.0 {
>     global:
>         ...
>         virConnectDomainEventDeregister;

Good catch! I think the problem here is that when I run:

    git tag --list 'v*' | grep -v 'rc'

the order that I got was

...
    v0.1.8
    v0.1.9
    v0.10.0
    v0.10.1
    v0.10.2
    v0.10.2.1
    v0.10.2.2
    v0.10.2.3
    v0.10.2.4
    v0.10.2.5
    v0.10.2.6
    v0.10.2.7
    v0.10.2.8
    v0.2.0
    v0.2.1
    v0.2.2
...

And I tested in that order, so v0.10.0 was tested prior to
v0.5.0.

I can see that git grep works with v0.5.0. but not with v0.4.6,
as expected.

I'll fix it and send a v2 later. Many thanks!

> > I'm trying the simples approach I could find but I'm happy to improve
> > this further based on your suggestions.
> > 
> > Ah, the diff in the generated XMLs, per patch (sadly, pasteben drops it
> > after a single day). The initial reference is current master 67c77744d7
> > 'tests: Fixing compiler warning in cputest'
> > 
> >  0001 (enums) ... https://paste.centos.org/view/e9137ef0
> >  0002 (typedefs)  https://paste.centos.org/view/76f0b397
> >  0003 (macros) .. https://paste.centos.org/view/b68fb03a
> >  0004 (variables) https://paste.centos.org/view/4a20d9bb
> > 
> > Cheers,
> > Victor
> > 
> > Victor Toso (4):
> >   scripts: apibuild: parse 'Since' version for enums
> >   scripts: apibuild: parse 'Since' for typedefs
> >   scripts: apibuild: parse 'Since' for macros
> >   scripts: apibuild: add 'version' to variables
> > 
> >  scripts/apibuild.py        |   68 +-
> >  symbols.versions.allowlist | 2144 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 2199 insertions(+), 13 deletions(-)
> >  create mode 100644 symbols.versions.allowlist
> > 
> 
> I like this. It's not only for Golang bindings, but other bindings might
> benefit from this as well. And also developers reading the docs (they
> see immediately what version was the symbol they are looking at
> introduced in).
> 
> Having said that, maybe we should just add 'Since ...' to every symbol
> in include/**\.h instead of having it on a side then? If not, then I
> think scripts/ or docs/ is better location for the allowlist file.
> 
> Moreover, the allowlist file now contains
> 
> Michal
>