[dpdk-dev,v2,16/17] build: add option to version libs using DPDK version

Message ID 20170912103809.140473-17-bruce.richardson@intel.com
State Superseded, archived
Headers show

Checks

Context Check Description
checkpatch success coding style OK
Intel-compilation success Compilation OK

Commit Message

Bruce Richardson Sept. 12, 2017, 10:38 a.m.
Normally, each library has it's own version number based on the ABI.
Add an option to have all libs just use the DPDK version number as the
.so version.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/meson.build | 8 +++++++-
 lib/meson.build     | 8 +++++++-
 meson_options.txt   | 1 +
 3 files changed, 15 insertions(+), 2 deletions(-)

Comments

Luca Boccassi Sept. 13, 2017, 11:32 a.m.
On Tue, 2017-09-12 at 11:38 +0100, Bruce Richardson wrote:
> Normally, each library has it's own version number based on the ABI.
> Add an option to have all libs just use the DPDK version number as
> the
> .so version.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Reviewed-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  drivers/meson.build | 8 +++++++-
>  lib/meson.build     | 8 +++++++-
>  meson_options.txt   | 1 +
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/meson.build b/drivers/meson.build
> index f19da16fb..0c251bd61 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -92,6 +92,12 @@ foreach class:driver_classes
>  						depends:
> [pmdinfogen, tmp_lib])
>  			endforeach
>  
> +			if get_option('per_library_versions')
> +				so_version = '@0@.1'.format(version)
> +			else
> +				so_version = meson.project_version()
> +			endif
> +
>  			# now build the driver itself, and add to
> the drivers list
>  			lib_name = driver_name_fmt.format(name)
>  			version_map = '@0@/@1@/@2@_version.map'.form
> at(
> @@ -105,7 +111,7 @@ foreach class:driver_classes
>  				c_args: cflags,
>  				link_args: '-Wl,--version-script=' +
> version_map,
>  				link_depends: version_map,
> -				version: '@0@.1'.format(version),
> +				version: so_version,
>  				install: true,
>  				install_dir: driver_install_path)
>  
> diff --git a/lib/meson.build b/lib/meson.build
> index d814721de..36652cfe1 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -76,6 +76,12 @@ foreach l:libraries
>  			dep_objs += [get_variable('dep_rte_' + d)]
>  		endforeach
>  
> +		if get_option('per_library_versions')
> +			so_version = '@0@.1'.format(version)
> +		else
> +			so_version = meson.project_version()
> +		endif
> +
>  		version_map = '@0@/@1@/rte_@2@_version.map'.format(
>  				meson.current_source_dir(),
> dir_name, name)
>  		libname = 'rte_' + name
> @@ -87,7 +93,7 @@ foreach l:libraries
>  				include_directories:
> include_directories(dir_name),
>  				link_args: '-Wl,--version-script=' +
> version_map,
>  				link_depends: version_map,
> -				version: '@0@.1'.format(version),
> +				version: so_version,
>  				install: true)
>  		dep = declare_dependency(link_with: lib,
>  				include_directories:
> include_directories(dir_name),
> diff --git a/meson_options.txt b/meson_options.txt
> index 9c45b8159..636226ce8 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -6,3 +6,4 @@ option('allow_invalid_socket_id', type: 'boolean',
> value: false,
>  	description: 'allow out-of-range NUMA socket id\'s for
> platforms that don\'t report the value correctly')
>  option('enable_kmods', type: 'boolean', value: true, description:
> 'build kernel modules')
>  option('kernel_dir', type: 'string', value: '', description: 'path
> to the kernel for building kernel modules')
> +option('per_library_versions', type: 'boolean', value: true,
> description: 'true: each lib gets its own version number, false: DPDK
> version used for each lib')

Hi Bruce,

First of all thanks for implementing this option, and sorry for the
delay.

A couple of minor things:

1) The project version has 3 digits, but the ABI version should have 2
- eg: should be 17.08 but right now it's 17.08.0 - given point releases
will be ABI compatible so they must not change the ABI version. In the
packaging code we use cut to chop it off:

DPDK_ABI := $(shell echo $(DEB_VERSION_UPSTREAM) | cut -d '.'  -f1-2)

Not sure how to achieve the same in a Meson script though (can it shell
out? or are there string manipulation built-ins?), sorry - just
starting to look at it.

Right now the rte_config option allows the user to completely override
the version - I'm fine with having a single boolean provided this
change is applied, it simplifies packaging a bit, but if it's easier
for you to just accepted a version instead of a boolean it's fine as
well, as you prefer

2) When the option is true, it should not generate a symlink with the
ABI "revision", as the first digit is really not the revision, so it
does not make much sense. Worse, it will stop 2 versions being co-
installable (which was the whole point :-) ), as, eg, 17.02, 17.05 and
17.08 will all ship a libfoo.so.17 symlink. I can fix it in the
packaging, but I don't think it should be generated upstream at all
when this option is enabled.

