diff mbox series

devtools: give some hints for ABI errors

Message ID 20200708102212.3311-1-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series devtools: give some hints for ABI errors |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot success Travis build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

David Marchand July 8, 2020, 10:22 a.m. UTC 
  abidiff can provide some more information about the ABI difference it
detected.
In all cases, a discussion on the mailing must happen but we can give
some hints to know if this is a problem with the script calling abidiff,
a potential ABI breakage or an unambiguous ABI breakage.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 devtools/check-abi.sh | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Ray Kinsella July 8, 2020, 1:09 p.m. UTC | #1 
+ Aaron

On 08/07/2020 11:22, David Marchand wrote:
> abidiff can provide some more information about the ABI difference it
> detected.
> In all cases, a discussion on the mailing must happen but we can give
> some hints to know if this is a problem with the script calling abidiff,
> a potential ABI breakage or an unambiguous ABI breakage.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  devtools/check-abi.sh | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
> index e17fedbd9f..521e2cce7c 100755
> --- a/devtools/check-abi.sh
> +++ b/devtools/check-abi.sh
> @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
>  		error=1
>  		continue
>  	fi
> -	if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
> +	abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
> +		abiret=$?
>  		echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
>  		error=1
> -	fi
> +		echo
> +		if [ $(($abiret & 3)) != 0 ]; then
> +			echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
> +		fi
> +		if [ $(($abiret & 4)) != 0 ]; then
> +			echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
> +		fi
> +		if [ $(($abiret & 8)) != 0 ]; then
> +			echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
> +		fi
> +		echo
> +	}
>  done
>  
>  [ -z "$error" ] || [ -n "$warnonly" ]
> 

This look good to me, my only thought was can we do anything to help the ABI checks play nice with Travis.
At the moment it takes time to find the failure reason in the Travis log.

Ray K
David Marchand July 8, 2020, 1:15 p.m. UTC | #2 
On Wed, Jul 8, 2020 at 3:09 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
>
> + Aaron
>
> On 08/07/2020 11:22, David Marchand wrote:
> > abidiff can provide some more information about the ABI difference it
> > detected.
> > In all cases, a discussion on the mailing must happen but we can give
> > some hints to know if this is a problem with the script calling abidiff,
> > a potential ABI breakage or an unambiguous ABI breakage.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  devtools/check-abi.sh | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
> > index e17fedbd9f..521e2cce7c 100755
> > --- a/devtools/check-abi.sh
> > +++ b/devtools/check-abi.sh
> > @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
> >               error=1
> >               continue
> >       fi
> > -     if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
> > +     abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
> > +             abiret=$?
> >               echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
> >               error=1
> > -     fi
> > +             echo
> > +             if [ $(($abiret & 3)) != 0 ]; then
> > +                     echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."

Forgot to --amend.
Hopefully yes, this will be reported to dev@dpdk.org... I wanted to
highlight this could be a script or env issue.


> > +             fi
> > +             if [ $(($abiret & 4)) != 0 ]; then
> > +                     echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
> > +             fi
> > +             if [ $(($abiret & 8)) != 0 ]; then
> > +                     echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
> > +             fi
> > +             echo
> > +     }
> >  done
> >
> >  [ -z "$error" ] || [ -n "$warnonly" ]
> >
>
> This look good to me, my only thought was can we do anything to help the ABI checks play nice with Travis.
> At the moment it takes time to find the failure reason in the Travis log.

I usually look for "FILES_TO" to get to the last error.
Ray Kinsella July 8, 2020, 1:22 p.m. UTC | #3 
On 08/07/2020 14:15, David Marchand wrote:
> On Wed, Jul 8, 2020 at 3:09 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
>>
>> + Aaron
>>
>> On 08/07/2020 11:22, David Marchand wrote:
>>> abidiff can provide some more information about the ABI difference it
>>> detected.
>>> In all cases, a discussion on the mailing must happen but we can give
>>> some hints to know if this is a problem with the script calling abidiff,
>>> a potential ABI breakage or an unambiguous ABI breakage.
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>  devtools/check-abi.sh | 16 ++++++++++++++--
>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
>>> index e17fedbd9f..521e2cce7c 100755
>>> --- a/devtools/check-abi.sh
>>> +++ b/devtools/check-abi.sh
>>> @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
>>>               error=1
>>>               continue
>>>       fi
>>> -     if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
>>> +     abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
>>> +             abiret=$?
>>>               echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
>>>               error=1
>>> -     fi
>>> +             echo
>>> +             if [ $(($abiret & 3)) != 0 ]; then
>>> +                     echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
> 
> Forgot to --amend.
> Hopefully yes, this will be reported to dev@dpdk.org... I wanted to
> highlight this could be a script or env issue.
> 
> 
>>> +             fi
>>> +             if [ $(($abiret & 4)) != 0 ]; then
>>> +                     echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
>>> +             fi
>>> +             if [ $(($abiret & 8)) != 0 ]; then
>>> +                     echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
>>> +             fi
>>> +             echo
>>> +     }
>>>  done
>>>
>>>  [ -z "$error" ] || [ -n "$warnonly" ]
>>>
>>
>> This look good to me, my only thought was can we do anything to help the ABI checks play nice with Travis.
>> At the moment it takes time to find the failure reason in the Travis log.
> 
> I usually look for "FILES_TO" to get to the last error.
> 
Right, but there is hopefully a better way to give Travis some clues ...
Aaron Conole July 8, 2020, 1:45 p.m. UTC | #4 
"Kinsella, Ray" <mdr@ashroe.eu> writes:

