Mentorship report first draft

Dwaipayan Ray dwaipayanray1 at gmail.com
Sat Mar 20 16:04:17 UTC 2021


On Sat, Mar 20, 2021 at 7:33 PM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
>
> Dwaipayan, see my comments below inline.
>
> Fix those minor issues, give the report to somebody else for another
> review (and get some more comments), and then I think we are all good
> to publish this.

Sure I will fix them. How are we going to publish it? We just share the link it
is hosted on or something else?

>
> Lukas
>
> > ---
> > title: "Linux Kernel checkpatch.pl mentorship"
> > date: 2021-03-06T02:05:17+05:30
> > draft: false
> > ---
> >
> > ## Checkpatch - What is it?
> > [checkpatch.pl](https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl) is a trivial style checker for patches sent to the linux kernel.
> >
> > Why is it needed you say?</br>
> > I did an analysis on checkpatch warnings on about 50k commits from v5.4.</br>
> > (Find the full report {{% link checkpatch_out_50k.txt %}}here{{% /link %}})
> >
> > The top errors were then generated via the following shell command:
> > ```/bin/bash
> > cat checkpatch_out_50k.txt | grep -oP "(?:WARNING|ERROR|CHECK):(\w+)" \
> > | sort | uniq -c | sort -nr  | head -n6
> > ```
> >
> > Count of errors | Error Types | Error
> > ----------------|-------------|------
> > 41999 | <span style="color:green">CHECK</span> | CAMELCASE
> > 13629 | <span style="color:green">CHECK</span> | PARENTHESIS_ALIGNMENT
> > 11491 | <span style="color:orange">WARNING</span> | LONG_LINE
> > 8106 | <span style="color:green">CHECK</span> | SPACING
> > 6825 | <span style="color:orange">WARNING</span> | LEADING_SPACE
> > 4394 | <span style="color:green">CHECK</span> | OPEN_ENDED_LINE
> >
> > Hmm...The no. of stylistically incorrect bits still continue to exist and
>
> Drop the Hmm... or replace it by "As we can see,"
>
> Replace no. by number. (It is not good to abbreviate here.)
>
Okay will do.

> > are still merged into the kernel code. To make the kernel code more
> > stylistically coherent, we can:
> >
> > > - Create cleanup patches for the existing code
> > > - Make sure new patches are stylistically consistent
> >
> > Both of these are where checkpatch comes handy.
> >
> > ## Checkpatch in the patch submission process
> >
> > Before sending out the patches to the mailing list, it is recommended to
> > run checkpatch over the patches.
> >
> > In general, a Kernel developer would:
> >
> > > - Create the patch with his proposed changes.
> > > - Build check the patch, run additional tools like checkstack, sparse etc.
>
> maybe add "check for compiler warnings",
> checkstack does not exist anymore.
>

Oops the documentation seems to be outdated then.
https://www.kernel.org/doc/html/latest/process/submit-checklist.html
point no. 10
Will correct