IOW, the following is the current result:

$ ls -l debian/librte-ethdev17.08/usr/lib/x86_64-linux-gnu/
total 72
lrwxrwxrwx 1 lboccass lboccass    24 Sep 13 11:50 librte_ethdev.so.17 -> librte_ethdev.so.17.08.0
-rw-r--r-- 1 lboccass lboccass 71672 Sep 13 11:50 librte_ethdev.so.17.08.0

The expected result would be to create a single file "librte_ethdev.so.17.08" instead.
Bruce Richardson Sept. 13, 2017, 1:11 p.m.
On Wed, Sep 13, 2017 at 12:32:24PM +0100, Luca Boccassi wrote:
> On Tue, 2017-09-12 at 11:38 +0100, Bruce Richardson wrote:
> > Normally, each library has it's own version number based on the ABI.
> > Add an option to have all libs just use the DPDK version number as
> > the
> > .so version.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Reviewed-by: Harry van Haaren <harry.van.haaren@intel.com>
> > ---
> >  drivers/meson.build | 8 +++++++-
> >  lib/meson.build     | 8 +++++++-
> >  meson_options.txt   | 1 +
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/meson.build b/drivers/meson.build
> > index f19da16fb..0c251bd61 100644
> > --- a/drivers/meson.build
> > +++ b/drivers/meson.build
> > @@ -92,6 +92,12 @@ foreach class:driver_classes
> >  						depends:
> > [pmdinfogen, tmp_lib])
> >  			endforeach
> >  
> > +			if get_option('per_library_versions')
> > +				so_version = '@0@.1'.format(version)
> > +			else
> > +				so_version = meson.project_version()
> > +			endif
> > +
> >  			# now build the driver itself, and add to
> > the drivers list
> >  			lib_name = driver_name_fmt.format(name)
> >  			version_map = '@0@/@1@/@2@_version.map'.form
> > at(
> > @@ -105,7 +111,7 @@ foreach class:driver_classes
> >  				c_args: cflags,
> >  				link_args: '-Wl,--version-script=' +
> > version_map,
> >  				link_depends: version_map,
> > -				version: '@0@.1'.format(version),
> > +				version: so_version,
> >  				install: true,
> >  				install_dir: driver_install_path)
> >  
> > diff --git a/lib/meson.build b/lib/meson.build
> > index d814721de..36652cfe1 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -76,6 +76,12 @@ foreach l:libraries
> >  			dep_objs += [get_variable('dep_rte_' + d)]
> >  		endforeach
> >  
> > +		if get_option('per_library_versions')
> > +			so_version = '@0@.1'.format(version)
> > +		else
> > +			so_version = meson.project_version()
> > +		endif
> > +
> >  		version_map = '@0@/@1@/rte_@2@_version.map'.format(
> >  				meson.current_source_dir(),
> > dir_name, name)
> >  		libname = 'rte_' + name
> > @@ -87,7 +93,7 @@ foreach l:libraries
> >  				include_directories:
> > include_directories(dir_name),
> >  				link_args: '-Wl,--version-script=' +
> > version_map,
> >  				link_depends: version_map,
> > -				version: '@0@.1'.format(version),
> > +				version: so_version,
> >  				install: true)
> >  		dep = declare_dependency(link_with: lib,
> >  				include_directories:
> > include_directories(dir_name),
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 9c45b8159..636226ce8 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -6,3 +6,4 @@ option('allow_invalid_socket_id', type: 'boolean',
> > value: false,
> >  	description: 'allow out-of-range NUMA socket id\'s for
> > platforms that don\'t report the value correctly')
> >  option('enable_kmods', type: 'boolean', value: true, description:
> > 'build kernel modules')
> >  option('kernel_dir', type: 'string', value: '', description: 'path
> > to the kernel for building kernel modules')
> > +option('per_library_versions', type: 'boolean', value: true,
> > description: 'true: each lib gets its own version number, false: DPDK
> > version used for each lib')
> 
> Hi Bruce,
> 
> First of all thanks for implementing this option, and sorry for the
> delay.
> 
> A couple of minor things:
> 
> 1) The project version has 3 digits, but the ABI version should have 2
> - eg: should be 17.08 but right now it's 17.08.0 - given point releases
> will be ABI compatible so they must not change the ABI version. In the
> packaging code we use cut to chop it off:
> 
> DPDK_ABI := $(shell echo $(DEB_VERSION_UPSTREAM) | cut -d '.'  -f1-2)
> 
> Not sure how to achieve the same in a Meson script though (can it shell
> out? or are there string manipulation built-ins?), sorry - just
> starting to look at it.
> 
That should be easy enough to do. It's 3-digits because I'm just pulling
the project version unmodified from the initial project definition. I'd
say I can manage to strip off one digit as part of the build.

> Right now the rte_config option allows the user to completely override
> the version - I'm fine with having a single boolean provided this
> change is applied, it simplifies packaging a bit, but if it's easier
> for you to just accepted a version instead of a boolean it's fine as
> well, as you prefer

If there is a case where we might want to provide a particular version,
I'm happy to edit the option, but if not I'll keep it as-is, because it
will ensure all users of the option get the exact same behaviour, and
means I only have two possibilities to test.

> 
> 2) When the option is true, it should not generate a symlink with the
> ABI "revision", as the first digit is really not the revision, so it
> does not make much sense. Worse, it will stop 2 versions being co-
> installable (which was the whole point :-) ), as, eg, 17.02, 17.05 and
> 17.08 will all ship a libfoo.so.17 symlink. I can fix it in the
> packaging, but I don't think it should be generated upstream at all
> when this option is enabled.

