From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 417A1C433DF for ; Fri, 17 Jul 2020 09:54:21 +0000 (UTC) Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0501320684 for ; Fri, 17 Jul 2020 09:54:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="tH7trWGD" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0501320684 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linux-kernel-mentees-bounces@lists.linuxfoundation.org Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id D033B86239; Fri, 17 Jul 2020 09:54:20 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bnGakJln0fNY; Fri, 17 Jul 2020 09:54:19 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 730B88626F; Fri, 17 Jul 2020 09:54:19 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 5EA74C07FF; Fri, 17 Jul 2020 09:54:19 +0000 (UTC) Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8C5B9C0733 for ; Fri, 17 Jul 2020 09:54:18 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 87C7D8625F for ; Fri, 17 Jul 2020 09:54:18 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id k1obua2Ymn8R for ; Fri, 17 Jul 2020 09:54:16 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-pj1-f68.google.com (mail-pj1-f68.google.com [209.85.216.68]) by fraxinus.osuosl.org (Postfix) with ESMTPS id CC3EE860C0 for ; Fri, 17 Jul 2020 09:54:16 +0000 (UTC) Received: by mail-pj1-f68.google.com with SMTP id b92so6131129pjc.4 for ; Fri, 17 Jul 2020 02:54:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=r+LVQfjdE16qYuKfTGqfiBa12IP+RUVx9WNtyqp1/Bw=; b=tH7trWGDOlIX6VLs2MstBmYWtSUnLKBBfWZi1z1xWkNqUTLxafS2Go83+Jgsv3FqKp iVsRbupdcetOP0W5mRqz1rw5pnQ7jV6AxFsuX0roZ9ujHeLWSwfLDXF4JfgbJGnO2gt+ /nylRHURfxYTJjsjfIfIOhYSnlUBY4eUWeoE6oMukXyvywFO8RBeuMIHoBtW3jjCGlo1 2H3bHAF3ayL+Qb7QQflyq7dRIZn59YID9bFd8azqTtHzItS6dGx8wZK0nfJZ6jNUXgHt yadCJM2yyU8VIAm0lOTLlbsdANiVGRlixVGJRIQ2Wloq/Lkv8FpeBQ0TvfA/F1DI03F4 2wPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=r+LVQfjdE16qYuKfTGqfiBa12IP+RUVx9WNtyqp1/Bw=; b=tHvea4Z525MRAk/Y+uUiIFpUMe+yY6XdsXysgvoRmTPapF78e4IISJu8vru3ECsWLk EIEawCtOm0cNBl/L73PO0xWneL9fLsfZ1RJMxSCxzWv1Wy3THU84rogIAsdVk8uyLovl FfW4hkkitkQxWoFXKZxYSqJXsz15UxyHtT6BOZeF776I2JHWASEI3NWpjkhWim1B1Rf1 dhrFcygfJ8SwTJDvJupFRY15bLMoF3YpEO+ss7MKtFy84T40Y3/HK6rwCh2mNPYXRUeP GgELiZ7oMluhdh3HxlaQOKN+r9udiM/Nj5YyvK9zi3HQZGoY+gEITrcp6N5KYbvlDC80 2tEw== X-Gm-Message-State: AOAM530HQ2FR6jkwKh3E2pqkNrF4cecW3UGy5vm8scLaqBiaxtLV+lDv uxUSF1d76aUa0NElwbX1JNQcmN19YZYDpAhMhSs= X-Google-Smtp-Source: ABdhPJyF92UICro4WX71c9dEsBixgxpSwEBY0vJoLsSVktmsoZUTX3N3Ecv8Dk7dzVaMwSNSlHiJHsv7wX9pJRYHS3s= X-Received: by 2002:a17:90b:4c8b:: with SMTP id my11mr9116604pjb.153.1594979656200; Fri, 17 Jul 2020 02:54:16 -0700 (PDT) MIME-Version: 1.0 References: <20200713095740.mi3cnx7tccoetxgc@mrinalpandey> In-Reply-To: From: Mrinal Pandey Date: Fri, 17 Jul 2020 15:24:04 +0530 Message-ID: To: Lukas Bulwahn , Shuah Khan , Linux-kernel-mentees@lists.linuxfoundation.org Subject: Re: [Linux-kernel-mentees] [PATCH] checkpatch: Fix SPDX license check for scripts X-BeenThere: linux-kernel-mentees@lists.linuxfoundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============2172402676386029050==" Errors-To: linux-kernel-mentees-bounces@lists.linuxfoundation.org Sender: "Linux-kernel-mentees" --===============2172402676386029050== Content-Type: multipart/alternative; boundary="000000000000d4f63905aaa023ec" --000000000000d4f63905aaa023ec Content-Type: text/plain; charset="UTF-8" On Thu, Jul 16, 2020, 11:01 Lukas Bulwahn wrote: > > > On Thu, Jul 16, 2020 at 7:15 AM Mrinal Pandey wrote: > >> >> >> On Tue, Jul 14, 2020 at 11:33 AM Lukas Bulwahn >> wrote: >> >>> >>> >>> On Tue, 14 Jul 2020, Mrinal Pandey wrote: >>> >>> > >>> > >>> > On Tue, Jul 14, 2020 at 1:16 AM Lukas Bulwahn >>> 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 >>> > > --- >>> > > 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, maybe this whole logic needs to be reworked. If you do not know the >>> first line, you need to have a different criteria in the first place >>> to determine if you expect the license tag in the first or the second, >>> e.g., the file extension, and then checking line 1 for a shebang is just >>> sanity checking. If it is of a specific file extension, you know line 1 >>> and it is not a shebang, that is probably worth noting as a different >>> recommendation in checkpatch.pl anyway. >>> >> >> Sir, >> >> When we know the first line, i.e. we are running checkpatch against a >> file, the existing logic >> works fine. We probably don't want to induce any changes there. >> >> > Why not? Do you think we would break things there? Then we should not > touch the code at all. > Do you think we cannot test it properly after the change? Then we should > think about how we make a proper regression test suite for that. > Sir, No, breaking code or not being able to test is not why I suggest this. I feel that the existing logic handles the case of "Improper or malformed SPDX tag" and "Misplaced SPDX tag" for files i.e. when the first line is known. Anyway, the logic for "Misplaced SPDX tag" is written as a different rule. We just need to add in the logic for patches there. I tried to do this by checking for the scripts directory which was wrong. If I check instead for the file being a script that would make much more sense. Please let me know if you suggest something else. > > But when we don't know the first line, if am not wrong, it would go >> somewhat like: >> if (the file is a script) { >> if (the first line is shebang) { >> if (the second line is SPDX) { >> All good >> } else { >> Issue a misplaced or missing SPDX tag warning >> } >> } else { >> Issue a missing shebang warning >> } >> } else { >> if (the first line is SPDX) { >> All good >> } else { >> Issue a misplaced or missing SPDX tag warning >> } >> } >> >> > Basically agree, but that logic applies when you know the first line as > well (and only, right?). What if you do not know the first line, how would > you check "the first line is shebang" if you do not know the first line? > > > The missing shebang warning probably needs to go elsewhere in the whole > script. > By not knowing the first line I mean to say that the first line doesn't show up in diff content of the patch but what if we open the file at that point in the commit history and check for the first line to be a shebang? Would it be okay to do that? Once we check the first line we can then continue as suggested. > > > >> Lukas >>> >>> >>> > 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. >>> > >>> >>> Why would you think that scripts are only in scripts? >>> >>> How about first listing all files where the SPDX tag is in line 2 in the >>> current repository, e.g., v5.8-rc5? >>> >>> Then, we look at that list and determine a suitable criteria for looking >>> in line 2 for the SPDX tag. >>> >> >> Yes, the scripts are not only in scripts. I have listed all the files >> where the SPDX tag should be >> on the second line. I've attached the list for reference. We should >> probably be checking the file >> extension to determine if the tag needs to be on the second line or not. >> The documentation says the SPDX tag should be present in all source >> files. Do these source files include >> Documentation files too? >> >> > How did you create that list? > Agree (if the way you created that list makes sense). File extension seems > to cover all cases, and checking for the directory 'scripts' does not. > > I issued the command `find . -regex ".*\.\(py\|sh\|pl\)"` to make this list. I should have included awk, YAML and tc files too since they are scripts too. We might also add a further sanity check in checkpatch.pl if someone adds > an executable file that is not with extension sh, pl, or py. > Yes, this check should be present. Thank you. > > Lukas > >> --000000000000d4f63905aaa023ec Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Jul 16, 2020, 11:01 Lukas Bul= wahn <lukas.bulwahn@gmail.com> wrote:


On Thu, Jul 16, 202= 0 at 7:15 AM Mrinal Pandey <mrinalmni@gmail.com> wr= ote:


On Tue, Jul 14, 2020 at 11:33 AM Lukas Bulwahn &= lt;lukas.bulwahn@gmail.com> wrote:


On Tue, 14 Jul 2020, Mrinal Pandey wrote:

>
>
> On Tue, Jul 14, 2020 at 1:16 AM Lukas Bulwahn <luka= s.bulwahn@gmail.com> wrote:
>
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0On Mon, 13 Jul 2020, Mrinal Pandey wrote: >
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> In all the scripts, the SPDX license sh= ould be on the second line,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> the first line being the "sh-bang&= quot;, but checkpatch issues a warning
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> "Misplaced SPDX-License-Identifier= tag - use line 1 instead" for the
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> scripts that have SPDX license in the s= econd line.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> However, this warning is not issued whe= n checkpatch is run on a file using
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> `-f` option. The case for files has bee= n handled gracefully by changing
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> `$checklicenseline` to `2` but a corres= ponding check when running checkpatch
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> on a commit hash is missing.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> I noticed this false positive while run= ning checkpatch on the set of
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> commits from v5.7 to v5.8-rc1 of the ke= rnel on the commits which modified
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> a script file.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> This check is missing in checkpatch sin= ce commit a8da38a9cf0e
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> ("checkpatch: add test for SPDX-Li= cense-Identifier on wrong line #")
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> when the corresponding rule was first c= ommited.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> Fix this by setting `$checklicenseline`= to `2` when the diff content that
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> is being checked originates from a scri= pt, thus, informing checkpatch that
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> the SPDX license should be on the secon= d line.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> ---
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0 scripts/checkpat= ch.pl | 3 +++
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0 1 file changed, 3 insertions(+) >=C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> diff --git a/scripts/c= heckpatch.pl b/scripts/checkpatch.pl
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> index 4c820607540b..bbffd0c4449d 100755=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> --- a/scripts/checkpat= ch.pl
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> +++ b/scripts/checkpat= ch.pl
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> @@ -3218,6 +3218,9 @@ sub process {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0 # check for using SPDX-License-Id= entifier on the wrong line number
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0if ($realfile =3D~ /^scripts/) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 $checklicenseline =3D 2;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0I think this is somehow wrong here. The chec= k for checklicenseline =3D 2
>=C2=A0 =C2=A0 =C2=A0 =C2=A0looks very different above.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Why 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 an= d 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.<= br> > The `if` block below my proposed changes evaluates to false in this ca= se and thus it emits no false warning.
>
> However, If we are checking a diff content, the above `if` block is no= t triggered at all. This is
> because `realline` stores the actual line number of the line we are ch= ecking currently out of diff content.
> This value is 2 because SPDX identifier is indeed at the second line i= n 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 `checkli= censeline` to 2 as the regular expression
> is not satisfied since we don't have sh-bang in diff content and j= ust 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, maybe this whole logic needs to be reworked. If you do not know the first line, you need to have a different criteria in the first place
to determine if you expect the license tag in the first or the second,
e.g., the file extension, and then checking line 1 for a shebang is just sanity checking. If it is of a specific file extension, you know line 1
and it is not a shebang, that is probably worth noting as a different
recommendation in checkpatch.pl anyway.

Sir,

When we know the f= irst line, i.e. we are running checkpatch against a file, the existing logi= c
works fine. We probably don't want to induce any changes th= ere.


=
Why not? Do you think we would break things there? Then we should not = touch the code at all.
Do you think we cannot test it properly af= ter the change? Then we should think about how we make a proper regression = test suite for that.

Sir,

No, breaking code or not being able to test is not why I sug= gest this. I feel that the existing logic handles the case of
"Improper or malformed SPDX tag" and "Misplac= ed SPDX tag" for files i.e. when the first line is known. Anyway, the = logic
for "Misplaced SPDX tag" is written as a differen= t rule. We just need to add in the logic for patches there.
= I tried to do this by checking for the scripts directory which was wrong. I= f I check instead for the file being a script that would make much more sen= se.
Please let me know if you suggest something = else.

But when we don't know the = first line, if am not wrong, it would go somewhat like:
if (t= he file is a script) {
=C2=A0=C2=A0=C2=A0 if (the first line is s= hebang) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (the seco= nd line is SPDX) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 All good
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 } else {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 Issue a misplaced or missing SPDX tag warning
<= /div>
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
=C2=A0= =C2=A0=C2=A0 } else {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 Issue a missing shebang warning
=C2= =A0=C2=A0=C2=A0 }
} else {
=C2=A0=C2=A0=C2=A0 if (t= he first line is SPDX) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 All good
=C2=A0=C2=A0=C2=A0 } else {
=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Issue a misplaced or missing SPDX tag war= ning
=C2=A0=C2=A0=C2=A0 }
}