> + Aaron
>
> On 08/07/2020 11:22, David Marchand wrote:
>> abidiff can provide some more information about the ABI difference it
>> detected.
>> In all cases, a discussion on the mailing must happen but we can give
>> some hints to know if this is a problem with the script calling abidiff,
>> a potential ABI breakage or an unambiguous ABI breakage.
>> 
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> ---
>>  devtools/check-abi.sh | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
>> index e17fedbd9f..521e2cce7c 100755
>> --- a/devtools/check-abi.sh
>> +++ b/devtools/check-abi.sh
>> @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
>>  		error=1
>>  		continue
>>  	fi
>> -	if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
>> +	abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
>> +		abiret=$?
>>  		echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
>>  		error=1
>> -	fi
>> +		echo
>> +		if [ $(($abiret & 3)) != 0 ]; then
>> +			echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
>> +		fi
>> +		if [ $(($abiret & 4)) != 0 ]; then
>> +			echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
>> +		fi
>> +		if [ $(($abiret & 8)) != 0 ]; then
>> +			echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
>> +		fi
>> +		echo
>> +	}
>>  done
>>  
>>  [ -z "$error" ] || [ -n "$warnonly" ]
>> 
>
> This look good to me, my only thought was can we do anything to help the ABI checks play nice with Travis.
> At the moment it takes time to find the failure reason in the Travis log.

That's a problem even for non-ABI failures.  I was considering pulling
the travis log for each failed build and attaching it, but even that
isn't a great solution (very large emails aren't much easier to search).

I'm open to suggestions.

> Ray K
Ray Kinsella July 8, 2020, 2:01 p.m. UTC | #5 
On 08/07/2020 14:45, Aaron Conole wrote:
> "Kinsella, Ray" <mdr@ashroe.eu> writes:
> 
>> + Aaron
>>
>> On 08/07/2020 11:22, David Marchand wrote:
>>> abidiff can provide some more information about the ABI difference it
>>> detected.
>>> In all cases, a discussion on the mailing must happen but we can give
>>> some hints to know if this is a problem with the script calling abidiff,
>>> a potential ABI breakage or an unambiguous ABI breakage.
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>  devtools/check-abi.sh | 16 ++++++++++++++--
>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
>>> index e17fedbd9f..521e2cce7c 100755
>>> --- a/devtools/check-abi.sh
>>> +++ b/devtools/check-abi.sh
>>> @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
>>>  		error=1
>>>  		continue
>>>  	fi
>>> -	if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
>>> +	abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
>>> +		abiret=$?
>>>  		echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
>>>  		error=1
>>> -	fi
>>> +		echo
>>> +		if [ $(($abiret & 3)) != 0 ]; then
>>> +			echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
>>> +		fi
>>> +		if [ $(($abiret & 4)) != 0 ]; then
>>> +			echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
>>> +		fi
>>> +		if [ $(($abiret & 8)) != 0 ]; then
>>> +			echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
>>> +		fi
>>> +		echo
>>> +	}
>>>  done
>>>  
>>>  [ -z "$error" ] || [ -n "$warnonly" ]
>>>
>>
>> This look good to me, my only thought was can we do anything to help the ABI checks play nice with Travis.
>> At the moment it takes time to find the failure reason in the Travis log.
> 
> That's a problem even for non-ABI failures.  I was considering pulling
> the travis log for each failed build and attaching it, but even that
> isn't a great solution (very large emails aren't much easier to search).
> 
> I'm open to suggestions.

