[libvirt PATCH] scripts: include function versions in API definition

Daniel P. Berrangé posted 1 patch 2 years, 7 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210923104758.2691774-1-berrange@redhat.com
scripts/apibuild.py | 68 +++++++++++++++++++++++++++++++++++++++++----
1 file changed, 62 insertions(+), 6 deletions(-)
[libvirt PATCH] scripts: include function versions in API definition
Posted by Daniel P. Berrangé 2 years, 7 months ago
In order to auto-generate more of the language binding code, it is
desirable to know what libvirt version an API was introduced in.
We can extract this information from the .syms files and expose
it in the API description

eg instead of

  <function name='virNodeNumOfDevices' file='libvirt-nodedev'
            module='libvirt-nodedev'>

we now have

  <function name='virNodeNumOfDevices' file='libvirt-nodedev'
            module='libvirt-nodedev' version='0.5.0'>

This will benefit this proposal:

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

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/apibuild.py | 68 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 6 deletions(-)

diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index 9b29466e1d..bdd3077c48 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2030,8 +2030,9 @@ class CParser:
 
 class docBuilder:
     """A documentation builder"""
-    def __init__(self, name, path='.', directories=['.'], includes=[]):
+    def __init__(self, name, syms, path='.', directories=['.'], includes=[]):
         self.name = name
+        self.syms = syms
         self.path = path
         self.directories = directories
         if name == "libvirt":
@@ -2044,6 +2045,7 @@ class docBuilder:
             self.includes = includes + list(admin_included_files.keys())
         self.modules = {}
         self.headers = {}
+        self.versions = {}
         self.idx = index()
         self.xref = {}
         self.index = {}
@@ -2114,6 +2116,44 @@ class docBuilder:
             self.modules[module] = idx
             self.idx.merge_public(idx)
 
+    def scanVersions(self):
+        prefix = self.name.upper().replace("-", "_") + "_"
+
+        version = None
+        prevversion = None
+        with open(self.syms, "r") as syms:
+            while True:
+                line = syms.readline()
+                if not line:
+                    break
+                line = line.strip()
+                if line.startswith("#"):
+                    continue
+                if line == "":
+                    continue
+
+                if line.startswith(prefix) and line.endswith(" {"):
+                    version = line[len(prefix):-2]
+                elif line == "global:":
+                    continue
+                elif line == "local:":
+                    continue
+                elif line.startswith("}"):
+                    if prevversion is None:
+                        if line != "};":
+                            raise Exception("Unexpected closing version")
+                    else:
+                        if line != ("} %s%s;" % (prefix, prevversion)):
+                            raise Exception("Unexpected end of version '%s': %s'" % (line, "} " + prefix + version))
+
+                    prevversion = version
+                    version = None
+                elif line.endswith(";") and version is not None:
+                    func = line[:-1]
+                    self.versions[func] = version
+                else:
+                    raise Exception("Unexpected line in syms file: %s" % line)
+
     def scan(self):
         for directory in self.directories:
             files = glob.glob(directory + "/*.c")
@@ -2136,6 +2176,7 @@ class docBuilder:
                     self.headers[file] = None
         self.scanHeaders()
         self.scanModules()
+        self.scanVersions()
 
     def modulename_file(self, file):
         module = os.path.basename(file)
@@ -2275,9 +2316,17 @@ class docBuilder:
             print("=>", id)
 
         # NB: this is consumed by a regex in 'getAPIFilenames' in hvsupport.pl
-        output.write("    <%s name='%s' file='%s' module='%s'>\n" % (id.type,
-                     name, self.modulename_file(id.header),
-                     self.modulename_file(id.module)))
+        if id.type == "function":
+            ver = self.versions[name]
+            if ver is None:
+                raise Exception("Missing version for '%s'" % name)
+            output.write("    <function name='%s' file='%s' module='%s' version='%s'>\n" % (
+                name, self.modulename_file(id.header),
+                self.modulename_file(id.module), self.versions[name]))
+        else:
+            output.write("    <functype name='%s' file='%s' module='%s'>\n" % (
+                name, self.modulename_file(id.header),
+                self.modulename_file(id.module)))
         #
         # Processing of conditionals modified by Bill 1/1/05
         #
@@ -2406,9 +2455,16 @@ class app:
         print(msg)
 
     def rebuild(self, name, srcdir, builddir):
-        if name not in ["libvirt", "libvirt-qemu", "libvirt-lxc", "libvirt-admin"]:
+        syms = {
+            "libvirt": srcdir + "/../src/libvirt_public.syms",
+            "libvirt-qemu": srcdir + "/../src/libvirt_qemu.syms",
+            "libvirt-lxc": srcdir + "/../src/libvirt_lxc.syms",
+            "libvirt-admin": srcdir + "/../src/admin/libvirt_admin_public.syms",
+        }
+        if name not in syms:
             self.warning("rebuild() failed, unknown module %s" % name)
             return None