Basically agree, but that logic = applies when you know the first line as well (and only, right?). What if yo= u do not know the first line, how would you check "the first line is s= hebang" if you do not know the first line?

The missing shebang warning probably needs to go elsewhere in = the whole script.

By not = knowing the first line I mean to say that the first line doesn't show u= p in diff content of the patch but
what if we open the file a= t that point in the commit history and check for the first line to be a she= bang?
Would it be okay to do that? Once we check the first line w= e can then continue as suggested.
<= div dir=3D"ltr">

=C2=A0
=
Lukas


> So, what I did was to check if the diff content we are checking actual= ly comes from a script, if yes, we can set
> `checklicenseline` to `2` to avoid this confusion.
>

Why would you think that scripts are only in scripts?

How about first listing all files where the SPDX tag is in line 2 in the current repository, e.g., v5.8-rc5?

Then, we look at that list and determine a suitable criteria for looking in line 2 for the SPDX tag.

Yes, the sc= ripts are not only in scripts. I have listed all the files where the SPDX t= ag should be
on the second line. I've attached the list for r= eference. We should probably be checking the file
extension to de= termine if the tag needs to be on the second line or not.
The doc= umentation says the SPDX tag should be present in all source files. Do thes= e source files include
Documentation files too?


How did you create that lis= t?
Agree (if the way you created that list makes sense). File ext= ension seems to cover all cases, and checking for the directory 'script= s' does not.

I issued= the command `find . -regex ".*\.\(py\|sh\|pl\)"` to make this li= st. I should have included awk, YAML and tc files too since they are script= s too.

=
We might also add a further sani= ty check in checkpatch.pl if someone adds an executable file that= is not with extension sh, pl, or py.
Yes, this check should be present.

Thank you.

Lukas
--000000000000d4f63905aaa023ec-- --===============2172402676386029050== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees --===============2172402676386029050==--