[v7,01/10] config: change ABI versioning to global
diff mbox series

Message ID 24aae2ed616a13f9597cc6059f0288ddd91bf359.1573230233.git.anatoly.burakov@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • Implement the new ABI policy and add helper scripts
Related show

Checks

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

Commit Message

Burakov, Anatoly Nov. 8, 2019, 4:25 p.m. UTC 
From: Marcin Baran <marcinx.baran@intel.com>

As per new ABI policy, all of the libraries are now versioned using
one global ABI version. Changes in this patch implement the
necessary steps to enable that.

Signed-off-by: Marcin Baran <marcinx.baran@intel.com>
Signed-off-by: Pawel Modrak <pawelx.modrak@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---

Notes:
    v6:
    - Silenced grep error message on trying to grep a directory
    
    v3:
    - Removed Windows support from Makefile changes
    - Removed unneeded path conversions from meson files

 buildtools/meson.build |  2 ++
 config/ABI_VERSION     |  1 +
 config/meson.build     |  4 +++-
 drivers/meson.build    | 20 ++++++++++++--------
 lib/meson.build        | 18 ++++++++++++------
 meson_options.txt      |  2 --
 mk/rte.lib.mk          | 13 ++++---------
 7 files changed, 34 insertions(+), 26 deletions(-)
 create mode 100644 config/ABI_VERSION

Comments

Thomas Monjalon Nov. 19, 2019, 1:53 p.m. UTC | #1 
08/11/2019 17:25, Anatoly Burakov:
> From: Marcin Baran <marcinx.baran@intel.com>
> 
> As per new ABI policy, all of the libraries are now versioned using
> one global ABI version. Changes in this patch implement the
> necessary steps to enable that.

For the history, would be nice to describe the "why" of each change here.
Please do not be lazy :)

> --- a/buildtools/meson.build
> +++ b/buildtools/meson.build
> +is_experimental_cmd = [find_program('grep', 'findstr'), '^DPDK_']

A comment is missing to explain the relationship between
"experimental" and "^DPDK_".


> --- /dev/null
> +++ b/config/ABI_VERSION
> +20.0

Why in config/ directory and not in root as for VERSION file?


> +			if is_experimental != 0
> +				lib_version = '0.1'

Why 0.1 and not 0.0?
How do we increment the minor version of experimental libs?

> +				so_version = '0'

How so_version is incremented?
It would deserve a comment here.

>  			if not use_function_versioning
> -				# use pre-build objects to build shared lib
> +				# then use pre-build objects to build shared lib

Is this change relevant?

> -option('per_library_versions', type: 'boolean', value: true,
> -	description: 'true: each lib gets its own version number, false: DPDK version used for each lib')

Good to see this option removed.
Burakov, Anatoly Nov. 19, 2019, 3:48 p.m. UTC | #2 
On 19-Nov-19 1:53 PM, Thomas Monjalon wrote:
> 08/11/2019 17:25, Anatoly Burakov:
>> From: Marcin Baran <marcinx.baran@intel.com>
>>
>> As per new ABI policy, all of the libraries are now versioned using
>> one global ABI version. Changes in this patch implement the
>> necessary steps to enable that.
> 
> For the history, would be nice to describe the "why" of each change here.

Here and other review comments:

I never really touched this script because my meson-foo and ABI-foo is 
non-existent. I would really appreciate some help here on what the 
changes are supposed to do (esp. with regards to ABI versioning 
mechanics), because i understand very little of what this patch does.

> Please do not be lazy :)

