[dpdk-dev] [PATCH] add ABI checks

[Neil Horman]

On Wed, Jan 15, 2020 at 01:38:17PM +0100, Thomas Monjalon wrote:
> 15/01/2020 12:33, Neil Horman:
> > On Wed, Jan 15, 2020 at 12:19:30AM +0100, Thomas Monjalon wrote:
> > > 20/12/2019 17:20, Kinsella, Ray:
> > > > From: Richardson, Bruce <bruce.richardson at intel.com>
> > > > > From: David Marchand <david.marchand at redhat.com>
> > > > > > +Checking ABI compatibility
> > > > > > +--------------------------
> > > > > > +
> > > > > > +The first thing is to build reference binaries for the latest
> > > > > release
> > > > > > +your patches are built on top of.
> > > > > > +
> > > > > > +Either you are in a git tree and an easy way to identify this is to
> > > > > run::
> > > > > > +
> > > > > > +  git checkout $(git describe --abbrev=0)
> > > > > > +
> > > > > > +Or you use a tarball and you extract the sources in a director of
> > > > > > +your
> > > > > > choice.
> > > > > > +
> > > > > > +Next is building those sources, refer to the previous paragraph.
> > > > > > +You can set ``DPDK_BUILD_TEST_DIR=reference``, so that the builds
> > > > > > +occur in this directory.
> > > > > > +
> > > > > > +Finally, the ABI dump files are generated with the
> > > > > > +``devtools/gen-abi-reference.sh`` script. This script will look for
> > > > > > +builds in the current sub directory ``reference``. But you can set
> > > > > > +the environment variable ``DPDK_ABI_REF_BUILD_DIR`` to a different
> > > > > location.
> > > > > > +
> > > > > > +Once done, you can check your current binaries ABI with this
> > > > > > +reference with the ``devtools/check-abi-reference.sh`` script.
> > > > > >
> > > > > 
> > > > > I still very much dislike forcing the user to generate his own
> > > > > reference version to compare the ABI against. These should be archived
> > > > > and the user should just be able to pull them down via git or http or
> > > > > otherwise. Two reasons for this:
> > > > > 
> > > > > 1. Less error prone, since there is no chance of the user having an
> > > > > incorrect build for whatever reason.
> > > > > 
> > > > > 2. Less effort for the user than asking them to do extra builds. The
> > > > > more steps the user has to follow, the less likely they are to attempt
> > > > > the process.
> > > > 
> > > > +1 ... 100% agree with this.
> > > > 
> > > > Many people won't know or understand what the reference is, 
> > > > or why they to generate it.
> > > 
> > > I don't want to generate and save the reference in git for each arch.
> > > 
> > Can I ask what your reluctance is?  Is it related to not wanting to have to save
> > all this information that is otherwise not used for building purposes?
> 
> Yes I prefer keeping only the sources in the repository.
> And these dumps are big.
> And last but not the least, there is no ready-to-use environment to build
> and dump all libs for all archs.
> 
> > If so I might suggest saving the dumps in a separate git tree and pulling them
> > in as a git submodule when the check is performed
> > 
> > I really like the idea of caching the results so everyone is working from a
> > known ABI baseline.
> 
> You don't trust the result of the build made from tagged sources?
> 
I trust the result from the tools, sure, its trusting that people will take the
extra time to build a version from a prior tag that I'm less sure of.
Consistent use in my mind is predicated on ease and timeliness of use.

I get not wanting to store large dumps in the source tree, but storage is cheap,
and I don't see the issue with storing the xml dump in a separate git tree to be
referenced through a git submodule that gets pulled in when the check is run.

Neil

> > > We can make reference build more automatic with a command like this:
> > > 	git clone --branch v19.11 . $DPDK_BUILD_TEST_DIR/abiref-19.11
> > > 
> > > Also I don't like mixing build and check steps.
> > > I believe the compilation should be simple and right to the point.
> > > 
> > > This approach, from David, does not prevent from saving the dumps later
> > > if we really feel a strong need.
> > > 
> > > That's why I suggest going with this patch.
> 
> 
> 
>