[Linux-kernel-mentees] [PATCH] checkpatch: Improve SPDX license check for script files

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Jul 27 20:42:00 UTC 2020



On Mon, 27 Jul 2020, Mrinal Pandey wrote:

> In all the script files, SPDX license identifier is expected on the second
> line, the first line being the shebang.
> 
> The diff content includes the SPDX licensing information but excludes the
> shebang when a change is made to a script file in commit 37f8173dd849
> ("locking/atomics: Flip fallbacks and  instrumentation") and commit
> 075c8aa79d54 ("selftests: forwarding: tc_actions.sh: add matchall mirror
> test"). In these cases checkpatch issues a false positive warning:
> "Misplaced SPDX-License-Identifier tag - use line 1 instead".
> 
> I noticed this false positive, while running checkpatch on the set of
> commits from v5.7 to v5.8-rc1 of the kernel, on the said commits.
> This false positive exists in checkpatch since commit a8da38a9cf0e
> ("checkpatch: add test for SPDX-License-Identifier on wrong line #")
> when the corresponding rule was first added.
> 
> Currently, if checkpatch finds a shebang in line 1, it expects the
> license identifier in line 2. However, this doesn't work when a shebang
> isn't found on the line 1.
> 
> Reduce the number of false warnings for such patches on scripts by
> checking the permissions of the file.
>

I would certainly explain the different options you considered and why you 
finally decided for the solution you propose.
 
> Signed-off-by: Mrinal Pandey <mrinalmni at gmail.com>
> ---
>  scripts/checkpatch.pl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4c820607540b..695300839f48 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3166,6 +3166,10 @@ sub process {
>  		}
>  
>  # check for using SPDX license tag at beginning of files
> +		my $filepermissions = `stat -c "%a" $realfile`;

I fear this is now placed somewhere, so you are running stat on every line 
again and again and ...

How about determining permissions when the realfile is set?

I bet that is already done anyway by getting the information from the 
patch, as you cannot assume the $realfile to exist.

You only can use the information in the patch file.

Read more code in checkpatch.pl.

> +		if (int(substr($filepermissions, 0, 1)) % 2) {

That is a really complicated way to express it. I will bet you will find a 
better way already implemented in checkpatch.pl.

> +			$checklicenseline = 2;
> +		}
>  		if ($realline == $checklicenseline) {
>  			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
>  				$checklicenseline = 2;
> -- 
> 2.25.1


I guess we will need another iteration of this patch.

Lukas 


More information about the Linux-kernel-mentees mailing list