Says the guy who waited until last moment to review it!
Kinsella, Ray Nov. 20, 2019, 12:10 p.m. UTC | #3 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Friday 8 November 2019 16:25
> To: dev@dpdk.org
> Cc: Baran, MarcinX <marcinx.baran@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Richardson, Bruce <bruce.richardson@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>; Kinsella, Ray
> <ray.kinsella@intel.com>; david.marchand@redhat.com; Pawel Modrak
> <pawelx.modrak@intel.com>
> Subject: [PATCH v7 01/10] config: change ABI versioning to global
> 
> From: Marcin Baran <marcinx.baran@intel.com>
> 
> As per new ABI policy, all of the libraries are now versioned using one
> global ABI version. Changes in this patch implement the necessary steps
> to enable that.
> 
> Signed-off-by: Marcin Baran <marcinx.baran@intel.com>
> Signed-off-by: Pawel Modrak <pawelx.modrak@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> 
> Notes:
>     v6:
>     - Silenced grep error message on trying to grep a directory
> 
>     v3:
>     - Removed Windows support from Makefile changes
>     - Removed unneeded path conversions from meson files
> 
>  buildtools/meson.build |  2 ++
>  config/ABI_VERSION     |  1 +
>  config/meson.build     |  4 +++-
>  drivers/meson.build    | 20 ++++++++++++--------
>  lib/meson.build        | 18 ++++++++++++------
>  meson_options.txt      |  2 --
>  mk/rte.lib.mk          | 13 ++++---------
>  7 files changed, 34 insertions(+), 26 deletions(-)  create mode 100644
> config/ABI_VERSION
> 
> diff --git a/buildtools/meson.build b/buildtools/meson.build index
> 32c79c1308..78ce69977d 100644
> --- a/buildtools/meson.build
> +++ b/buildtools/meson.build
> @@ -12,3 +12,5 @@ if python3.found()
>  else
>  	map_to_def_cmd = ['meson', 'runpython', files('map_to_def.py')]
> endif
> +
> +is_experimental_cmd = [find_program('grep', 'findstr'), '^DPDK_']
> diff --git a/config/ABI_VERSION b/config/ABI_VERSION new file mode
> 100644 index 0000000000..9a7c1e503f
> --- /dev/null
> +++ b/config/ABI_VERSION
> @@ -0,0 +1 @@
> +20.0
> diff --git a/config/meson.build b/config/meson.build index
> 2b1cb92e7e..30aa2a5313 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -18,6 +18,8 @@ endforeach
>  # depending on the configuration options  pver =
> meson.project_version().split('.')
>  major_version = '@0@.@1@'.format(pver.get(0), pver.get(1))
> +abi_version = run_command(find_program('cat', 'more'),
> +	files('ABI_VERSION')).stdout().strip()
> 
>  # extract all version information into the build configuration
> dpdk_conf.set('RTE_VER_YEAR', pver.get(0).to_int()) @@ -37,7 +39,7 @@
> endif
> 
>  pmd_subdir_opt = get_option('drivers_install_subdir')
>  if pmd_subdir_opt.contains('<VERSION>')
> -	pmd_subdir_opt =
> major_version.join(pmd_subdir_opt.split('<VERSION>'))
> +	pmd_subdir_opt =
> abi_version.join(pmd_subdir_opt.split('<VERSION>'))
>  endif
>  driver_install_path = join_paths(get_option('libdir'), pmd_subdir_opt)
> eal_pmd_path = join_paths(get_option('prefix'), driver_install_path)
> diff --git a/drivers/meson.build b/drivers/meson.build index
> 156d2dc717..b03e0c3159 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -124,12 +124,19 @@ foreach class:dpdk_driver_classes
>  					output: out_filename,
>  					depends: [pmdinfogen, tmp_lib])
> 
> -			if get_option('per_library_versions')
> -				lib_version = '@0@.1'.format(version)
> -				so_version = '@0@'.format(version)
> +			version_map = '@0@/@1@/@2@_version.map'.format(
> +					meson.current_source_dir(),
> +					drv_path, lib_name)
> +
> +			is_experimental = run_command(is_experimental_cmd,
> +				files(version_map)).returncode()
> +
> +			if is_experimental != 0
> +				lib_version = '0.1'
[rk] This all makes sense - except this part.
[rk] I would expect the experimental major version to always be zero ...
[rk] However I would expect the minor version to increment with each new release or at the maintainers discretion. 