For me the problem arises when you log on to the Travis interface,
you need to search for ERROR etc ... there must a better way.

> 
>> Ray K
>
Dodji Seketeli July 9, 2020, 3:52 p.m. UTC | #6 
Hello,

David Marchand <david.marchand@redhat.com> writes:

> abidiff can provide some more information about the ABI difference it
> detected.
> In all cases, a discussion on the mailing must happen but we can give
> some hints to know if this is a problem with the script calling abidiff,
> a potential ABI breakage or an unambiguous ABI breakage.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

For what it's worth, the change looks good to me, at least from an
abidiff perspective.

Thanks.

Cheers.
Ray Kinsella July 10, 2020, 7:37 a.m. UTC | #7 
On 08/07/2020 11:22, David Marchand wrote:
> abidiff can provide some more information about the ABI difference it
> detected.
> In all cases, a discussion on the mailing must happen but we can give
> some hints to know if this is a problem with the script calling abidiff,
> a potential ABI breakage or an unambiguous ABI breakage.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  devtools/check-abi.sh | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
> index e17fedbd9f..521e2cce7c 100755
> --- a/devtools/check-abi.sh
> +++ b/devtools/check-abi.sh
> @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
>  		error=1
>  		continue
>  	fi
> -	if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
> +	abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
> +		abiret=$?
>  		echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
>  		error=1
> -	fi
> +		echo
> +		if [ $(($abiret & 3)) != 0 ]; then
> +			echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
> +		fi
> +		if [ $(($abiret & 4)) != 0 ]; then
> +			echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
> +		fi
> +		if [ $(($abiret & 8)) != 0 ]; then
> +			echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
> +		fi
> +		echo
> +	}
>  done
>  
>  [ -z "$error" ] || [ -n "$warnonly" ]
> 

Acked-by: Ray Kinsella <mdr@ashroe.eu>
Neil Horman July 10, 2020, 10:58 a.m. UTC | #8 
On Wed, Jul 08, 2020 at 12:22:12PM +0200, David Marchand wrote:
> abidiff can provide some more information about the ABI difference it
> detected.
> In all cases, a discussion on the mailing must happen but we can give
> some hints to know if this is a problem with the script calling abidiff,
> a potential ABI breakage or an unambiguous ABI breakage.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  devtools/check-abi.sh | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
> index e17fedbd9f..521e2cce7c 100755
> --- a/devtools/check-abi.sh
> +++ b/devtools/check-abi.sh
> @@ -50,10 +50,22 @@ for dump in $(find $refdir -name "*.dump"); do
>  		error=1
>  		continue
>  	fi
> -	if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
> +	abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
> +		abiret=$?
>  		echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
>  		error=1
> -	fi
> +		echo
> +		if [ $(($abiret & 3)) != 0 ]; then
> +			echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
> +		fi
> +		if [ $(($abiret & 4)) != 0 ]; then
> +			echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
> +		fi
> +		if [ $(($abiret & 8)) != 0 ]; then
> +			echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
> +		fi
> +		echo
> +	}
>  done
>  
>  [ -z "$error" ] || [ -n "$warnonly" ]
> -- 
> 2.23.0
> 
> 
this looks pretty reasonable to me, sure.
Acked-by: Neil Horman <nhorman@tuxdriver.com>
diff mbox series

Patch

diff --git a/devtools/check-abi.sh b/devtools/check-abi.sh
index e17fedbd9f..521e2cce7c 100755
--- a/devtools/check-abi.sh
+++ b/devtools/check-abi.sh
@@ -50,10 +50,22 @@  for dump in $(find $refdir -name "*.dump"); do
 		error=1
 		continue
 	fi
-	if ! abidiff $ABIDIFF_OPTIONS $dump $dump2; then
+	abidiff $ABIDIFF_OPTIONS $dump $dump2 || {
+		abiret=$?
 		echo "Error: ABI issue reported for 'abidiff $ABIDIFF_OPTIONS $dump $dump2'"
 		error=1
-	fi
+		echo
+		if [ $(($abiret & 3)) != 0 ]; then
+			echo "ABIDIFF_ERROR|ABIDIFF_USAGE_ERROR, please report this to dev@dpdk.org."
+		fi
+		if [ $(($abiret & 4)) != 0 ]; then
+			echo "ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue)."
+		fi
+		if [ $(($abiret & 8)) != 0 ]; then
+			echo "ABIDIFF_ABI_INCOMPATIBLE_CHANGE, this change breaks the ABI."
+		fi
+		echo
+	}
 done
 
 [ -z "$error" ] || [ -n "$warnonly" ]