+
         builder = None
         if glob.glob(srcdir + "/../src/libvirt.c") != []:
             if not quiet:
@@ -2418,7 +2474,7 @@ class app:
                     srcdir + "/../src/util",
                     srcdir + "/../include/libvirt",
                     builddir + "/../include/libvirt"]
-            builder = docBuilder(name, builddir, dirs, [])
+            builder = docBuilder(name, syms[name], builddir, dirs, [])
         else:
             self.warning("rebuild() failed, unable to guess the module")
             return None
-- 
2.31.1

Re: [libvirt PATCH] scripts: include function versions in API definition
Posted by Victor Toso 2 years, 7 months ago
Hi,

On Thu, Sep 23, 2021 at 11:47:58AM +0100, Daniel P. Berrangé wrote:
> In order to auto-generate more of the language binding code, it is
> desirable to know what libvirt version an API was introduced in.
> We can extract this information from the .syms files and expose
> it in the API description
> 
> eg instead of
> 
>   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
>             module='libvirt-nodedev'>
> 
> we now have
> 
>   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
>             module='libvirt-nodedev' version='0.5.0'>
> 
> This will benefit this proposal:
> 
>   https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Tested-by: Victor Toso <victortoso@redhat.com>

Thanks, this definitely helps.

Do you think it would make sense to add version metadata to other
types such as structs and enums too?

> ---
>  scripts/apibuild.py | 68 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/apibuild.py b/scripts/apibuild.py
> index 9b29466e1d..bdd3077c48 100755
> --- a/scripts/apibuild.py
> +++ b/scripts/apibuild.py
> @@ -2030,8 +2030,9 @@ class CParser:
>  
>  class docBuilder:
>      """A documentation builder"""
> -    def __init__(self, name, path='.', directories=['.'], includes=[]):
> +    def __init__(self, name, syms, path='.', directories=['.'], includes=[]):
>          self.name = name
> +        self.syms = syms
>          self.path = path
>          self.directories = directories
>          if name == "libvirt":
> @@ -2044,6 +2045,7 @@ class docBuilder:
>              self.includes = includes + list(admin_included_files.keys())
>          self.modules = {}
>          self.headers = {}
> +        self.versions = {}
>          self.idx = index()
>          self.xref = {}
>          self.index = {}
> @@ -2114,6 +2116,44 @@ class docBuilder:
>              self.modules[module] = idx
>              self.idx.merge_public(idx)
>  
> +    def scanVersions(self):
> +        prefix = self.name.upper().replace("-", "_") + "_"
> +
> +        version = None
> +        prevversion = None
> +        with open(self.syms, "r") as syms:
> +            while True:
> +                line = syms.readline()
> +                if not line:
> +                    break
> +                line = line.strip()
> +                if line.startswith("#"):
> +                    continue
> +                if line == "":
> +                    continue
> +
> +                if line.startswith(prefix) and line.endswith(" {"):
> +                    version = line[len(prefix):-2]
> +                elif line == "global:":
> +                    continue
> +                elif line == "local:":
> +                    continue
> +                elif line.startswith("}"):
> +                    if prevversion is None:
> +                        if line != "};":
> +                            raise Exception("Unexpected closing version")
> +                    else:
> +                        if line != ("} %s%s;" % (prefix, prevversion)):
> +                            raise Exception("Unexpected end of version '%s': %s'" % (line, "} " + prefix + version))
> +
> +                    prevversion = version
> +                    version = None
> +                elif line.endswith(";") and version is not None:
> +                    func = line[:-1]
> +                    self.versions[func] = version
> +                else:
> +                    raise Exception("Unexpected line in syms file: %s" % line)
> +
>      def scan(self):
>          for directory in self.directories:
>              files = glob.glob(directory + "/*.c")
> @@ -2136,6 +2176,7 @@ class docBuilder:
>                      self.headers[file] = None
>          self.scanHeaders()
>          self.scanModules()
> +        self.scanVersions()
>  
>      def modulename_file(self, file):
>          module = os.path.basename(file)
> @@ -2275,9 +2316,17 @@ class docBuilder:
>              print("=>", id)
>  
>          # NB: this is consumed by a regex in 'getAPIFilenames' in hvsupport.pl
> -        output.write("    <%s name='%s' file='%s' module='%s'>\n" % (id.type,
> -                     name, self.modulename_file(id.header),
> -                     self.modulename_file(id.module)))
> +        if id.type == "function":
> +            ver = self.versions[name]
> +            if ver is None:
> +                raise Exception("Missing version for '%s'" % name)
> +            output.write("    <function name='%s' file='%s' module='%s' version='%s'>\n" % (
> +                name, self.modulename_file(id.header),
> +                self.modulename_file(id.module), self.versions[name]))
> +        else:
> +            output.write("    <functype name='%s' file='%s' module='%s'>\n" % (
> +                name, self.modulename_file(id.header),
> +                self.modulename_file(id.module)))
>          #
>          # Processing of conditionals modified by Bill 1/1/05
>          #
> @@ -2406,9 +2455,16 @@ class app:
>          print(msg)
>  
>      def rebuild(self, name, srcdir, builddir):
> -        if name not in ["libvirt", "libvirt-qemu", "libvirt-lxc", "libvirt-admin"]:
> +        syms = {
> +            "libvirt": srcdir + "/../src/libvirt_public.syms",
> +            "libvirt-qemu": srcdir + "/../src/libvirt_qemu.syms",
> +            "libvirt-lxc": srcdir + "/../src/libvirt_lxc.syms",
> +            "libvirt-admin": srcdir + "/../src/admin/libvirt_admin_public.syms",
> +        }
> +        if name not in syms:
>              self.warning("rebuild() failed, unknown module %s" % name)
>              return None
> +
>          builder = None
>          if glob.glob(srcdir + "/../src/libvirt.c") != []:
>              if not quiet:
> @@ -2418,7 +2474,7 @@ class app:
>                      srcdir + "/../src/util",
>                      srcdir + "/../include/libvirt",
>                      builddir + "/../include/libvirt"]
> -            builder = docBuilder(name, builddir, dirs, [])
> +            builder = docBuilder(name, syms[name], builddir, dirs, [])
>          else:
>              self.warning("rebuild() failed, unable to guess the module")
>              return None
> -- 
> 2.31.1
> 
Re: [libvirt PATCH] scripts: include function versions in API definition
Posted by Daniel P. Berrangé 2 years, 7 months ago
On Thu, Sep 23, 2021 at 04:07:17PM +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, Sep 23, 2021 at 11:47:58AM +0100, Daniel P. Berrangé wrote:
> > In order to auto-generate more of the language binding code, it is
> > desirable to know what libvirt version an API was introduced in.
> > We can extract this information from the .syms files and expose
> > it in the API description
> > 
> > eg instead of
> > 
> >   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
> >             module='libvirt-nodedev'>
> > 
> > we now have
> > 
> >   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
> >             module='libvirt-nodedev' version='0.5.0'>
> > 
> > This will benefit this proposal:
> > 
> >   https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Tested-by: Victor Toso <victortoso@redhat.com>
> 
> Thanks, this definitely helps.
> 
> Do you think it would make sense to add version metadata to other
> types such as structs and enums too?