> +				so_version = '0'
>  			else
> -				lib_version = major_version
> -				so_version = major_version
> +				lib_version = abi_version
> +				so_version = abi_version
>  			endif
> 
>  			# now build the static driver
> @@ -142,9 +149,6 @@ foreach class:dpdk_driver_classes
>  				install: true)
> 
>  			# now build the shared driver
> -			version_map = '@0@/@1@/@2@_version.map'.format(
> -					meson.current_source_dir(),
> -					drv_path, lib_name)
>  			shared_lib = shared_library(lib_name,
>  				sources,
>  				objects: objs,
> diff --git a/lib/meson.build b/lib/meson.build index
> b2ec9c09a9..3cff2482b1 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -106,12 +106,18 @@ foreach l:libraries
>  				cflags += '-DRTE_USE_FUNCTION_VERSIONING'
>  			endif
> 
> -			if get_option('per_library_versions')
> -				lib_version = '@0@.1'.format(version)
> -				so_version = '@0@'.format(version)
> +			version_map = '@0@/@1@/rte_@2@_version.map'.format(
> +					meson.current_source_dir(), dir_name, name)
> +
> +			is_experimental = run_command(is_experimental_cmd,
> +					files(version_map)).returncode()
> +
> +			if is_experimental != 0
> +				lib_version = '0.1'
> +				so_version = '0'
>  			else
> -				lib_version = major_version
> -				so_version = major_version
> +				lib_version = abi_version
> +				so_version = abi_version
>  			endif
> 
>  			# first build static lib
> @@ -127,7 +133,7 @@ foreach l:libraries
>  					dependencies: static_deps)
> 
>  			if not use_function_versioning
> -				# use pre-build objects to build shared lib
> +				# then use pre-build objects to build shared lib
>  				sources = []
>  				objs += static_lib.extract_all_objects(recursive:
> false)
>  			else
> diff --git a/meson_options.txt b/meson_options.txt index
> 89650b0e9c..da6a7f0302 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -30,8 +30,6 @@ option('max_lcores', type: 'integer', value: 128,
>  	description: 'maximum number of cores/threads supported by EAL')
> option('max_numa_nodes', type: 'integer', value: 4,
>  	description: 'maximum number of NUMA nodes supported by EAL') -
> option('per_library_versions', type: 'boolean', value: true,
> -	description: 'true: each lib gets its own version number, false:
> DPDK version used for each lib')
>  option('tests', type: 'boolean', value: true,
>  	description: 'build unit tests')
>  option('use_hpet', type: 'boolean', value: false, diff --git
> a/mk/rte.lib.mk b/mk/rte.lib.mk index 4df8849a08..b49af9192b 100644
> --- a/mk/rte.lib.mk
> +++ b/mk/rte.lib.mk
> @@ -11,20 +11,15 @@ EXTLIB_BUILD ?= n
>  # VPATH contains at least SRCDIR
>  VPATH += $(SRCDIR)
> 
> -ifneq ($(CONFIG_RTE_MAJOR_ABI),)
> -ifneq ($(LIBABIVER),)
> -LIBABIVER := $(CONFIG_RTE_MAJOR_ABI)
> -endif
> +ifneq ($(shell grep -s "^DPDK_" $(SRCDIR)/$(EXPORT_MAP)),) LIBABIVER
> :=
> +$(shell cat $(RTE_SRCDIR)/config/ABI_VERSION) else LIBABIVER := 0
>  endif
> 
>  ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
>  LIB := $(patsubst %.a,%.so.$(LIBABIVER),$(LIB))  ifeq
> ($(EXTLIB_BUILD),n) -ifeq ($(CONFIG_RTE_MAJOR_ABI),) -ifeq
> ($(CONFIG_RTE_NEXT_ABI),y) -LIB := $(LIB).1 -endif -endif  CPU_LDFLAGS
> += --version-script=$(SRCDIR)/$(EXPORT_MAP)
>  endif
>  endif
> --
> 2.17.1
Thomas Monjalon Nov. 20, 2019, 1:31 p.m. UTC | #4 
20/11/2019 13:10, Kinsella, Ray:
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > --- a/drivers/meson.build
> > +++ b/drivers/meson.build
> > +			if is_experimental != 0
> > +				lib_version = '0.1'
> [rk] This all makes sense - except this part.
> [rk] I would expect the experimental major version to always be zero ...
> [rk] However I would expect the minor version to increment with each new release or at the maintainers discretion. 

Yes, the minor must be incremented with each new release
if we want to allow 2 DPDK versions to be installed in the same system.

This policy must be changed:
"
Experimental libraries always have a major version of 0
to indicate they exist outside of ABI Versioning,
with the minor version incremented with each ABI change to library.
"

I propose to re-use the global ABI version for experimental
by prefixing with "0.".
So for ABI 20.0, it could be 0.20.0 or 0.200? Which one?
Kinsella, Ray Nov. 20, 2019, 2:10 p.m. UTC | #5 
+1 - that's a plan.

Ray K

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday 20 November 2019 13:32
> To: Kinsella, Ray <ray.kinsella@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Cc: dev@dpdk.org; Baran, MarcinX <marcinx.baran@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; david.marchand@redhat.com; Pawel Modrak
> <pawelx.modrak@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [PATCH v7 01/10] config: change ABI versioning to global
> 
> 20/11/2019 13:10, Kinsella, Ray:
> > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > > --- a/drivers/meson.build
> > > +++ b/drivers/meson.build
> > > +			if is_experimental != 0
> > > +				lib_version = '0.1'
> > [rk] This all makes sense - except this part.
> > [rk] I would expect the experimental major version to always be zero
> ...
> > [rk] However I would expect the minor version to increment with each
> new release or at the maintainers discretion.
> 
> Yes, the minor must be incremented with each new release if we want to
> allow 2 DPDK versions to be installed in the same system.
> 
> This policy must be changed:
> "
> Experimental libraries always have a major version of 0 to indicate
> they exist outside of ABI Versioning, with the minor version
> incremented with each ABI change to library.
> "
> 
> I propose to re-use the global ABI version for experimental by
> prefixing with "0.".
> So for ABI 20.0, it could be 0.20.0 or 0.200? Which one?
>

Patch
diff mbox series 

diff --git a/buildtools/meson.build b/buildtools/meson.build
index 32c79c1308..78ce69977d 100644
--- a/buildtools/meson.build
+++ b/buildtools/meson.build
@@ -12,3 +12,5 @@  if python3.found()
 else
 	map_to_def_cmd = ['meson', 'runpython', files('map_to_def.py')]
 endif
+
+is_experimental_cmd = [find_program('grep', 'findstr'), '^DPDK_']
diff --git a/config/ABI_VERSION b/config/ABI_VERSION
new file mode 100644
index 0000000000..9a7c1e503f
--- /dev/null
+++ b/config/ABI_VERSION
@@ -0,0 +1 @@ 
+20.0
diff --git a/config/meson.build b/config/meson.build
index 2b1cb92e7e..30aa2a5313 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -18,6 +18,8 @@  endforeach
 # depending on the configuration options
 pver = meson.project_version().split('.')
 major_version = '@0@.@1@'.format(pver.get(0), pver.get(1))
+abi_version = run_command(find_program('cat', 'more'),
+	files('ABI_VERSION')).stdout().strip()
 
 # extract all version information into the build configuration
 dpdk_conf.set('RTE_VER_YEAR', pver.get(0).to_int())
@@ -37,7 +39,7 @@  endif
 
 pmd_subdir_opt = get_option('drivers_install_subdir')
 if pmd_subdir_opt.contains('<VERSION>')
-	pmd_subdir_opt = major_version.join(pmd_subdir_opt.split('<VERSION>'))
+	pmd_subdir_opt = abi_version.join(pmd_subdir_opt.split('<VERSION>'))
 endif
 driver_install_path = join_paths(get_option('libdir'), pmd_subdir_opt)
 eal_pmd_path = join_paths(get_option('prefix'), driver_install_path)
diff --git a/drivers/meson.build b/drivers/meson.build
index 156d2dc717..b03e0c3159 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -124,12 +124,19 @@  foreach class:dpdk_driver_classes
 					output: out_filename,
 					depends: [pmdinfogen, tmp_lib])
 
-			if get_option('per_library_versions')
-				lib_version = '@0@.1'.format(version)
-				so_version = '@0@'.format(version)
+			version_map = '@0@/@1@/@2@_version.map'.format(
+					meson.current_source_dir(),
+					drv_path, lib_name)
+
+			is_experimental = run_command(is_experimental_cmd,
+				files(version_map)).returncode()
+
+			if is_experimental != 0
+				lib_version = '0.1'
+				so_version = '0'
 			else
-				lib_version = major_version
-				so_version = major_version
+				lib_version = abi_version
+				so_version = abi_version
 			endif
 
 			# now build the static driver
@@ -142,9 +149,6 @@  foreach class:dpdk_driver_classes
 				install: true)
 
 			# now build the shared driver
