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

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Aug 3 10:51:21 UTC 2020



On Mon, 3 Aug 2020, Mrinal Pandey wrote:

> On 20/08/01 08:04AM, Lukas Bulwahn wrote:
> > 
> > 
> > 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.
> 
> Sir,
> 
> I tried to improve the patch and the commit message. Please let me know
> if I can further improve on it.
> > 
> > >  			$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.
> 
> Yes, I copied this check from line 2649 in checkpatch.pl.

Just a hint:

A line number has no meaning if you do not say the commit sha of the 
repository you are looking at.

I simply ignored your sentence anyway and just read yes; so, it does not 
matter. I assume you know what you are doing.

Lukas

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


More information about the Linux-kernel-mentees mailing list