Re: [dpdk-dev] [PATCH v6] checkpatches.sh: Add checks for ABI symbol addition

[Neil Horman <nhorman@tuxdriver.com>]

On Sun, May 27, 2018 at 09:34:13PM +0200, Thomas Monjalon wrote:
> Hi Neil,
> 
> Sorry, this patch has been forgotten during the whole release cycle.
> 
Its ok, though that is frustrating, as we now need to re-familiarize ourselves
with how this patch works.

> I see an issue about the dependency on filterdiff.
> Is there a way to avoid it?
> 
I suppose, but to do so would require some awk magic.  Is there any advantage to
having an awk dependency rather than a filterdiff dependency?  filterdiff is a
pretty universal tool.

> I have some minor comments below.
> 
> 14/02/2018 20:19, Neil Horman:
> > @@ -61,6 +65,7 @@ print_usage () {
> >  	END_OF_HELP
> >  }
> >  
> > +
> 
> This new blank line is probably a leftover.
Yeah, if I wind up respinning this, I'll remove it.

> 
> >  number=0
> >  quiet=false
> >  verbose=false
> > @@ -86,19 +91,50 @@ total=0
> >  status=0
> >  
> >  check () { # <patch> <commit> <title>
> > +	local ret=0
> > +	TMPINPUT=`mktemp checkpatches.XXXXXX`
> 
> It is preferred to use $() construct to be consistent in the file.
> 
> > +
> >  	total=$(($total + 1))
> >  	! $verbose || printf '\n### %s\n\n' "$3"
> >  	if [ -n "$1" ] ; then
> >  		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
> >  	elif [ -n "$2" ] ; then
> > -		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
> > +		git format-patch --find-renames --no-stat --stdout -1 $commit > ./$TMPINPUT
> > +		report=$(cat ./$TMPINPUT |
> >  			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> 
> No need to use cat here.
> 
Actually, I vaguely recall with these changes I did encounter a problem with passing
output to checkpatch, but its been so long I can't recall now.

> >  	else
> > -		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > +		cat > ./$TMPINPUT
> > +		report=$(cat ./$TMPINPUT | $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > +	fi
> 
> All calls to DPDK_CHECKPATCH_PATH can be moved to one call after the "fi".
> Just need to set TPMINPUT=$1 in first case (and not remove TPMINPUT
> in this case).
> 
Sure.

> > +	if [ $? -ne 0 ]
> > +	then
> 
> For consistency, the style in this file is:
> 	if [ $? -ne 0 ] ; then
> 
> > +		$verbose || printf '\n### %s\n\n' "$3"
> > +		printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> > +		ret=1
> > +	fi
> > +
> > +	! $verbose || printf '\nChecking API additions/removals:\n'
> > +
> > +	if [ -n "$1" ] ; then
> > +		report=$($VALIDATE_NEW_API "$1")
> > +	elif [ -n "$2" ] ; then
> > +		report=$( cat ./$TMPINPUT | 
> > +			$VALIDATE_NEW_API -)
> > +	else
> > +		report=$(cat ./$TMPINPUT | $VALIDATE_NEW_API -)
> > +	fi
> 
> Same as above, it can be simplified by only one line for all cases.

Fine, at this point, I don't know when I'll get to this though, its pretty busy
here at the moment.

> 
> 
> 
>