-			version_map = '@0@/@1@/@2@_version.map'.format(
-					meson.current_source_dir(),
-					drv_path, lib_name)
 			shared_lib = shared_library(lib_name,
 				sources,
 				objects: objs,
diff --git a/lib/meson.build b/lib/meson.build
index b2ec9c09a9..3cff2482b1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -106,12 +106,18 @@  foreach l:libraries
 				cflags += '-DRTE_USE_FUNCTION_VERSIONING'
 			endif
 
-			if get_option('per_library_versions')
-				lib_version = '@0@.1'.format(version)
-				so_version = '@0@'.format(version)
+			version_map = '@0@/@1@/rte_@2@_version.map'.format(
+					meson.current_source_dir(), dir_name, name)
+
+			is_experimental = run_command(is_experimental_cmd,
+					files(version_map)).returncode()
+
+			if is_experimental != 0
+				lib_version = '0.1'
+				so_version = '0'
 			else
-				lib_version = major_version
-				so_version = major_version
+				lib_version = abi_version
+				so_version = abi_version
 			endif
 
 			# first build static lib
@@ -127,7 +133,7 @@  foreach l:libraries
 					dependencies: static_deps)
 
 			if not use_function_versioning
-				# use pre-build objects to build shared lib
+				# then use pre-build objects to build shared lib
 				sources = []
 				objs += static_lib.extract_all_objects(recursive: false)
 			else
