Re: [dpdk-dev] [PATCH v4] devtools: add new SPDX license compliance checker

["Gaëtan Rivet" <grive@u256.net>]

Hi Thomas, Stephen,

On 11/06/20 23:39 +0200, Thomas Monjalon wrote:
> 24/02/2020 22:01, Stephen Hemminger:
> > +tmpfile=$(mktemp)
> 
> Please check how other temp files are created in other scripts
> for consitency.
> 
> > +    git grep -L SPDX-License-Identifier -- \
> > +	':^.git*' ':^.ci/*' ':^.travis.yml' \
> > +	':^README' ':^MAINTAINERS' ':^VERSION' ':^ABI_VERSION' \
> > +	':^*/Kbuild' ':^*/README' \
> > +	':^license/' ':^doc/' ':^config/' ':^buildtools/' \
> 
> I think doc/ should be part of the license check,
> same for buildtools/.
> 
> > +	':^*.cocci' ':^*.abignore' \
> > +	':^*.def' ':^*.map' ':^*.ini' ':^*.data' ':^*.cfg' ':^*.txt' \
> > +	> $tmpfile
> > +
> > +    errors=0
> > +    while read -r line
> > +    do $quiet || echo $line
> > +       errors=$((errors + 1))
> 
> I'm surprised this works for you.
> In general, "while" creates a subshell which makes impossible
> updating a variable.
> I recommend using "for" with IFS=$'\n'.
> 

No it should work, while will only spawn a subshell if you use a pipe with it.
Ex:

   err=0; while true; do err=$((err + 1)); done            # $err changes

   err=0; yes | while read -r y; do err=$((err + 1)); done # $err is always 0

Using for could be an issue, as you are then limited by the number of
parameters allowed. Additionally, the script is written for POSIX shell,
using $'\n' is forbidden and the POSIX equivalent is very ugly:

   IFS="$(printf '%b_' '\n')"; IFS="${IFS%_}" # protect trailing \n

> > +    done < $tmpfile
> > +}
> > +
> > +check_boilerplate() {
> > +    if $verbose ; then
> > +	echo
> > +	echo "Files with redundant license text"
> > +	echo "---------------------------------"
> > +    fi
> > +
> > +    git grep -l Redistribution -- \
> > +	':^license/' ':^/devtools/check-spdx-tag.sh' |
> > +	while read line

Missing a -r to read here: https://www.shellcheck.net/wiki/SC2162
Generally, best to use shellcheck when writing a script, especially if
using /bin/sh.

> > +	do $quiet || echo $line
> > +	   warnings=$((warnings + 1))
> > +	done
> 
> Same comment about "while" subshell.
> 
> > +
> > +    warnings=0
> > +    while read -r line
> > +    do $quiet || echo $line
> > +       warnings=$((errors + 1))
> 
> Here too
> 
> > +    done < $tmpfile
> > +}
> 
> [...]
> > +Each file must begin with a special comment containing the
> > +`Software Package Data Exchange (SPDX) License Identfier <https://spdx.org/using-spdx-license-identifier>`_.
> 
> Typo: Identifier
> 
> > +
> > +Generally this is the BSD License, except for code granted special exceptions.
> 
> Is a verb missing?
> 
> > +The SPDX licences identifier is sufficient, a file should not contain
> > +an additional text version of the license (boilerplate).
> 
> 
> 

-- 
Gaëtan