linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
From: Lukas Bulwahn <lukas.bulwahn@gmail.com>
To: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] Linux kernel checkpatch.pl mentorship
Date: Mon, 14 Sep 2020 15:49:04 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.21.2009141535010.17999@felia> (raw)
In-Reply-To: <CABJPP5D87ZVTfZ4QHxp5vweTN16LQCy+rniq-uDVCvKkbYRDNg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 4697 bytes --]



On Mon, 14 Sep 2020, Dwaipayan Ray wrote:

> 
> 
> 
> On Mon, Sep 14, 2020 at 10:47 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> >
> >
> > On Mon, 14 Sep 2020, Dwaipayan Ray wrote:
> >
> > >
> > >
> > > > You should also try to find a case where checkpatch.pl can be improved:
> > > >
> > > > - It is best if you look at the checkpatch.pl error/warning reports and
> > > > check if you can find a class of typical false positives in the reported
> > > > data. Then, think about how to make the pattern in checkpatch and propose
> > > > a patch to checkpatch.pl on this mailing list; with the evaluation data
> > > > backing that your patch really improves the situation. We will then test
> > > > and check your patch as well.
> > > >
> > > > - If you really cannot find a typical false positive, maybe you can take
> > > > the task from Ayush to fix checkpatch with git ranges?
> > > >
> > > Hi,I will try to find and evaluate the possibiltiy of such a case, and report my finding
> > > as soon as I find something substantial.
> > >
> > > I may also require some advice from you on the patch submission process.
> > >
> >
> > That is what a mentor is there for, but try hard to understand the
> > warnings yourself first. This is a mentorship, not a tutorial.
> >
> > Lukas
> Hi,
> I looked into the checkpatch.pl 's output I collected. And I have found some possible candidates
> but I would like your opinion on them.
> 
> The first one is double warnings for the same kind of coding style error.
> If an SPDX tag is in the following format: "// SPDX-License-Identifier: GPL-2.0",
> then checkpatch.pl generates two warnings:
> 
> 1) Improper SPDX comment style for ... , please use '/*' instead.
> 2) Missing or malformed SPDX-License-Identifier tag in line 1.
> 
> Example: commit 726721a51838
> Warnings generated:
> 
> WARNING:SPDX_LICENSE_TAG: Improper SPDX comment style for 'kernel/trace/trace_synth.h', please use '/*' instead
> #4041: FILE: kernel/trace/trace_synth.h:1:
> +// SPDX-License-Identifier: GPL-2.0
> 
> WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
> #4041: FILE: kernel/trace/trace_synth.h:1:
> +// SPDX-License-Identifier: GPL-2.0
> 
> In my opinion, only one warning should suffice if a '//' comment style is used in spdx tag. 
> But I would like to know your view on it.
> 
> ----------------------------------------------------------------------------------------------
> The second candidate is related to the following warning:
>

I do not think that this is a good candidate.
 
> WARNING:SPDX_LICENSE_TAG: DT binding documents should be licensed (GPL-2.0-only OR BSD-2-Clause)
> #110: FILE: Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml:1:
> +# SPDX-License-Identifier: GPL-2.0-only
> 
> In checkpath.pl line 3209:
>  $fixed[$fixlinenr] =~ s/SPDX-License-Identifier: .*/SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)/;
> 
> The desired fix is "(GPL-2.0-only OR BSD-2-Clause)" . Shouldn't the identifier be "GPL-2.0-only" 
> or "BSD-2-Clause", that is either of them? Or was the former the decided one?
>

Hmm, I do not think so. It is supposed to be dual-licensed with 
GPL-2.0-only and BSD-2-Clause. In SPDX, this is written exactly as 
described in the provided warning of checkpatch.pl.
  
> ----------------------------------------------------------------------------------------------
> The third candidate is related to the warning:
> 
> WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ...
> 
> I found several such commits in which the author had used different mail addresses in the 
> signed-off -by section, due to which this warning is generated.
> 
> An example is Commit dc5bdb68b5b3 .
> Git log show:
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> ....,.
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> This is infact a common scenario. I easily found another commit 207324a321a8.
> Git log shows:
> Author: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>
> ...
> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
> 
> This warning could be avoided or at least handled better.
> 
> 
> I would like to know if any of them can be worked on.
> 

This last one might be good to look into.

But what is your specific solution you have in mind?

There is a file .mailmap in the repository that allows some kind of 
mapping. Maybe that is helpful.

I suggest that you describe your proposed change in a clear way.
Then, we can discuss if that change is reasonable or not.


Maybe you can continue to look at further potential candidates, just to 
see if there is another type for improvement.

Best regards,

Lukas

[-- 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

  reply	other threads:[~2020-09-14 13:49 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CABJPP5CSa_QowD-O3_E5ndoQJvuBv=n2x5WG-acwQKE=rt5+Rw@mail.gmail.com>
     [not found] ` <alpine.DEB.2.21.2009110925160.9220@felia>
2020-09-12  9:09   ` Dwaipayan Ray
2020-09-12 11:03     ` Lukas Bulwahn
2020-09-12 12:08       ` Dwaipayan Ray
2020-09-12 12:21         ` Lukas Bulwahn
2020-09-13  8:16           ` Dwaipayan Ray
2020-09-13 11:05             ` Lukas Bulwahn
     [not found]               ` <CABJPP5BmRcC+OTSjuX_QrYononVq__DkhjGOgiKrP147MAXK+g@mail.gmail.com>
     [not found]                 ` <alpine.DEB.2.21.2009132015570.6806@felia>
2020-09-13 18:23                   ` Dwaipayan Ray
     [not found]                 ` <alpine.DEB.2.21.2009132010300.6806@felia>
2020-09-13 18:39                   ` Dwaipayan Ray
2020-09-14  5:17                     ` Lukas Bulwahn
2020-09-14 12:31                       ` Dwaipayan Ray
2020-09-14 13:49                         ` Lukas Bulwahn [this message]
2020-09-14 15:39                           ` Dwaipayan Ray
2020-09-14 18:32                             ` Lukas Bulwahn
2020-09-15 13:04                               ` Dwaipayan Ray
2020-09-16  7:01                                 ` Lukas Bulwahn
2020-09-17 13:09                                   ` Dwaipayan Ray
2020-09-17 13:41                                     ` Dwaipayan Ray
2020-09-17 14:18                                       ` Lukas Bulwahn
2020-09-17 14:43                                         ` Dwaipayan Ray
2020-09-17 15:03                                           ` Lukas Bulwahn
2020-09-17 14:13                                     ` 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=alpine.DEB.2.21.2009141535010.17999@felia \
    --to=lukas.bulwahn@gmail.com \
    --cc=dwaipayanray1@gmail.com \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --subject='Re: [Linux-kernel-mentees] Linux kernel checkpatch.pl mentorship' \
    /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

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).