Not exactly sure how to fix this, but I think it may work by using
soversion as well as version in the library() calls.

> 
> IOW, the following is the current result:
> 
> $ ls -l debian/librte-ethdev17.08/usr/lib/x86_64-linux-gnu/
> total 72
> lrwxrwxrwx 1 lboccass lboccass    24 Sep 13 11:50 librte_ethdev.so.17 -> librte_ethdev.so.17.08.0
> -rw-r--r-- 1 lboccass lboccass 71672 Sep 13 11:50 librte_ethdev.so.17.08.0
> 
> The expected result would be to create a single file "librte_ethdev.so.17.08" instead.
> 

Let me see what I can do :-)

/Bruce
Luca Boccassi Sept. 13, 2017, 5:02 p.m.
On Wed, 2017-09-13 at 14:11 +0100, Bruce Richardson wrote:
> On Wed, Sep 13, 2017 at 12:32:24PM +0100, Luca Boccassi wrote:
> > On Tue, 2017-09-12 at 11:38 +0100, Bruce Richardson wrote:
> > > Normally, each library has it's own version number based on the
> > > ABI.
> > > Add an option to have all libs just use the DPDK version number
> > > as
> > > the
> > > .so version.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Reviewed-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > ---
> > >  drivers/meson.build | 8 +++++++-
> > >  lib/meson.build     | 8 +++++++-
> > >  meson_options.txt   | 1 +
> > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/meson.build b/drivers/meson.build
> > > index f19da16fb..0c251bd61 100644
> > > --- a/drivers/meson.build
> > > +++ b/drivers/meson.build
> > > @@ -92,6 +92,12 @@ foreach class:driver_classes
> > >  						depends:
> > > [pmdinfogen, tmp_lib])
> > >  			endforeach
> > >  
> > > +			if get_option('per_library_versions')
> > > +				so_version = '@0@.1'.format(vers
> > > ion)
> > > +			else
> > > +				so_version =
> > > meson.project_version()
> > > +			endif
> > > +
> > >  			# now build the driver itself, and add
> > > to
> > > the drivers list
> > >  			lib_name = driver_name_fmt.format(name)
> > >  			version_map = '@0@/@1@/@2@_version.map'.
> > > form
> > > at(
> > > @@ -105,7 +111,7 @@ foreach class:driver_classes
> > >  				c_args: cflags,
> > >  				link_args: '-Wl,--version-
> > > script=' +
> > > version_map,
> > >  				link_depends: version_map,
> > > -				version: '@0@.1'.format(version)
> > > ,
> > > +				version: so_version,
> > >  				install: true,
> > >  				install_dir:
> > > driver_install_path)
> > >  
> > > diff --git a/lib/meson.build b/lib/meson.build
> > > index d814721de..36652cfe1 100644
> > > --- a/lib/meson.build
> > > +++ b/lib/meson.build
> > > @@ -76,6 +76,12 @@ foreach l:libraries
> > >  			dep_objs += [get_variable('dep_rte_' +
> > > d)]
> > >  		endforeach
> > >  
> > > +		if get_option('per_library_versions')
> > > +			so_version = '@0@.1'.format(version)
> > > +		else
> > > +			so_version = meson.project_version()
> > > +		endif
> > > +
> > >  		version_map = '@0@/@1@/rte_@2@_version.map'.form
> > > at(
> > >  				meson.current_source_dir(),
> > > dir_name, name)
> > >  		libname = 'rte_' + name
> > > @@ -87,7 +93,7 @@ foreach l:libraries
> > >  				include_directories:
> > > include_directories(dir_name),
> > >  				link_args: '-Wl,--version-
> > > script=' +
> > > version_map,
> > >  				link_depends: version_map,
> > > -				version: '@0@.1'.format(version)
> > > ,
> > > +				version: so_version,
> > >  				install: true)
> > >  		dep = declare_dependency(link_with: lib,
> > >  				include_directories:
> > > include_directories(dir_name),
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 9c45b8159..636226ce8 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -6,3 +6,4 @@ option('allow_invalid_socket_id', type:
> > > 'boolean',
> > > value: false,
> > >  	description: 'allow out-of-range NUMA socket id\'s for
> > > platforms that don\'t report the value correctly')
> > >  option('enable_kmods', type: 'boolean', value: true,
> > > description:
> > > 'build kernel modules')
> > >  option('kernel_dir', type: 'string', value: '', description:
> > > 'path
> > > to the kernel for building kernel modules')
> > > +option('per_library_versions', type: 'boolean', value: true,
> > > description: 'true: each lib gets its own version number, false:
> > > DPDK
> > > version used for each lib')
> > 
> > Hi Bruce,
> > 
> > First of all thanks for implementing this option, and sorry for the
> > delay.
> > 
> > A couple of minor things:
> > 
> > 1) The project version has 3 digits, but the ABI version should
> > have 2
> > - eg: should be 17.08 but right now it's 17.08.0 - given point
> > releases
> > will be ABI compatible so they must not change the ABI version. In
> > the
> > packaging code we use cut to chop it off:
> > 
> > DPDK_ABI := $(shell echo $(DEB_VERSION_UPSTREAM) | cut -d '.'  -f1-
> > 2)
> > 
> > Not sure how to achieve the same in a Meson script though (can it
> > shell
> > out? or are there string manipulation built-ins?), sorry - just
> > starting to look at it.
> > 
> 
> That should be easy enough to do. It's 3-digits because I'm just
> pulling
> the project version unmodified from the initial project definition.
> I'd
> say I can manage to strip off one digit as part of the build.
> 
> > Right now the rte_config option allows the user to completely
> > override
> > the version - I'm fine with having a single boolean provided this
> > change is applied, it simplifies packaging a bit, but if it's
> > easier
> > for you to just accepted a version instead of a boolean it's fine
> > as
> > well, as you prefer
> 
> If there is a case where we might want to provide a particular
> version,
> I'm happy to edit the option, but if not I'll keep it as-is, because
> it
> will ensure all users of the option get the exact same behaviour, and
> means I only have two possibilities to test.

I agree, it looks fine as it is right now - I'm not aware of any other
use case at the moment.

> > 
> > 2) When the option is true, it should not generate a symlink with
> > the
> > ABI "revision", as the first digit is really not the revision, so
> > it
> > does not make much sense. Worse, it will stop 2 versions being co-
> > installable (which was the whole point :-) ), as, eg, 17.02, 17.05
> > and
> > 17.08 will all ship a libfoo.so.17 symlink. I can fix it in the
> > packaging, but I don't think it should be generated upstream at all
> > when this option is enabled.
> 
> Not exactly sure how to fix this, but I think it may work by using
> soversion as well as version in the library() calls.
> 
> > 
> > IOW, the following is the current result:
> > 
> > $ ls -l debian/librte-ethdev17.08/usr/lib/x86_64-linux-gnu/
> > total 72
> > lrwxrwxrwx 1 lboccass lboccass    24 Sep 13 11:50
> > librte_ethdev.so.17 -> librte_ethdev.so.17.08.0
> > -rw-r--r-- 1 lboccass lboccass 71672 Sep 13 11:50
> > librte_ethdev.so.17.08.0
> > 
> > The expected result would be to create a single file
> > "librte_ethdev.so.17.08" instead.
> > 
> 
> Let me see what I can do :-)
> 
> /Bruce

