[v3] compat: merge compat library into EAL
Checks
Commit Message
Since compat library is only a single header, we can easily move it into
the EAL common headers instead of tracking it separately. The downside of
this is that it becomes a little more difficult to have any libs that are
built before EAL depend on it. Thankfully, this is not a major problem as
the only library which uses rte_compat.h and is built before EAL (kvargs)
already has the path to the compat.h header file explicitly called out as
an include path.
However, to ensure that we don't hit problems later with this, we can add
EAL common headers folder to the global include list in the meson build
which means that all common headers can be safely used by all libraries, no
matter what their build order.
As a side-effect, this patch also fixes an issue with building on BSD using
meson, due to compat lib no longer needing to be listed as a dependency.
Fixes: a8499f65a1d1 ("log: add missing experimental tag")
CC: stable@dpdk.org
CC: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Tested-by: David Marchand <david.marchand@redhat.com>
Tested-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
V3: Add stable on CC, and remove extra word in commit log.
V2: Clean up a few missed references to the compat library in our
documentation and MAINTAINERS file.
Added in fixes tag, as this patch should also fix build issues
with BSD.
---
MAINTAINERS | 1 -
doc/api/doxy-api.conf.in | 1 -
doc/guides/contributing/documentation.rst | 1 -
doc/guides/contributing/versioning.rst | 2 +-
lib/Makefile | 2 --
lib/librte_cmdline/meson.build | 1 -
lib/librte_compat/Makefile | 13 -------------
lib/librte_compat/meson.build | 8 --------
lib/librte_eal/common/Makefile | 2 +-
.../common/include}/rte_compat.h | 0
lib/librte_eal/common/meson.build | 1 +
lib/librte_eal/linuxapp/eal/meson.build | 2 +-
lib/librte_eal/meson.build | 1 -
lib/librte_kvargs/meson.build | 3 ---
lib/meson.build | 2 +-
meson.build | 2 +-
16 files changed, 6 insertions(+), 36 deletions(-)
delete mode 100644 lib/librte_compat/Makefile
delete mode 100644 lib/librte_compat/meson.build
rename lib/{librte_compat => librte_eal/common/include}/rte_compat.h (100%)
Comments
On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> Since compat library is only a single header, we can easily move it into
> the EAL common headers instead of tracking it separately. The downside of
> this is that it becomes a little more difficult to have any libs that are
> built before EAL depend on it. Thankfully, this is not a major problem as
> the only library which uses rte_compat.h and is built before EAL (kvargs)
> already has the path to the compat.h header file explicitly called out as
> an include path.
>
> However, to ensure that we don't hit problems later with this, we can add
> EAL common headers folder to the global include list in the meson build
> which means that all common headers can be safely used by all libraries, no
> matter what their build order.
>
This assumes that the compat lib will always just be a header though, no? Will
this work in the event that someone wants to add some compatibility code that
requires its own C compilation unit?
> As a side-effect, this patch also fixes an issue with building on BSD using
> meson, due to compat lib no longer needing to be listed as a dependency.
>
Can you elaborate here a bit please? listing a lib as a dependency seems like a
fundamental function of a build system, was there a bug with meson in this
capacity?
Thanks
Neil
> Fixes: a8499f65a1d1 ("log: add missing experimental tag")
>
> CC: stable@dpdk.org
> CC: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Tested-by: David Marchand <david.marchand@redhat.com>
> Tested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> V3: Add stable on CC, and remove extra word in commit log.
> V2: Clean up a few missed references to the compat library in our
> documentation and MAINTAINERS file.
> Added in fixes tag, as this patch should also fix build issues
> with BSD.
> ---
> MAINTAINERS | 1 -
> doc/api/doxy-api.conf.in | 1 -
> doc/guides/contributing/documentation.rst | 1 -
> doc/guides/contributing/versioning.rst | 2 +-
> lib/Makefile | 2 --
> lib/librte_cmdline/meson.build | 1 -
> lib/librte_compat/Makefile | 13 -------------
> lib/librte_compat/meson.build | 8 --------
> lib/librte_eal/common/Makefile | 2 +-
> .../common/include}/rte_compat.h | 0
> lib/librte_eal/common/meson.build | 1 +
> lib/librte_eal/linuxapp/eal/meson.build | 2 +-
> lib/librte_eal/meson.build | 1 -
> lib/librte_kvargs/meson.build | 3 ---
> lib/meson.build | 2 +-
> meson.build | 2 +-
> 16 files changed, 6 insertions(+), 36 deletions(-)
> delete mode 100644 lib/librte_compat/Makefile
> delete mode 100644 lib/librte_compat/meson.build
> rename lib/{librte_compat => librte_eal/common/include}/rte_compat.h (100%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 835d8a201..0707caea5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -121,7 +121,6 @@ F: buildtools/symlink-drivers-solibs.sh
>
> ABI versioning
> M: Neil Horman <nhorman@tuxdriver.com>
> -F: lib/librte_compat/
> F: doc/guides/rel_notes/deprecation.rst
> F: devtools/validate-abi.sh
> F: devtools/check-symbol-change.sh
> diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in
> index bef9320c0..a365e669b 100644
> --- a/doc/api/doxy-api.conf.in
> +++ b/doc/api/doxy-api.conf.in
> @@ -23,7 +23,6 @@ INPUT = @TOPDIR@/doc/api/doxy-api-index.md \
> @TOPDIR@/lib/librte_bpf \
> @TOPDIR@/lib/librte_cfgfile \
> @TOPDIR@/lib/librte_cmdline \
> - @TOPDIR@/lib/librte_compat \
> @TOPDIR@/lib/librte_compressdev \
> @TOPDIR@/lib/librte_cryptodev \
> @TOPDIR@/lib/librte_distributor \
> diff --git a/doc/guides/contributing/documentation.rst b/doc/guides/contributing/documentation.rst
> index c72280a29..baf0921fb 100644
> --- a/doc/guides/contributing/documentation.rst
> +++ b/doc/guides/contributing/documentation.rst
> @@ -22,7 +22,6 @@ The main directories that contain files related to documentation are shown below
> |-- librte_acl
> |-- librte_cfgfile
> |-- librte_cmdline
> - |-- librte_compat
> |-- librte_eal
> | |-- ...
> ...
> diff --git a/doc/guides/contributing/versioning.rst b/doc/guides/contributing/versioning.rst
> index 01b36247e..18b031998 100644
> --- a/doc/guides/contributing/versioning.rst
> +++ b/doc/guides/contributing/versioning.rst
> @@ -167,7 +167,7 @@ functionality or behavior. When that occurs, it is desirable to allow for
> backward compatibility for a time with older binaries that are dynamically
> linked to the DPDK.
>
> -To support backward compatibility the ``lib/librte_compat/rte_compat.h``
> +To support backward compatibility the ``rte_compat.h``
> header file provides macros to use when updating exported functions. These
> macros are used in conjunction with the ``rte_<library>_version.map`` file for
> a given library to allow multiple versions of a symbol to exist in a shared
> diff --git a/lib/Makefile b/lib/Makefile
> index d6239d27c..ffbfd0d94 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -3,9 +3,7 @@
>
> include $(RTE_SDK)/mk/rte.vars.mk
>
> -DIRS-y += librte_compat
> DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
> -DEPDIRS-librte_kvargs := librte_compat
> DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
> DEPDIRS-librte_eal := librte_kvargs
> DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci
> diff --git a/lib/librte_cmdline/meson.build b/lib/librte_cmdline/meson.build
> index 30498906c..0fa61385f 100644
> --- a/lib/librte_cmdline/meson.build
> +++ b/lib/librte_cmdline/meson.build
> @@ -3,7 +3,6 @@
>
> # This library is processed before EAL
> includes = [global_inc]
> -includes += include_directories('../librte_eal/common/include')
>
> version = 2
> sources = files('cmdline.c',
> diff --git a/lib/librte_compat/Makefile b/lib/librte_compat/Makefile
> deleted file mode 100644
> index 61089fe77..000000000
> --- a/lib/librte_compat/Makefile
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -# SPDX-License-Identifier: BSD-3-Clause
> -# Copyright(c) 2013 Neil Horman <nhorman@tuxdriver.com>
> -# All rights reserved.
> -
> -include $(RTE_SDK)/mk/rte.vars.mk
> -
> -
> -LIBABIVER := 1
> -
> -# install includes
> -SYMLINK-y-include := rte_compat.h
> -
> -include $(RTE_SDK)/mk/rte.install.mk
> diff --git a/lib/librte_compat/meson.build b/lib/librte_compat/meson.build
> deleted file mode 100644
> index 82c7eea55..000000000
> --- a/lib/librte_compat/meson.build
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -# SPDX-License-Identifier: BSD-3-Clause
> -# Copyright(c) 2017 Intel Corporation
> -
> -
> -install_headers('rte_compat.h')
> -
> -set_variable('dep_rte_compat',
> - declare_dependency(include_directories: include_directories('.')))
> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
> index 87d8c455d..c487201b3 100644
> --- a/lib/librte_eal/common/Makefile
> +++ b/lib/librte_eal/common/Makefile
> @@ -3,7 +3,7 @@
>
> include $(RTE_SDK)/mk/rte.vars.mk
>
> -INC := rte_branch_prediction.h rte_common.h
> +INC := rte_branch_prediction.h rte_common.h rte_compat.h
> INC += rte_debug.h rte_eal.h rte_eal_interrupts.h
> INC += rte_errno.h rte_launch.h rte_lcore.h
> INC += rte_log.h rte_memory.h rte_memzone.h
> diff --git a/lib/librte_compat/rte_compat.h b/lib/librte_eal/common/include/rte_compat.h
> similarity index 100%
> rename from lib/librte_compat/rte_compat.h
> rename to lib/librte_eal/common/include/rte_compat.h
> diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build
> index 2a10d57d8..5ecae0b1f 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -53,6 +53,7 @@ common_headers = files(
> 'include/rte_bitmap.h',
> 'include/rte_class.h',
> 'include/rte_common.h',
> + 'include/rte_compat.h',
> 'include/rte_debug.h',
> 'include/rte_devargs.h',
> 'include/rte_dev.h',
> diff --git a/lib/librte_eal/linuxapp/eal/meson.build b/lib/librte_eal/linuxapp/eal/meson.build
> index 6e31c2aaa..7e68b2c0d 100644
> --- a/lib/librte_eal/linuxapp/eal/meson.build
> +++ b/lib/librte_eal/linuxapp/eal/meson.build
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2017 Intel Corporation
>
> -eal_inc += include_directories('include', '../../../librte_compat')
> +eal_inc += include_directories('include')
> install_subdir('include/exec-env', install_dir: get_option('includedir'))
>
> env_objs = []
> diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
> index a18f3a826..64d857a4a 100644
> --- a/lib/librte_eal/meson.build
> +++ b/lib/librte_eal/meson.build
> @@ -23,7 +23,6 @@ endif
>
> version = 9 # the version of the EAL API
> allow_experimental_apis = true
> -deps += 'compat'
> deps += 'kvargs'
> sources = common_sources + env_sources
> objs = common_objs + env_objs
> diff --git a/lib/librte_kvargs/meson.build b/lib/librte_kvargs/meson.build
> index acd3e5432..ecaedf5a5 100644
> --- a/lib/librte_kvargs/meson.build
> +++ b/lib/librte_kvargs/meson.build
> @@ -2,10 +2,7 @@
> # Copyright(c) 2017 Intel Corporation
>
> includes = [global_inc]
> -includes += include_directories('../librte_eal/common/include')
>
> version = 1
> sources = files('rte_kvargs.c')
> headers = files('rte_kvargs.h')
> -
> -deps += 'compat'
> diff --git a/lib/meson.build b/lib/meson.build
> index e8b40f546..edcccdcb6 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -8,7 +8,7 @@
> # sometimes skip deps that would be implied by others, e.g. if mempool is
> # given as a dep, no need to mention ring. This is especially true for the
> # core libs which are widely reused, so their deps are kept to a minimum.
> -libraries = [ 'compat', # just a header, used for versioning
> +libraries = [
> 'cmdline', # ethdev depends on cmdline for parsing functions
> 'kvargs', # eal depends on kvargs
> 'eal', 'ring', 'mempool', 'mbuf', 'net', 'meter', 'ethdev', 'pci', # core
> diff --git a/meson.build b/meson.build
> index 21d428fa1..2c83a5452 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -32,7 +32,7 @@ eal_pmd_path = join_paths(get_option('prefix'), driver_install_path)
> # configure the build, and make sure configs here and in config folder are
> # able to be included in any file. We also store a global array of include dirs
> # for passing to pmdinfogen scripts
> -global_inc = include_directories('.', 'config')
> +global_inc = include_directories('.', 'config', 'lib/librte_eal/common/include')
> subdir('config')
>
> # build libs and drivers
> --
> 2.20.1
>
>
On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > Since compat library is only a single header, we can easily move it into
> > the EAL common headers instead of tracking it separately. The downside of
> > this is that it becomes a little more difficult to have any libs that are
> > built before EAL depend on it. Thankfully, this is not a major problem as
> > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > already has the path to the compat.h header file explicitly called out as
> > an include path.
> >
> > However, to ensure that we don't hit problems later with this, we can add
> > EAL common headers folder to the global include list in the meson build
> > which means that all common headers can be safely used by all libraries, no
> > matter what their build order.
> >
> This assumes that the compat lib will always just be a header though, no? Will
> this work in the event that someone wants to add some compatibility code that
> requires its own C compilation unit?
>
No, it probably won't work, you'll hit an issue with any libraries that
don't depend on EAL and need that functionality. The question is whether
this is likely to be an issue in the future for us. I'd say the possiblity
is fairly remote, but I'm open to input on it.
> > As a side-effect, this patch also fixes an issue with building on BSD using
> > meson, due to compat lib no longer needing to be listed as a dependency.
> >
> Can you elaborate here a bit please? listing a lib as a dependency seems like a
> fundamental function of a build system, was there a bug with meson in this
> capacity?
>
It was a bug in DPDK. There was already a dependency on the compat
library from libeal from linux, but not from BSD, so when a further
dependency was added globally, the BSD build broke, but the linux one
didn't.
/Bruce
On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > Since compat library is only a single header, we can easily move it into
> > > the EAL common headers instead of tracking it separately. The downside of
> > > this is that it becomes a little more difficult to have any libs that are
> > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > already has the path to the compat.h header file explicitly called out as
> > > an include path.
> > >
> > > However, to ensure that we don't hit problems later with this, we can add
> > > EAL common headers folder to the global include list in the meson build
> > > which means that all common headers can be safely used by all libraries, no
> > > matter what their build order.
> > >
> > This assumes that the compat lib will always just be a header though, no? Will
> > this work in the event that someone wants to add some compatibility code that
> > requires its own C compilation unit?
> >
>
> No, it probably won't work, you'll hit an issue with any libraries that
> don't depend on EAL and need that functionality. The question is whether
> this is likely to be an issue in the future for us. I'd say the possiblity
> is fairly remote, but I'm open to input on it.
>
Im afraid I don't have any more visibility on that than anyone else. The fact
that it hasn't been needed yet is likely a good sign, but I am concerned at the
notion that this change enjoins us from having that flexibility.
> > > As a side-effect, this patch also fixes an issue with building on BSD using
> > > meson, due to compat lib no longer needing to be listed as a dependency.
> > >
> > Can you elaborate here a bit please? listing a lib as a dependency seems like a
> > fundamental function of a build system, was there a bug with meson in this
> > capacity?
> >
>
> It was a bug in DPDK. There was already a dependency on the compat
> library from libeal from linux, but not from BSD, so when a further
> dependency was added globally, the BSD build broke, but the linux one
> didn't.
>
Do you have a link to the breakage details? I'd like to look at it to see if
there is a way around this without enjoining us from adding compat C files in
the future.
Thanks
Neil
> /Bruce
>
On Thu, Feb 07, 2019 at 09:34:26AM -0500, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> > On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > > Since compat library is only a single header, we can easily move it into
> > > > the EAL common headers instead of tracking it separately. The downside of
> > > > this is that it becomes a little more difficult to have any libs that are
> > > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > > already has the path to the compat.h header file explicitly called out as
> > > > an include path.
> > > >
> > > > However, to ensure that we don't hit problems later with this, we can add
> > > > EAL common headers folder to the global include list in the meson build
> > > > which means that all common headers can be safely used by all libraries, no
> > > > matter what their build order.
> > > >
> > > This assumes that the compat lib will always just be a header though, no? Will
> > > this work in the event that someone wants to add some compatibility code that
> > > requires its own C compilation unit?
> > >
> >
> > No, it probably won't work, you'll hit an issue with any libraries that
> > don't depend on EAL and need that functionality. The question is whether
> > this is likely to be an issue in the future for us. I'd say the possiblity
> > is fairly remote, but I'm open to input on it.
> >
> Im afraid I don't have any more visibility on that than anyone else. The fact
> that it hasn't been needed yet is likely a good sign, but I am concerned at the
> notion that this change enjoins us from having that flexibility.
>
Yes. However, in general is it not the case that compatibility code belongs
in the actual library wanting to provide the compatibility? That is what
has been done up till now. If we do need compatibility code placed more
centrally, I think EAL is as good a place for it as any - the only library
which doesn't depend on EAL now is kvargs, so our risk area is pretty low,
I think.
Also, if we do need a compat libraries with .c files in it, there is no
reason we can't undo this change. It would be no more user visible than
adding a .c file to the existing structure, given that in both cases an
extra .so file will appear in the build output.
> > > > As a side-effect, this patch also fixes an issue with building on BSD using
> > > > meson, due to compat lib no longer needing to be listed as a dependency.
> > > >
> > > Can you elaborate here a bit please? listing a lib as a dependency seems like a
> > > fundamental function of a build system, was there a bug with meson in this
> > > capacity?
> > >
> >
> > It was a bug in DPDK. There was already a dependency on the compat
> > library from libeal from linux, but not from BSD, so when a further
> > dependency was added globally, the BSD build broke, but the linux one
> > didn't.
> >
> Do you have a link to the breakage details? I'd like to look at it to see if
> there is a way around this without enjoining us from adding compat C files in
> the future.
>
Don't have a link handy, but the basics, as I understand them are as below:
Commit a8499f65a1d1 ("log: add missing experimental tag"), added the
experimental tag to a log function. To get this tag definition rte_compat.h
header was included in the rte_log.h file in the "common" EAL folder. In
build tests on linux, this was no problem since linuxapp/eal/meson.build had the line:
eal_inc += include_directories('include', '../../../librte_compat')
However, the BSD eal/meson.build file did not have this line, giving a
compiler error.
/Bruce
PS: Now that I also look at the files more closely, with this move to put
compat.h into EAL, we can probably remove the special case processing in
lib/meson.build for libraries with an empty source array.
On Thu, Feb 07, 2019 at 03:03:28PM +0000, Bruce Richardson wrote:
> On Thu, Feb 07, 2019 at 09:34:26AM -0500, Neil Horman wrote:
> > On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> > > On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > > > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > > > Since compat library is only a single header, we can easily move it into
> > > > > the EAL common headers instead of tracking it separately. The downside of
> > > > > this is that it becomes a little more difficult to have any libs that are
> > > > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > > > already has the path to the compat.h header file explicitly called out as
> > > > > an include path.
> > > > >
> > > > > However, to ensure that we don't hit problems later with this, we can add
> > > > > EAL common headers folder to the global include list in the meson build
> > > > > which means that all common headers can be safely used by all libraries, no
> > > > > matter what their build order.
> > > > >
> > > > This assumes that the compat lib will always just be a header though, no? Will
> > > > this work in the event that someone wants to add some compatibility code that
> > > > requires its own C compilation unit?
> > > >
> > >
> > > No, it probably won't work, you'll hit an issue with any libraries that
> > > don't depend on EAL and need that functionality. The question is whether
> > > this is likely to be an issue in the future for us. I'd say the possiblity
> > > is fairly remote, but I'm open to input on it.
> > >
> > Im afraid I don't have any more visibility on that than anyone else. The fact
> > that it hasn't been needed yet is likely a good sign, but I am concerned at the
> > notion that this change enjoins us from having that flexibility.
> >
> Yes. However, in general is it not the case that compatibility code belongs
> in the actual library wanting to provide the compatibility? That is what
> has been done up till now. If we do need compatibility code placed more
> centrally, I think EAL is as good a place for it as any - the only library
> which doesn't depend on EAL now is kvargs, so our risk area is pretty low,
> I think.
>
> Also, if we do need a compat libraries with .c files in it, there is no
> reason we can't undo this change. It would be no more user visible than
> adding a .c file to the existing structure, given that in both cases an
> extra .so file will appear in the build output.
>
If the consensus is that compat code can all live in the EAL library, then I'm
ok with it, even if its C code. The only thing I don't want is for our plan to
be, in the event we need C code, to immediately undo this change. That just
doesn't make sense to me.
So, if you're ok with compat C code in eal, then
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > As a side-effect, this patch also fixes an issue with building on BSD using
> > > > > meson, due to compat lib no longer needing to be listed as a dependency.
> > > > >
> > > > Can you elaborate here a bit please? listing a lib as a dependency seems like a
> > > > fundamental function of a build system, was there a bug with meson in this
> > > > capacity?
> > > >
> > >
> > > It was a bug in DPDK. There was already a dependency on the compat
> > > library from libeal from linux, but not from BSD, so when a further
> > > dependency was added globally, the BSD build broke, but the linux one
> > > didn't.
> > >
> > Do you have a link to the breakage details? I'd like to look at it to see if
> > there is a way around this without enjoining us from adding compat C files in
> > the future.
> >
>
> Don't have a link handy, but the basics, as I understand them are as below:
>
> Commit a8499f65a1d1 ("log: add missing experimental tag"), added the
> experimental tag to a log function. To get this tag definition rte_compat.h
> header was included in the rte_log.h file in the "common" EAL folder. In
> build tests on linux, this was no problem since linuxapp/eal/meson.build had the line:
> eal_inc += include_directories('include', '../../../librte_compat')
> However, the BSD eal/meson.build file did not have this line, giving a
> compiler error.
>
> /Bruce
>
> PS: Now that I also look at the files more closely, with this move to put
> compat.h into EAL, we can probably remove the special case processing in
> lib/meson.build for libraries with an empty source array.
>
>
On Fri, Feb 08, 2019 at 10:37:40AM -0500, Neil Horman wrote:
> On Thu, Feb 07, 2019 at 03:03:28PM +0000, Bruce Richardson wrote:
> > On Thu, Feb 07, 2019 at 09:34:26AM -0500, Neil Horman wrote:
> > > On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> > > > On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > > > > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > > > > Since compat library is only a single header, we can easily move it into
> > > > > > the EAL common headers instead of tracking it separately. The downside of
> > > > > > this is that it becomes a little more difficult to have any libs that are
> > > > > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > > > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > > > > already has the path to the compat.h header file explicitly called out as
> > > > > > an include path.
> > > > > >
> > > > > > However, to ensure that we don't hit problems later with this, we can add
> > > > > > EAL common headers folder to the global include list in the meson build
> > > > > > which means that all common headers can be safely used by all libraries, no
> > > > > > matter what their build order.
> > > > > >
> > > > > This assumes that the compat lib will always just be a header though, no? Will
> > > > > this work in the event that someone wants to add some compatibility code that
> > > > > requires its own C compilation unit?
> > > > >
> > > >
> > > > No, it probably won't work, you'll hit an issue with any libraries that
> > > > don't depend on EAL and need that functionality. The question is whether
> > > > this is likely to be an issue in the future for us. I'd say the possiblity
> > > > is fairly remote, but I'm open to input on it.
> > > >
> > > Im afraid I don't have any more visibility on that than anyone else. The fact
> > > that it hasn't been needed yet is likely a good sign, but I am concerned at the
> > > notion that this change enjoins us from having that flexibility.
> > >
> > Yes. However, in general is it not the case that compatibility code belongs
> > in the actual library wanting to provide the compatibility? That is what
> > has been done up till now. If we do need compatibility code placed more
> > centrally, I think EAL is as good a place for it as any - the only library
> > which doesn't depend on EAL now is kvargs, so our risk area is pretty low,
> > I think.
> >
> > Also, if we do need a compat libraries with .c files in it, there is no
> > reason we can't undo this change. It would be no more user visible than
> > adding a .c file to the existing structure, given that in both cases an
> > extra .so file will appear in the build output.
> >
> If the consensus is that compat code can all live in the EAL library, then I'm
> ok with it, even if its C code. The only thing I don't want is for our plan to
> be, in the event we need C code, to immediately undo this change. That just
> doesn't make sense to me.
>
> So, if you're ok with compat C code in eal, then
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>
Can you clarify what you would see as the compat C code that would be
needed - perhaps an example from another project? From the little function
versioning I've done in DPDK, I would have thought what was in the headers
was enough for all cases we might encounter.
/Bruce
On Fri, Feb 08, 2019 at 04:18:38PM +0000, Bruce Richardson wrote:
> On Fri, Feb 08, 2019 at 10:37:40AM -0500, Neil Horman wrote:
> > On Thu, Feb 07, 2019 at 03:03:28PM +0000, Bruce Richardson wrote:
> > > On Thu, Feb 07, 2019 at 09:34:26AM -0500, Neil Horman wrote:
> > > > On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> > > > > On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > > > > > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > > > > > Since compat library is only a single header, we can easily move it into
> > > > > > > the EAL common headers instead of tracking it separately. The downside of
> > > > > > > this is that it becomes a little more difficult to have any libs that are
> > > > > > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > > > > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > > > > > already has the path to the compat.h header file explicitly called out as
> > > > > > > an include path.
> > > > > > >
> > > > > > > However, to ensure that we don't hit problems later with this, we can add
> > > > > > > EAL common headers folder to the global include list in the meson build
> > > > > > > which means that all common headers can be safely used by all libraries, no
> > > > > > > matter what their build order.
> > > > > > >
> > > > > > This assumes that the compat lib will always just be a header though, no? Will
> > > > > > this work in the event that someone wants to add some compatibility code that
> > > > > > requires its own C compilation unit?
> > > > > >
> > > > >
> > > > > No, it probably won't work, you'll hit an issue with any libraries that
> > > > > don't depend on EAL and need that functionality. The question is whether
> > > > > this is likely to be an issue in the future for us. I'd say the possiblity
> > > > > is fairly remote, but I'm open to input on it.
> > > > >
> > > > Im afraid I don't have any more visibility on that than anyone else. The fact
> > > > that it hasn't been needed yet is likely a good sign, but I am concerned at the
> > > > notion that this change enjoins us from having that flexibility.
> > > >
> > > Yes. However, in general is it not the case that compatibility code belongs
> > > in the actual library wanting to provide the compatibility? That is what
> > > has been done up till now. If we do need compatibility code placed more
> > > centrally, I think EAL is as good a place for it as any - the only library
> > > which doesn't depend on EAL now is kvargs, so our risk area is pretty low,
> > > I think.
> > >
> > > Also, if we do need a compat libraries with .c files in it, there is no
> > > reason we can't undo this change. It would be no more user visible than
> > > adding a .c file to the existing structure, given that in both cases an
> > > extra .so file will appear in the build output.
> > >
> > If the consensus is that compat code can all live in the EAL library, then I'm
> > ok with it, even if its C code. The only thing I don't want is for our plan to
> > be, in the event we need C code, to immediately undo this change. That just
> > doesn't make sense to me.
> >
> > So, if you're ok with compat C code in eal, then
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> >
> Can you clarify what you would see as the compat C code that would be
> needed - perhaps an example from another project? From the little function
> versioning I've done in DPDK, I would have thought what was in the headers
> was enough for all cases we might encounter.
>
I can't, hence my ACK. I was really just concerned that we were making a change
that enjoined us from being able to add C compilation units should we need them,
but if we can add them directly to the EAL libraries, I'm satisfied with that.
Neil
> /Bruce
>
On Fri, Feb 08, 2019 at 11:55:09AM -0500, Neil Horman wrote:
> On Fri, Feb 08, 2019 at 04:18:38PM +0000, Bruce Richardson wrote:
> > On Fri, Feb 08, 2019 at 10:37:40AM -0500, Neil Horman wrote:
> > > On Thu, Feb 07, 2019 at 03:03:28PM +0000, Bruce Richardson wrote:
> > > > On Thu, Feb 07, 2019 at 09:34:26AM -0500, Neil Horman wrote:
> > > > > On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> > > > > > On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > > > > > > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > > > > > > Since compat library is only a single header, we can easily move it into
> > > > > > > > the EAL common headers instead of tracking it separately. The downside of
> > > > > > > > this is that it becomes a little more difficult to have any libs that are
> > > > > > > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > > > > > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > > > > > > already has the path to the compat.h header file explicitly called out as
> > > > > > > > an include path.
> > > > > > > >
> > > > > > > > However, to ensure that we don't hit problems later with this, we can add
> > > > > > > > EAL common headers folder to the global include list in the meson build
> > > > > > > > which means that all common headers can be safely used by all libraries, no
> > > > > > > > matter what their build order.
> > > > > > > >
> > > > > > > This assumes that the compat lib will always just be a header though, no? Will
> > > > > > > this work in the event that someone wants to add some compatibility code that
> > > > > > > requires its own C compilation unit?
> > > > > > >
> > > > > >
> > > > > > No, it probably won't work, you'll hit an issue with any libraries that
> > > > > > don't depend on EAL and need that functionality. The question is whether
> > > > > > this is likely to be an issue in the future for us. I'd say the possiblity
> > > > > > is fairly remote, but I'm open to input on it.
> > > > > >
> > > > > Im afraid I don't have any more visibility on that than anyone else. The fact
> > > > > that it hasn't been needed yet is likely a good sign, but I am concerned at the
> > > > > notion that this change enjoins us from having that flexibility.
> > > > >
> > > > Yes. However, in general is it not the case that compatibility code belongs
> > > > in the actual library wanting to provide the compatibility? That is what
> > > > has been done up till now. If we do need compatibility code placed more
> > > > centrally, I think EAL is as good a place for it as any - the only library
> > > > which doesn't depend on EAL now is kvargs, so our risk area is pretty low,
> > > > I think.
> > > >
> > > > Also, if we do need a compat libraries with .c files in it, there is no
> > > > reason we can't undo this change. It would be no more user visible than
> > > > adding a .c file to the existing structure, given that in both cases an
> > > > extra .so file will appear in the build output.
> > > >
> > > If the consensus is that compat code can all live in the EAL library, then I'm
> > > ok with it, even if its C code. The only thing I don't want is for our plan to
> > > be, in the event we need C code, to immediately undo this change. That just
> > > doesn't make sense to me.
> > >
> > > So, if you're ok with compat C code in eal, then
> > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > >
> > Can you clarify what you would see as the compat C code that would be
> > needed - perhaps an example from another project? From the little function
> > versioning I've done in DPDK, I would have thought what was in the headers
> > was enough for all cases we might encounter.
> >
> I can't, hence my ACK. I was really just concerned that we were making a change
> that enjoined us from being able to add C compilation units should we need them,
> but if we can add them directly to the EAL libraries, I'm satisfied with that.
>
> Neil
>
Ok, thanks, Neil.
+Thomas, in case you have any concerns or thoughts here.
On Fri, Feb 08, 2019 at 05:13:21PM +0000, Bruce Richardson wrote:
> On Fri, Feb 08, 2019 at 11:55:09AM -0500, Neil Horman wrote:
> > On Fri, Feb 08, 2019 at 04:18:38PM +0000, Bruce Richardson wrote:
> > > On Fri, Feb 08, 2019 at 10:37:40AM -0500, Neil Horman wrote:
> > > > On Thu, Feb 07, 2019 at 03:03:28PM +0000, Bruce Richardson wrote:
> > > > > On Thu, Feb 07, 2019 at 09:34:26AM -0500, Neil Horman wrote:
> > > > > > On Wed, Feb 06, 2019 at 02:17:45PM +0000, Bruce Richardson wrote:
> > > > > > > On Wed, Feb 06, 2019 at 07:22:54AM -0500, Neil Horman wrote:
> > > > > > > > On Wed, Feb 06, 2019 at 11:01:30AM +0000, Bruce Richardson wrote:
> > > > > > > > > Since compat library is only a single header, we can easily move it into
> > > > > > > > > the EAL common headers instead of tracking it separately. The downside of
> > > > > > > > > this is that it becomes a little more difficult to have any libs that are
> > > > > > > > > built before EAL depend on it. Thankfully, this is not a major problem as
> > > > > > > > > the only library which uses rte_compat.h and is built before EAL (kvargs)
> > > > > > > > > already has the path to the compat.h header file explicitly called out as
> > > > > > > > > an include path.
> > > > > > > > >
> > > > > > > > > However, to ensure that we don't hit problems later with this, we can add
> > > > > > > > > EAL common headers folder to the global include list in the meson build
> > > > > > > > > which means that all common headers can be safely used by all libraries, no
> > > > > > > > > matter what their build order.
> > > > > > > > >
> > > > > > > > This assumes that the compat lib will always just be a header though, no? Will
> > > > > > > > this work in the event that someone wants to add some compatibility code that
> > > > > > > > requires its own C compilation unit?
> > > > > > > >
> > > > > > >
> > > > > > > No, it probably won't work, you'll hit an issue with any libraries that
> > > > > > > don't depend on EAL and need that functionality. The question is whether
> > > > > > > this is likely to be an issue in the future for us. I'd say the possiblity
> > > > > > > is fairly remote, but I'm open to input on it.
> > > > > > >
> > > > > > Im afraid I don't have any more visibility on that than anyone else. The fact
> > > > > > that it hasn't been needed yet is likely a good sign, but I am concerned at the
> > > > > > notion that this change enjoins us from having that flexibility.
> > > > > >
> > > > > Yes. However, in general is it not the case that compatibility code belongs
> > > > > in the actual library wanting to provide the compatibility? That is what
> > > > > has been done up till now. If we do need compatibility code placed more
> > > > > centrally, I think EAL is as good a place for it as any - the only library
> > > > > which doesn't depend on EAL now is kvargs, so our risk area is pretty low,
> > > > > I think.
> > > > >
> > > > > Also, if we do need a compat libraries with .c files in it, there is no
> > > > > reason we can't undo this change. It would be no more user visible than
> > > > > adding a .c file to the existing structure, given that in both cases an
> > > > > extra .so file will appear in the build output.
> > > > >
> > > > If the consensus is that compat code can all live in the EAL library, then I'm
> > > > ok with it, even if its C code. The only thing I don't want is for our plan to
> > > > be, in the event we need C code, to immediately undo this change. That just
> > > > doesn't make sense to me.
> > > >
> > > > So, if you're ok with compat C code in eal, then
> > > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > > >
> > > Can you clarify what you would see as the compat C code that would be
> > > needed - perhaps an example from another project? From the little function
> > > versioning I've done in DPDK, I would have thought what was in the headers
> > > was enough for all cases we might encounter.
> > >
> > I can't, hence my ACK. I was really just concerned that we were making a change
> > that enjoined us from being able to add C compilation units should we need them,
> > but if we can add them directly to the EAL libraries, I'm satisfied with that.
> >
> > Neil
> >
> Ok, thanks, Neil.
>
> +Thomas, in case you have any concerns or thoughts here.
Ping on this patch. As far as I am concerned, this is ready for merge.
Currently the FreeBSD builds using meson are broken, and this patch or
another fix is needed to get them working again.
Regards,
/Bruce
06/02/2019 12:01, Bruce Richardson:
> Since compat library is only a single header, we can easily move it into
> the EAL common headers instead of tracking it separately. The downside of
> this is that it becomes a little more difficult to have any libs that are
> built before EAL depend on it. Thankfully, this is not a major problem as
> the only library which uses rte_compat.h and is built before EAL (kvargs)
> already has the path to the compat.h header file explicitly called out as
> an include path.
>
> However, to ensure that we don't hit problems later with this, we can add
> EAL common headers folder to the global include list in the meson build
> which means that all common headers can be safely used by all libraries, no
> matter what their build order.
>
> As a side-effect, this patch also fixes an issue with building on BSD using
> meson, due to compat lib no longer needing to be listed as a dependency.
>
> Fixes: a8499f65a1d1 ("log: add missing experimental tag")
>
> CC: stable@dpdk.org
> CC: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Tested-by: David Marchand <david.marchand@redhat.com>
> Tested-by: Anatoly Burakov <anatoly.burakov@intel.com>
Applied, thanks
@@ -121,7 +121,6 @@ F: buildtools/symlink-drivers-solibs.sh
ABI versioning
M: Neil Horman <nhorman@tuxdriver.com>
-F: lib/librte_compat/
F: doc/guides/rel_notes/deprecation.rst
F: devtools/validate-abi.sh
F: devtools/check-symbol-change.sh
@@ -23,7 +23,6 @@ INPUT = @TOPDIR@/doc/api/doxy-api-index.md \
@TOPDIR@/lib/librte_bpf \
@TOPDIR@/lib/librte_cfgfile \
@TOPDIR@/lib/librte_cmdline \
- @TOPDIR@/lib/librte_compat \
@TOPDIR@/lib/librte_compressdev \
@TOPDIR@/lib/librte_cryptodev \
@TOPDIR@/lib/librte_distributor \
@@ -22,7 +22,6 @@ The main directories that contain files related to documentation are shown below
|-- librte_acl
|-- librte_cfgfile
|-- librte_cmdline
- |-- librte_compat
|-- librte_eal
| |-- ...
...
@@ -167,7 +167,7 @@ functionality or behavior. When that occurs, it is desirable to allow for
backward compatibility for a time with older binaries that are dynamically
linked to the DPDK.
-To support backward compatibility the ``lib/librte_compat/rte_compat.h``
+To support backward compatibility the ``rte_compat.h``
header file provides macros to use when updating exported functions. These
macros are used in conjunction with the ``rte_<library>_version.map`` file for
a given library to allow multiple versions of a symbol to exist in a shared
@@ -3,9 +3,7 @@
include $(RTE_SDK)/mk/rte.vars.mk
-DIRS-y += librte_compat
DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
-DEPDIRS-librte_kvargs := librte_compat
DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
DEPDIRS-librte_eal := librte_kvargs
DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci
@@ -3,7 +3,6 @@
# This library is processed before EAL
includes = [global_inc]
-includes += include_directories('../librte_eal/common/include')
version = 2
sources = files('cmdline.c',
deleted file mode 100644
@@ -1,13 +0,0 @@
-# SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2013 Neil Horman <nhorman@tuxdriver.com>
-# All rights reserved.
-
-include $(RTE_SDK)/mk/rte.vars.mk
-
-
-LIBABIVER := 1
-
-# install includes
-SYMLINK-y-include := rte_compat.h
-
-include $(RTE_SDK)/mk/rte.install.mk
deleted file mode 100644
@@ -1,8 +0,0 @@
-# SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
-
-
-install_headers('rte_compat.h')
-
-set_variable('dep_rte_compat',
- declare_dependency(include_directories: include_directories('.')))
@@ -3,7 +3,7 @@
include $(RTE_SDK)/mk/rte.vars.mk
-INC := rte_branch_prediction.h rte_common.h
+INC := rte_branch_prediction.h rte_common.h rte_compat.h
INC += rte_debug.h rte_eal.h rte_eal_interrupts.h
INC += rte_errno.h rte_launch.h rte_lcore.h
INC += rte_log.h rte_memory.h rte_memzone.h
similarity index 100%
rename from lib/librte_compat/rte_compat.h
rename to lib/librte_eal/common/include/rte_compat.h
@@ -53,6 +53,7 @@ common_headers = files(
'include/rte_bitmap.h',
'include/rte_class.h',
'include/rte_common.h',
+ 'include/rte_compat.h',
'include/rte_debug.h',
'include/rte_devargs.h',
'include/rte_dev.h',
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright(c) 2017 Intel Corporation
-eal_inc += include_directories('include', '../../../librte_compat')
+eal_inc += include_directories('include')
install_subdir('include/exec-env', install_dir: get_option('includedir'))
env_objs = []
@@ -23,7 +23,6 @@ endif
version = 9 # the version of the EAL API
allow_experimental_apis = true
-deps += 'compat'
deps += 'kvargs'
sources = common_sources + env_sources
objs = common_objs + env_objs
@@ -2,10 +2,7 @@
# Copyright(c) 2017 Intel Corporation
includes = [global_inc]
-includes += include_directories('../librte_eal/common/include')
version = 1
sources = files('rte_kvargs.c')
headers = files('rte_kvargs.h')
-
-deps += 'compat'
@@ -8,7 +8,7 @@
# sometimes skip deps that would be implied by others, e.g. if mempool is
# given as a dep, no need to mention ring. This is especially true for the
# core libs which are widely reused, so their deps are kept to a minimum.
-libraries = [ 'compat', # just a header, used for versioning
+libraries = [
'cmdline', # ethdev depends on cmdline for parsing functions
'kvargs', # eal depends on kvargs
'eal', 'ring', 'mempool', 'mbuf', 'net', 'meter', 'ethdev', 'pci', # core
@@ -32,7 +32,7 @@ eal_pmd_path = join_paths(get_option('prefix'), driver_install_path)
# configure the build, and make sure configs here and in config folder are
# able to be included in any file. We also store a global array of include dirs
# for passing to pmdinfogen scripts
-global_inc = include_directories('.', 'config')
+global_inc = include_directories('.', 'config', 'lib/librte_eal/common/include')
subdir('config')
# build libs and drivers