Re: [PATCH i-g-t] CONTRIBUTING: Add coding style guidelines

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 20/03/24 - 16:22, Kamil Konieczny a écrit :
> Hi Louis,
> On 2024-03-15 at 17:08:37 +0100, Louis Chauvet wrote:
> > As explained in [1], the submitted patches should follow the same style as
> > the Linux kernel and must be checked against checkpatch.pl, so add this
> > recommendation in the CONTRIBUTING.md file.
> > 
> > [1]: https://lore.kernel.org/all/20240314170643.77vzqqoie2otyu4w@kamilkon-desk.igk.intel.com/
> > 
> 
> There is code style mensioned at line 13, grep -n -A1 kernel CONTRIBUTING.md
> gives (in second match):

I missed it, I think it's because I read this section too fast and jumped 
to "Sending Patches".
 
> 13:- The code should follow kernel coding style:
> 14:  https://www.kernel.org/doc/html/latest/process/coding-style.html
> 
> So maybe place it after that line?

Yes, I can move the checkpatch sentence+command here.

But as the "The Code" section deals in more detail with the internal 
workings of the igt, I think it's better to keep something in "Sending 
Patches", like:

 - Your patch must adhere the code style defined in "The Code".

This way, the section "Sending Patches" can serve as a "checklist" (accept 
the licence, use igt prefix, check the style).

Additionally, as the documentation is automaticaly build, maybe it 
might be useful to include the following point:

 - If you're add new tests, please ensure that you've written the 
   documentation before submitting by running:

      ninja -C build igt-gpu-tools-doc

   Otherwise, auto-builders will complain if the documentation is missing.


I have a question regarding the documentation. [1] explains that 
igt_describe is deprecated, but CONTRIBUTION.md and igt_describe() doc 
don't explain the "new" testplan documentation. Perhaps I could revise 
"The Code" section to include:

 - All new test must be documented in two ways:
     - A call to igt_describe(), just before the test. This should provide 
       a concise description of the test's purpose, and it will be 
       displayed during execution.
     - A testplan comment at the top of the file. This should 
       provide further explanation of the test's functionality and is 
       utilized in reporting tools.
   Please refer to [1] for more details

[1]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/docs/test_documentation.md?ref_type=heads

Thanks,
Louis Chauvet

> Regards,
> Kamil
> 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > 
> > To: igt-dev@lists.freedesktop.org
> > Cc: miquel.raynal@bootlin.com
> > Cc: thomas.petazzoni@bootlin.com
> > Cc: jeremie.dautheribes@bootlin.com
> > Cc: arthurgrillo@riseup.net
> > Cc: seanpaul@google.com
> > Cc: marcheu@google.com
> > Cc: nicolejadeyee@google.com
> > Cc: kamil.konieczny@linux.intel.com
> > ---
> > To: igt-dev@lists.freedesktop.org
> > Cc: miquel.raynal@bootlin.com
> > Cc: thomas.petazzoni@bootlin.com
> > Cc: jeremie.dautheribes@bootlin.com
> > Cc: arthurgrillo@riseup.net
> > Cc: seanpaul@google.com
> > Cc: marcheu@google.com
> > Cc: nicolejadeyee@google.com
> > Cc: kamil.konieczny@linux.intel.com
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  CONTRIBUTING.md | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> > index 2ab8a48f6ae9..ec8debb4091e 100644
> > --- a/CONTRIBUTING.md
> > +++ b/CONTRIBUTING.md
> > @@ -78,6 +78,8 @@ Sending Patches
> >  - Changes to the testcases are automatically tested. Take the results into
> >    account before merging.
> >  
> > +- The coding rules are the same as for the Linux kernel. To check for common
> > +  style problems, you need to use the kernel's checkpatch.pl script.
> >  
> >  Commit Rights
> >  -------------
> > 
> > ---
> > base-commit: ed47dd9fb6b5be6c88513c2816f0f7cfdd0e02f6
> > change-id: 20240315-add_coding_style-b755454de958
> > 
> > Best regards,
> > -- 
> > Louis Chauvet <louis.chauvet@bootlin.com>
> > 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com