Now it works as expected, thanks!

Patch hide | download patch | download mbox

diff --git a/drivers/meson.build b/drivers/meson.build
index f19da16fb..0c251bd61 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -92,6 +92,12 @@  foreach class:driver_classes
 						depends: [pmdinfogen, tmp_lib])
 			endforeach
 
+			if get_option('per_library_versions')
+				so_version = '@0@.1'.format(version)
+			else
+				so_version = meson.project_version()
+			endif
+
 			# now build the driver itself, and add to the drivers list
 			lib_name = driver_name_fmt.format(name)
 			version_map = '@0@/@1@/@2@_version.map'.format(
@@ -105,7 +111,7 @@  foreach class:driver_classes
 				c_args: cflags,
 				link_args: '-Wl,--version-script=' + version_map,
 				link_depends: version_map,
-				version: '@0@.1'.format(version),
+				version: so_version,
 				install: true,
 				install_dir: driver_install_path)
 
diff --git a/lib/meson.build b/lib/meson.build
index d814721de..36652cfe1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -76,6 +76,12 @@  foreach l:libraries
 			dep_objs += [get_variable('dep_rte_' + d)]
 		endforeach
 
+		if get_option('per_library_versions')
+			so_version = '@0@.1'.format(version)
+		else
+			so_version = meson.project_version()
+		endif
+
 		version_map = '@0@/@1@/rte_@2@_version.map'.format(
 				meson.current_source_dir(), dir_name, name)
 		libname = 'rte_' + name
@@ -87,7 +93,7 @@  foreach l:libraries
 				include_directories: include_directories(dir_name),
 				link_args: '-Wl,--version-script=' + version_map,
 				link_depends: version_map,
-				version: '@0@.1'.format(version),
+				version: so_version,
 				install: true)
 		dep = declare_dependency(link_with: lib,
 				include_directories: include_directories(dir_name),
diff --git a/meson_options.txt b/meson_options.txt
index 9c45b8159..636226ce8 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -6,3 +6,4 @@  option('allow_invalid_socket_id', type: 'boolean', value: false,
 	description: 'allow out-of-range NUMA socket id\'s for platforms that don\'t report the value correctly')
 option('enable_kmods', type: 'boolean', value: true, description: 'build kernel modules')
 option('kernel_dir', type: 'string', value: '', description: 'path to the kernel for building kernel modules')
+option('per_library_versions', type: 'boolean', value: true, description: 'true: each lib gets its own version number, false: DPDK version used for each lib')