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

Lukas Bulwahn lukas.bulwahn at gmail.com
Sat Aug 1 06:04:17 UTC 2020



On Thu, 30 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.
>
----
> Improve this by ensuring the patch to have originated from a script by
> checking the extension. However, there are 120 files in the kernel source
> that do not have an extension but have a shebang in line 1.
>

Well, you are not doing that anymore. So the commit message is wrong.

Maybe, you simply say:

  - what is the problem?
  - what are the alternatives considered?
  - what did you evaluate on these two alternatives?
  - why did you decide the one you chose?

If you structure it that way, it is easier to follow your thoughts.

> An alternate approach is to check for permissions of the file. There are
> 53 files in kernel source that have executable flag set but don't have a
> shebang in the first line. These files could be patched suitably so that
> they don't issue false warnings. Hence, choose this approach.
>

I would not mention the potential follow-up work in this commit.
You can say that:

At first sight on these 53 files, it seems that these files have a wrong 
file permission set or could be reasonably extended with a shebang and 
license information.
Hence, further clean-up in the repository would make this heuristics work 
even more precisely.

> Reduce SPDX license false warnings on patches by checking the permissions
> on the file.
> 
> Signed-off-by: Mrinal Pandey <mrinalmni at gmail.com>
> ---
>  scripts/checkpatch.pl | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4c820607540b..c55595113499 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2368,6 +2368,7 @@ sub process {
>  
>  	# Trace the real file/line as we go.
>  	my $realfile = '';
> +	my $realfile_perms = '';
>  	my $realline = 0;
>  	my $realcnt = 0;
>  	my $here = '';
> @@ -2555,11 +2556,13 @@ sub process {
>  		if ($line =~ /^diff --git.*?(\S+)$/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +			$realfile_perms = `stat -c "%a" $realfile`;

Again, this is totally wrong!

We already noted that you can only use the information provided in the 
patch file.

Is that information on file permissions provided with a patch?
Where is it provided? Find out and then parse that information.

>  			$in_commit_log = 0;
>  			$found_file = 1;
>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +			$realfile_perms = `stat -c "%a" $realfile`;
>  			$in_commit_log = 0;
>  
>  			$p1_prefix = $1;
> @@ -3166,6 +3169,9 @@ sub process {
>  		}
>  
>  # check for using SPDX license tag at beginning of files
> +		if ($realfile_perms =~ /[7531]\d{0,2}/) {
> +			$checklicenseline = 2;
> +		}

That check looks good. I assume you copied this expression from another 
place in checkpatch.pl.


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


More information about the Linux-kernel-mentees mailing list