From: Mrinal Pandey <mrinalmni@gmail.com>
To: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH] checkpatch: Fix SPDX license check for scripts
Date: Tue, 14 Jul 2020 11:05:21 +0530 [thread overview]
Message-ID: <CAD1=X6ktktiJ10f3sefs72_WfWLFEVUg85mvp9391=tsn7gULw@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2007132140160.11844@felia>
[-- Attachment #1.1: Type: text/plain, Size: 3910 bytes --]
On Tue, Jul 14, 2020 at 1:16 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:
>
>
> On Mon, 13 Jul 2020, Mrinal Pandey wrote:
>
> > In all the scripts, the SPDX license should be on the second line,
> > the first line being the "sh-bang", but checkpatch issues a warning
> > "Misplaced SPDX-License-Identifier tag - use line 1 instead" for the
> > scripts that have SPDX license in the second line.
> >
> > However, this warning is not issued when checkpatch is run on a file
> using
> > `-f` option. The case for files has been handled gracefully by changing
> > `$checklicenseline` to `2` but a corresponding check when running
> checkpatch
> > on a commit hash is missing.
> >
> > 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 commits which modified
> > a script file.
> >
> > This check is missing in checkpatch since commit a8da38a9cf0e
> > ("checkpatch: add test for SPDX-License-Identifier on wrong line #")
> > when the corresponding rule was first commited.
> >
> > Fix this by setting `$checklicenseline` to `2` when the diff content that
> > is being checked originates from a script, thus, informing checkpatch
> that
> > the SPDX license should be on the second line.
> >
> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> > ---
> > scripts/checkpatch.pl | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4c820607540b..bbffd0c4449d 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3218,6 +3218,9 @@ sub process {
> > next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> >
> > # check for using SPDX-License-Identifier on the wrong line number
> > + if ($realfile =~ /^scripts/) {
> > + $checklicenseline = 2;
> > + }
>
> I think this is somehow wrong here. The check for checklicenseline = 2
> looks very different above.
>
> Why does -f work and using a patch file not work?
>
Sir,
I am going to explain my observation based on file
`scripts/atomic/gen-atomic-fallback.sh` and
commit hash `37f8173dd849`.
If we are checking against the file, `checklicenseline` is set to 1 and
when `realline` is 1 the above
`if` block is triggered, then we check if this line is of the form `#!/`
using the regular expression
`^[ \+]\s*\#\!\s*\/`. If this is the case we set `checklicenseline` to `2`
informing checkpatch that it should
expect license on the second line and this works all fine for a file.
The `if` block below my proposed changes evaluates to false in this case
and thus it emits no false warning.
However, If we are checking a diff content, the above `if` block is not
triggered at all. This is
because `realline` stores the actual line number of the line we are
checking currently out of diff content.
This value is 2 because SPDX identifier is indeed at the second line in the
file but `checklicenseline` is still `1`.
`realline` will never become equal to 1 again and thus the above `if`
condition will never be true in this case.
Even if the above `if` block is triggered it would not update
`checklicenseline` to 2 as the regular expression
is not satisfied since we don't have sh-bang in diff content and just the
SPDX tag.
If we don't do this, the `if` block below evaluates to true when `realline`
is 2 and `checklicensline` is `1` leading
to the emission of a false warning.
So, what I did was to check if the diff content we are checking actually
comes from a script, if yes, we can set
`checklicenseline` to `2` to avoid this confusion.
Please let me know if this is reasonable.
Thank you.
>
> Lukas
>
>
> > if ($realline != $checklicenseline &&
> > $rawline =~ /\bSPDX-License-Identifier:/ &&
> > substr($line, @-, @+ - @-) eq "$;" x (@+ - @-)) {
> > --
> > 2.25.1
> >
> >
>
[-- Attachment #1.2: Type: text/html, Size: 5533 bytes --]
[-- Attachment #2: Type: text/plain, Size: 201 bytes --]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-07-14 5:35 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-13 9:57 [Linux-kernel-mentees] [PATCH] checkpatch: Fix SPDX license check for scripts Mrinal Pandey
2020-07-13 19:46 ` Lukas Bulwahn
2020-07-14 5:35 ` Mrinal Pandey [this message]
2020-07-14 6:03 ` Lukas Bulwahn
2020-07-16 5:15 ` Mrinal Pandey
2020-07-16 5:31 ` Lukas Bulwahn
2020-07-17 9:54 ` Mrinal Pandey
2020-07-17 11:47 ` Lukas Bulwahn
2020-07-19 6:27 ` Mrinal Pandey
2020-07-19 7:13 ` Lukas Bulwahn
2020-07-21 5:44 Mrinal Pandey
2020-07-22 10:20 ` Lukas Bulwahn
2020-07-24 7:02 ` Mrinal Pandey
2020-07-24 8:09 ` Lukas Bulwahn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD1=X6ktktiJ10f3sefs72_WfWLFEVUg85mvp9391=tsn7gULw@mail.gmail.com' \
--to=mrinalmni@gmail.com \
--cc=Linux-kernel-mentees@lists.linuxfoundation.org \
--cc=lukas.bulwahn@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).