Re: [Linux-kernel-mentees] Linux kernel checkpatch.pl mentorship

[Lukas Bulwahn <lukas.bulwahn@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