Re: [dpdk-stable] [PATCH v3] compat: merge compat library into EAL

[Bruce Richardson <bruce.richardson@intel.com>]

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.