Message ID | 24aae2ed616a13f9597cc6059f0288ddd91bf359.1573230233.git.anatoly.burakov@intel.com |
---|---|
State | Superseded, archived |
Delegated to: | Thomas Monjalon |
Headers | show |
Series |
|
Related | show |
Context | Check | Description |
---|---|---|
ci/Intel-compilation | success | Compilation OK |
ci/checkpatch | success | coding style OK |
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.
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!
> -----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
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?
+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? >
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