Re: [dpdk-dev] [PATCH v3] devtools: rework abi checker script

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Neil,

On Wed, Sep 13, 2017 at 11:00:13AM -0400, Neil Horman wrote:
> On Mon, Sep 11, 2017 at 10:46:35AM +0200, Olivier Matz wrote:
> > The initial version of the script had some limitations:
> > - cannot work on a non-clean workspace
> > - environment variables are not documented
> > - no compilation log in case of failure
> > - return success even it abi is incompatible
> > 
> > This patch addresses these issues and rework the code.
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> > 
> > v2->v3:
> > - fix when not launched from dpdk root dir
> > - use "-Og -Wno-error" instead of "-O0"
> > - fix typo in commit log
> > 
> > v1->v2:
> > - use /usr/bin/env to find bash (which is required)
> > - fix displayed path to html reports
> > - reword help for -f option
> > 
> >  devtools/validate-abi.sh | 392 ++++++++++++++++++++++++-----------------------
> >  1 file changed, 200 insertions(+), 192 deletions(-)
> > 
> 
> This is alot closer, I think theres just a little wierdness left.  When running
> this checker script, the end of the log shows this:
> 
> CMD: abi-compliance-checker -l librte_vhost.so -old /home/nhorman/git/dpdk/devtools/abi-check/222555480/librte_vhost.so.dump -new /home/nhorman/git/dpdk/devtools/abi-check/02657b4ad/librte_vhost.so.dump
> NOTICE: At least one call to abi-compliance-checker returned an error.
> NOTICE: ABI may be incompatible, please check logs for details.
> NOTICE: Incompatible list:  librte_cryptodev.so librte_eal.so librte_efd.so librte_ethdev.so librte_eventdev.so librte_hash.so librte_pdump.so librte_pmd_crypto_scheduler.so librte_pmd_ring.so librte_ring.so
> 
> Which I think is something of a false positive.  The line:
> NOTICE: At least one call to abi-compliance-checker returned an error.
>
> I think is emitted simply because abi-compilance-checker returns non-zero and
> reports that error if it finds any incompatibilities.  I'm not sure we want to
> flag that as an error per se.  It gives the impression something has gone wrong,
> rather than correctly identifying that there are incompatibilities.
> 
> Though on the other hand, maybe we do want to set that red flag so people look
> at what the incompatibilities are.  As I say it out loud, perhaps that
> preferable.

Yes, I can remove this line
"At least one call to abi-compliance-checker returned an error."
which does not give a lot of info anyway.

While reproducing the issue, I also realized that the report path
was not preoperly fixed. So I'm sending a v4 with these fixes.

Thanks,
Olivier