> such as sparse and coccinelle
>
> > > - Run checkpatch.pl over the patches to eliminate all trivial style violations.
> > > - Send out the patch to the maintainers and mailing list.
> >
> >
> > The only catch here is that checkpatch is not always right. It is just a dumb
> > perl script. If the code looks better with checkpatch errors, let it be.
> >
>
> This is fine to say for the author or as direct advice to a developer, here
> I would phrase more neutral.
>
> > ## Drawbacks of checkpatch
> >
> > Checkpatch is only a static trivial style checker. It does not have the
> > capabilities of a full c based parser like [clang-format](https://www.kernel.org/doc/html/latest/process/clang-format.html). In this sense it is limited in
>
> write instead
> "a tool using a full-fledged C parser, such as clang-format"
>
> > what it can do. Still, it is highly valuable as a style checker tool and can
> > ease the work of maintainers and increase the chances of the patch being accepted
> > by the community.
> >
> > ## What did we do to improve it?
> >
> > Over the first 3 months of my mentorship period, I primarily focused on
>
> s/3/three/
>
> (write out numbers up to twelve).
>
> > eliminating false-positives from checkpatch as well as adding new rules.
> >
> s/false-positives/false positives/
> > Consider the following patch commit log:
> >
> > ```diff
> > #include asm/bitsperlong.h avoid further potential userspace pollution
> > by moving the #define of SHIFT_PER_LONG to bitops.h which is not
> > exported to userspace.
> > ```
> >
> > A kernel developer in 2020 would not see any checkpatch warning.
> >
> > A kernel developer in 2021 would see the following warning:
> > ```\bin\bash
> > WARNING: Commit log lines starting with '#' are dropped by git as comments
> > #82:
> > #include asm/bitsperlong.h avoid further potential userspace pollution
> > ```
> >
> > During the last 3 months I worked on two major features:
> >
>
> s/3/three/
>
> > > - Addition of a verbose mode to checkpatch
> > > - Addition of an external documentation to checkpatch
> >
> > For the documentation we decided to document all checkpatch message types,
> > their meanings and how to resolve them. This is still a work in progress.
> > Contributions are definitely welcome :-P.
> >
>
> Drop the :-P ; you are serious, we want to see contributions from others.
>
> > ### Verbose Mode
> >
> > This was one of the new additions we did to checkpatch.
> >
> > One can run checkpatch like this:
> >
> > ```/bin/bash
> > ./scripts/checkpatch.pl -v test.patch
> > ```
> >
> > All verbose descriptions can be seen by pairing the verbose option
> > with --list-types:
> >
> > ```/bin/bash
> > ./scripts/checkpatch.pl --list-types -v
> > ```
> >
> > ### Checkpatch Documentation
> >
> > The checkpatch documentation can be viewed from the kernel root
> > at `Documentation/dev-tools/checkpatch.rst`.
> >
>
> simply:
>  The checkpatch documentation is at `Documentation/dev-tools/checkpatch.rst`
>  in the kernel repository.
>
> > To build the documentation make sure you have configured sphynx
> > either in the system or in a python virtualenv. Next the kernel
> > doc can be built using:
> >
> > ```/bin/bash
> > make htmldocs
> > ```
> >
> > ## How to contribute to the documentation?
> >
> > The documentation is still a work in progress. Feel free to send
> > the patches to the kernel-doc mailing list, and add me (Dwaipayan
> > Ray <dwaipayanray1 at gmail.com>) and Lukas (Lukas Bulwahn <lukas.bulwahn at gmail.com>) for reviewing it.
> >
> > The documentation is writted in rst syntax. Refer
> > [here](http://openalea.gforge.inria.fr/doc/openalea/doc/_build/html/source/sphinx/rest_syntax.html) for a quick guide.
> >
> > Before sending out the mail, please always:
> >
>
> s/writted/written/
> s/mail/patch/
>
> > > - checkpatch.pl your changes
> > > - build check your patch (in this case check the generated html docs)
> > > - spell check your patch (this can be easily done through checkpatch
> > >   using --codespell flag if you already have the codespell dictionary)
> >
> > ## Acknowledgements
> >
> > During my mentorship period I learned a lot from my mentors.
> > I would like to thank Lukas for his constant support as my mentor,
> > Joe for his guidance with checkpatch and everyone in the community for being such
> > amazing people.
> >
>
> > The kernel contribution process may look old but it works. This is due to the dedication of everyone in the community, which I am glad to be a part of.
>
> I think you need to explain that this is something you learned in this
> mentorship, and that newcomers might think at first, but where you
> have been surprised.

Yes sure I will be elaborating a bit on these. I feel that I should be able
to motivate others to enter this process.

Thanks for the review Lukas. I will be making the necessary changes.

Thanks & Regards,
Dwaipayan.


More information about the Linux-kernel-mentees mailing list