Re: [dpdk-dev] [PATCH] mk: enable next abi in static libs

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Jul 07, 2015 at 05:46:08AM -0700, Thomas Monjalon wrote:
> Thanks Neil, we are making good progress.
> 
> 2015-07-07 07:14, Neil Horman:
> > On Mon, Jul 06, 2015 at 11:44:59PM +0200, Thomas Monjalon wrote:
> > > 2015-07-06 14:22, Neil Horman:
> > > > On Mon, Jul 06, 2015 at 03:49:50PM +0200, Thomas Monjalon wrote:
> > > > > 2015-07-06 09:35, Neil Horman:
> > > > > > On Mon, Jul 06, 2015 at 03:18:51PM +0200, Thomas Monjalon wrote:
> > > > > > > Any comment or ack?
> > > > > > > 
> > > > > > > 2015-07-03 00:05, Thomas Monjalon:
> > > > > > > > When a change makes really hard to keep ABI compatibility,
> > > > > > > > instead of waiting next release to break the ABI, it is smoother
> > > > > > > > to introduce the new code and enable it only for static libraries.
> > > > > > > > The flag RTE_NEXT_ABI may be used to "ifdef" the new code.
> > > > > > > > When the release is out, a dynamically linked application can use
> > > > > > > > the new shared libraries without rebuild while developpers can prepare
> > > > > > > > their application for the next ABI by reading the deprecation notice
> > > > > > > > and easily testing the new code.
> > > > > > > > When starting the next release cycle, the "ifdefs" will be removed
> > > > > > > > and the ABI break will be marked by incrementing LIBABIVER.
> > > > > > > > 
> > > > > > > > The new option CONFIG_RTE_NEXT_ABI is not defined in the configuration
> > > > > > > > templates because it is deduced from CONFIG_RTE_BUILD_SHARED_LIB.
> > > > > > > > It is automatically enabled for static libraries and disabled for
> > > > > > > > shared libraries.
> > > > > > > > It can be forced to another value by editing the generated .config file.
> > > > > > > > It shouldn't be enabled for shared libraries because it would break the
> > > > > > > > ABI without changing the version number LIBABIVER. That's why a warning
> > > > > > > > is printed in this case.
> > > > > > > > 
> > > > > > > > The guideline is also updated to integrate this new possibility.
> > > [...]
> > > > I'd be ok with it iff:
> > > > 
> > > > 1) It applies to static and shared ABI's together.  That is to say that setting
> > > > the NEXT_ABI config flag creates the same ABI changes regardless of other build
> > > > configuration.  It needs to be used in such a way that a consistent ABI is
> > > > presented when set, otherwise it won't be useful.
> > > 
> > > Yes the option trigger exactly the same ABI for static and shared libraries.
> > > But it's too complicated (at least for 2.1) to make LIBABIVER and version map
> > > dependant of this build-time option.
> > 
> > No, I think thats a bridge too far.  I'm not sure whats difficult about
> > overriding LIBABIVER in lib.rte.mk and bump all numbers 1 higher (or better
> > still just add a .1 to the end of it), by checking CONFIG_NEXT_ABI
> 
> Good idea, I will submit a v2 which adds .1.
> 
> > As for maintaining the version map, I don't see any problem with duplicating the
> > map files, to a -next variant, and changing the CPU_LDFLAGS in rte.lib.mk based
> > on the NEXT_ABI config option again.
> 
> OK
> 
> > In fact, if this is a thing that people want, that might be beneficial, as
> > something else occurs to me.  I think you're going to want this to be a mandated
> > piece of the update process.  That is to say, if someone wants to deprecate an
> > aspect of the ABI, or change it, I think you'll want to mandate that they submit
> > the change at the same time they submit the deprecation notice, and simply
> > protect it with this NEXT_ABI config option.  That provides several advantages:
> 
> For the release 2.1, we have some deprecation notices without code. It was
> the policy agreed in 2.0 release.
> Maybe we can force code to be submitted with deprecation notices, starting
> with release 2.2.
> It needs to be amended in v2 of this patch.
> 
> > 1) It ensures that the notice is submitted at the same time as the actual change
> > 2) It ensures that the NEXT_ABI provides a complete view of what the next ABI
> > version looks like, not just a partial view of it
> 
> Yes it would be probably useful.
> 
> > Adding a *-version-next.map file for each library makes adhering to the above
> > easier, and allows for an easy converstion, in that when its time to officially
> > update the ABI, fixing the version map is a matter of copying
> > <library>-version-next.map to <library>-version.map.
> 
> OK
> 
> [...]
> > > That's why, it should not be enabled to deploy shared libraries, though it can
> > > be used for tests and development.
> > > As static libraries are almost never packaged, they will be built and linked
> > > at the same time. That's why users of static libraries tend to prefer the
> > > newest ABI, which is the default in this case.
> > > 
> > > > 2) It only applies to the next ABI.  That is to say, it can't be a hodgepodge of
> > > > the next ABI and the one after that, and the one after that, or it won't provide
> > > > an appropriate preview for anyone.
> > > 
> > > If you mean the next ABI must be promoted as the standard ABI in the next release,
> > > yes: ifdefs will be cleaned when starting a new release.
> > > Thanks, I learnt the english word hodgepodge :)
> > > 
> > Je-mexcuse, une meli-melo? :)
> 
> Oui un meli-melo, un ramassis. un beau bordel en somme.
> 
> > I mean't what you indicate yes, and in addition to that, I just wanted to
> > clarify that this option could strictly _only_ apply to the very next ABI.  That
> > is to say, someone can't use this without also posting an ABI deprecation
> > notice, or we would find ourselves in a situation where something would only be
> > available in NEXT_ABI for more than one release, which would be unacceptable.
> > But I think we're saying the same thing.
> 
> Yes. I'll try to make it clear in v2.
> 
> Neil, in the meantime, could you please help to check ABI breakage in the HEAD?
> 
Took a look, the only ABI break I see that we need to worry about is the one
introduced in commit 8eecb3295aed0a979def52245564d03be172a83c. It adds a
bitfield called lro into the existing uint8_t there, but does so in the middle
of the set, which pushes the other bits around, breaking ABI.  It should have
been added to the end.

Neil