We don't have a direct record of versions for these things, so it
is not very practical.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH] scripts: include function versions in API definition
Posted by Victor Toso 2 years, 7 months ago
Hi,

On Thu, Sep 23, 2021 at 03:24:22PM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 23, 2021 at 04:07:17PM +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Thu, Sep 23, 2021 at 11:47:58AM +0100, Daniel P. Berrangé wrote:
> > > In order to auto-generate more of the language binding code, it is
> > > desirable to know what libvirt version an API was introduced in.
> > > We can extract this information from the .syms files and expose
> > > it in the API description
> > > 
> > > eg instead of
> > > 
> > >   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
> > >             module='libvirt-nodedev'>
> > > 
> > > we now have
> > > 
> > >   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
> > >             module='libvirt-nodedev' version='0.5.0'>
> > > 
> > > This will benefit this proposal:
> > > 
> > >   https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > Tested-by: Victor Toso <victortoso@redhat.com>
> > 
> > Thanks, this definitely helps.
> > 
> > Do you think it would make sense to add version metadata to other
> > types such as structs and enums too?
> 
> We don't have a direct record of versions for these things, so it
> is not very practical.

I did a bit of brute-force with `git log -S --source --reverse`
to find the version of everything exported in 7.7.0 API
description:

    https://fedorapeople.org/~victortoso/libvirt/libvirt-v7.7.0.json

While I agree it is not practical to look at the version of each
of those and properly document it in libvirt, we could use the
above as a reference and build some sort of allowlist for types
that are not properly documented and require new types from 7.9.0
onwards to be documented.

Cheers,
Victor
Re: [libvirt PATCH] scripts: include function versions in API definition
Posted by Michal Prívozník 2 years, 7 months ago
On 9/23/21 12:47 PM, Daniel P. Berrangé wrote:
> In order to auto-generate more of the language binding code, it is
> desirable to know what libvirt version an API was introduced in.
> We can extract this information from the .syms files and expose
> it in the API description
> 
> eg instead of
> 
>   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
>             module='libvirt-nodedev'>
> 
> we now have
> 
>   <function name='virNodeNumOfDevices' file='libvirt-nodedev'
>             module='libvirt-nodedev' version='0.5.0'>
> 
> This will benefit this proposal:
> 
>   https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/apibuild.py | 68 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 62 insertions(+), 6 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal