[Linux-kernel-mentees] [PATCH] checkpatch: Fix SPDX license check for script files
Lukas Bulwahn
lukas.bulwahn at gmail.com
Sun Jul 26 17:09:55 UTC 2020
On Sun, 26 Jul 2020, Mrinal Pandey wrote:
> On 20/07/26 01:09PM, Lukas Bulwahn wrote:
> >
> >
> > On Sun, 26 Jul 2020, Mrinal Pandey wrote:
> >
> > >
> > >
> > > On Sun, Jul 26, 2020 at 11:43 AM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
> > >
> > >
> > > On Sat, 25 Jul 2020, Mrinal Pandey wrote:
> > >
> > > > In all the script files(except yaml), SPDX license identifier is expected
> > >
> > > Why do believe a yaml file is a script file? Where did you read that?
> > >
> > > To me, this reads like:
> > >
> > > All fish(except zebras) live in water...
> > >
> > > I think you can just drop that note on yaml.
> > >
> > > > on the second line, the first line being the shebang. Sometimes, the
> > > > diff content includes the SPDX licensing information but excludes the
> > > > shebang when a commit is made to a script file.
> > > >
> > >
> > > You can be more explicit here. Sometimes? You can exactly say when and how
> > > that happens.
> > >
> > > > This can be seen in the commits like 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.
> > > >
> > > > The existing logic looks for a shebang in the patch/file being checked
> > > > and if a shebang is encountered it directs checkpatch to expect
> > > > license on the second line by setting `$checklicenseline` to `2`.
> > > >
> > >
> > > You can drop those `quotes` around variable names and numbers.
> > >
> > > This can be written shorter:
> > > Currently, if checkpatch find a shebang in line 1, it expects the
> > > license identifier in line 2 instead of line 1.
> > >
> > > > However, this approach doesn't work when we don't have a shebang in the
> > > > patch and `$checklicensline` continues to be `1` in this case leading to
> > > > the unnecessary warning.
> > > >
> > >
> > > However, this only works when both line 1 and 2 are part of the patch.
> > >
> > > > Fix this by setting `$checklicenseline` to `2` by checking if the patch
> > > > comes from a script for the cases when the shebang doesn't appear in the
> > > > patch.
> > > >
> > >
> > > You are not really fixing it. You are improving the heuristics.
> > >
> > > So:
> > >
> > > Reduce the false warnings for such patches on scripts, by checking file
> > > extension.
> > >
> > > Alternatively, you could actually also check the file permissions of the
> > > file.
> > >
> >
> > First, please fix your email client. It is broken. If you want to work in
> > the kernel community, your email client needs to work as defined in the
> > kernel documentation.
> >
> > Read the kernel documentation on email clients and fix your client.
>
> Sir,
>
> I apologize if this has been causing trouble lately.
> I have been using neomutt for sending patches and Gmail for conversation.
> Could you please let me know once the issues you have seen with my mailing
> client so that I can address them?
It seems that Gmail does not work... look at the raw emails in email
archives.
> >
> > >
> > > There are 577 executable files in the kernel, out of which 53 don't have a shebang in the first line.
> > >
> > > Moreover, in these 53 files, 8 should have definitely had it as they are .py or .sh files.
> >
> > How about fixing those eight?
>
> Okay, I would send patches to fix this.
>
> >
> > > The other files are .dts, .dtsi, .S, .h, .c files, scripts/atomic/atomics.tbl file, Documentation/Changes file
> >
> > It seems wrong that those have an executable flag in the first place.
> >
> > That could be fixed as well.
>
> Is there anything I can do to fix this?
Send patches for those changes.
> >
> > > and all the files
> > > inside scripts/atomic/fallbacks that are without extension.
> > > If I am correct none of these should have a shebang but I am not very sure about files inside scripts/atomic/fallbacks.
> > >
> >
> > Probably that whole atomics stuff needs some documentation and cleanup.
>
> It would be great if we could do this.
>
> Thank you.
> >
> > > If we check for file extension we warn wrongly for 120 files iff shebang doesn't appear in the patch but if we check for
> > > permissions we would warn wrongly for 53 files. I think this is better than checking for extension.
> > > Should we use this heuristic in the new patch I send?
> > >
> >
> > Yes, go ahead a rework your patch.
> >
> >
> > Lukas
> >
> > > Thank you.
> > >
> > >
> > > Improve the patch; if it looks good then, it could be suitable for the
> > > larger discussion on lkml.
> > >
> > >
> > > Lukas
> > >
> > > > Signed-off-by: Mrinal Pandey <mrinalmni at gmail.com>
> > > > ---
> > > > scripts/checkpatch.pl | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 4c820607540b..80dfa83ed0fb 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -3167,7 +3167,7 @@ sub process {
> > > >
> > > > # check for using SPDX license tag at beginning of files
> > > > if ($realline == $checklicenseline) {
> > > > - if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
> > > > + if ($rawline =~ /^[ \+]\s*\#\!\s*\// || $realfile =~ /.*\.\(py\|sh\|pl\|awk\|tc\)/) {
> > > > $checklicenseline = 2;
> > > > } elsif ($rawline =~ /^\+/) {
> > > > my $comment = "";
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > >
> > >
> > >
>
>
More information about the Linux-kernel-mentees
mailing list