diff --git a/meson_options.txt b/meson_options.txt
index 89650b0e9c..da6a7f0302 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -30,8 +30,6 @@  option('max_lcores', type: 'integer', value: 128,
 	description: 'maximum number of cores/threads supported by EAL')
 option('max_numa_nodes', type: 'integer', value: 4,
 	description: 'maximum number of NUMA nodes supported by EAL')
-option('per_library_versions', type: 'boolean', value: true,
-	description: 'true: each lib gets its own version number, false: DPDK version used for each lib')
 option('tests', type: 'boolean', value: true,
 	description: 'build unit tests')
 option('use_hpet', type: 'boolean', value: false,
diff --git a/mk/rte.lib.mk b/mk/rte.lib.mk
index 4df8849a08..b49af9192b 100644
--- a/mk/rte.lib.mk
+++ b/mk/rte.lib.mk
@@ -11,20 +11,15 @@  EXTLIB_BUILD ?= n
 # VPATH contains at least SRCDIR
 VPATH += $(SRCDIR)
 
-ifneq ($(CONFIG_RTE_MAJOR_ABI),)
-ifneq ($(LIBABIVER),)
-LIBABIVER := $(CONFIG_RTE_MAJOR_ABI)
-endif
+ifneq ($(shell grep -s "^DPDK_" $(SRCDIR)/$(EXPORT_MAP)),)
+LIBABIVER := $(shell cat $(RTE_SRCDIR)/config/ABI_VERSION)
+else
+LIBABIVER := 0
 endif
 
 ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
 LIB := $(patsubst %.a,%.so.$(LIBABIVER),$(LIB))
 ifeq ($(EXTLIB_BUILD),n)
-ifeq ($(CONFIG_RTE_MAJOR_ABI),)
-ifeq ($(CONFIG_RTE_NEXT_ABI),y)
-LIB := $(LIB).1
-endif
-endif
 CPU_LDFLAGS += --version-script=$(SRCDIR)/$(EXPORT_MAP)
 endif
 endif