From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 08266A00BE; Fri, 12 Jun 2020 10:36:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1EAD4100C; Fri, 12 Jun 2020 10:36:57 +0200 (CEST) Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by dpdk.org (Postfix) with ESMTP id D96B823D for ; Fri, 12 Jun 2020 10:36:55 +0200 (CEST) X-Originating-IP: 86.246.31.132 Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 0D3E91BF213; Fri, 12 Jun 2020 08:36:54 +0000 (UTC) Date: Fri, 12 Jun 2020 10:36:49 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Thomas Monjalon Cc: Stephen Hemminger , dev@dpdk.org Message-ID: <20200612083649.cpe3m7tdd364ipme@u256.net> References: <20200129155907.20556-1-stephen@networkplumber.org> <20200224210130.672-1-stephen@networkplumber.org> <37903858.QyUTlBbKYA@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <37903858.QyUTlBbKYA@thomas> Subject: Re: [dpdk-dev] [PATCH v4] devtools: add new SPDX license compliance checker X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 `_. > > 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