* Xen checkpatch infrastructure design @ 2017-07-24 8:50 Juergen Gross 2017-07-24 11:44 ` Lars Kurth 0 siblings, 1 reply; 9+ messages in thread From: Juergen Gross @ 2017-07-24 8:50 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich, Lars Kurth On the Xen Developer Summit 2017 in Budapest we agreed to add a script to the Xen repository capable to test patches for style correctness, similar to checkpatch.pl of the Linux kernel. This is a first draft of the interface visible to users and developers. Problem to solve ---------------- Reviewing patches is requiring much bandwidth especially for maintainers of some core components of Xen. Often enough patches are not even following coding style of the modified component(s) resulting in the need to invest more time on the maintainer's side to request style related patch modifications. In order to reduce the effort spent on such pure mechanical issues of patches an interface for testing patches regarding style correctness is introduced. This script (similar to "checkpatch.pl" of the Linux kernel) will be usable by patch authors and reviewers to check for style errors. Unfortunately there is no single coding style in Xen. Depending on the source file one of several coding styles might apply: - Xen hypervisor style - Linux kernel style - libxl style - other styles? But even e.g. in the hypervisor some files are subject to the Linux kernel style as they are derived from Linux and might need patches from there, which should be easy to apply. Specification of Coding Style ----------------------------- As one patch might touch files with different code styles a single script is required being capable to test each touched file according to its style. This in turn requires a data base for defining the coding style of each source file. The easiest way to accomplish that is a file in the repository's root directory containing the necessary information. It will be named "STYLES" and contains lines in the format: style path where style specifies a coding style type (e.g. "linux", "xen", "libxl") and path specifies a path in the repository to which the style applies. A path can be either a directory or a file. When a directory is given all files in this directory (including any sub-directories) are of the specified coding style. It is possible to have multiple matching entries for a specific file, e.g.: xen ./xen linux ./xen/common/radix-tree.c In this case the most specific match will be used for determining the style type. Empty lines and lines starting with "#" in the STYLES file are being ignored by the patch checker. RFC: Design Considerations -------------------------- Remains the question how to design the style checker itself. It could be: (a) a monolithic script (perl, python, whatever) being capable of handling all the different coding styles (b) a main script checking the patch header and calling a code style specific script for each source file modified by the patch I believe (b) would be easier to maintain and to develop (we could start with the main script and add style specific scripts later). Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 8:50 Xen checkpatch infrastructure design Juergen Gross @ 2017-07-24 11:44 ` Lars Kurth 2017-07-24 12:46 ` Andrii Anisov 0 siblings, 1 reply; 9+ messages in thread From: Lars Kurth @ 2017-07-24 11:44 UTC (permalink / raw) To: Juergen Gross Cc: Lars Kurth, Wei Liu, Andrew Cooper, Ian Jackson, 'Jan Beulich', xen-devel, cardoe [-- Attachment #1.1: Type: text/plain, Size: 1505 bytes --] CC'ing Doug Goldstein as he has been reviewing some of Ishani's work (see below) Both Andy Cooper and Doug Goldstein had done some groundwork earlier on this topic > On 24 Jul 2017, at 09:50, Juergen Gross <jgross@suse.com> wrote: > > On the Xen Developer Summit 2017 in Budapest we agreed to add a > script to the Xen repository capable to test patches for style > correctness, similar to checkpatch.pl of the Linux kernel. > > This is a first draft of the interface visible to users and > developers. ... > RFC: Design Considerations > -------------------------- > Remains the question how to design the style checker itself. It could > be: > > (a) a monolithic script (perl, python, whatever) being capable of > handling all the different coding styles > (b) a main script checking the patch header and calling a code style > specific script for each source file modified by the patch > > I believe (b) would be easier to maintain and to develop (we could start > with the main script and add style specific scripts later). I don't have a view on this, but wanted to point the following docs which cover a little bit of groundwork on the subject, that can possibly be built upon https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edit <https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edit> http://markmail.org/message/tmdv2zzd4dvjin7v <http://markmail.org/message/tmdv2zzd4dvjin7v> Regards Lars [-- Attachment #1.2: Type: text/html, Size: 2482 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 11:44 ` Lars Kurth @ 2017-07-24 12:46 ` Andrii Anisov 2017-07-24 16:55 ` Iurii Artemenko 0 siblings, 1 reply; 9+ messages in thread From: Andrii Anisov @ 2017-07-24 12:46 UTC (permalink / raw) To: Lars Kurth, Juergen Gross, iurii_artemenko Cc: Lars Kurth, Wei Liu, Andrew Cooper, Ian Jackson, 'Jan Beulich', xen-devel, cardoe Dear Lars, Juergen, On our site we are working on the checkpatch-like script quite a while. Iurii Artemenko, our integrator, will get back with details by the end of the day. On 24.07.17 14:44, Lars Kurth wrote: > I don't have a view on this, but wanted to point the following docs > which cover a little bit of groundwork on the subject, that can > possibly be built upon > https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edit > http://markmail.org/message/tmdv2zzd4dvjin7v Lars, in the Ishani's document comments you mentioned some upstream contribution to the clang-format. Could you please point us to the mailing list thread where details are discussed. -- *Andrii Anisov* _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 12:46 ` Andrii Anisov @ 2017-07-24 16:55 ` Iurii Artemenko 2017-07-24 17:35 ` Lars Kurth 2017-07-24 18:08 ` Juergen Gross 0 siblings, 2 replies; 9+ messages in thread From: Iurii Artemenko @ 2017-07-24 16:55 UTC (permalink / raw) To: Juergen Gross Cc: Lars Kurth, Wei Liu, Lars Kurth, Andrew Cooper, Ian Jackson, Andrii Anisov, 'Jan Beulich', xen-devel, cardoe [-- Attachment #1.1: Type: text/plain, Size: 1610 bytes --] Hello Juergen, I've started to work on checkpatch-like python script. I make it based on clang-format-diff.py and it works as pre-commit hook. > The easiest way to accomplish that is a file in the repository's root > directory containing the necessary information. It will be named > "STYLES" and contains lines in the format: I will follow this approach. > Remains the question how to design the style checker itself. It could > be: > > (a) a monolithic script (perl, python, whatever) being capable of > handling all the different coding styles > (b) a main script checking the patch header and calling a code style > specific script for each source file modified by the patch It seems like specific script for style checking is not needed. Because clang-format tool does style checking by itself. All we need is just to provide appropriate coding style description file for each. Clang-format is a bit specific tool, so we can not specify explicitly file with coding style description. It just looks for a .clang-format file in one of a parent directories of a file being checked. As we got at least three coding-styles we have to substitute needed file in sources top directory for each check. It could be done by generating .clang-format file dynamically depending on style/path from the STYLES file. Another way could be using appropriate symlink on existing .clang-format file which is located somewhere in tools/clang-format/coding-style-file like: tools/clang-format/xen-style tools/clang-format/linux-style tools/clang-format/xl-style Regards Iurii [-- Attachment #1.2: Type: text/html, Size: 7509 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 16:55 ` Iurii Artemenko @ 2017-07-24 17:35 ` Lars Kurth 2017-07-26 16:08 ` Iurii Artemenko 2017-07-24 18:08 ` Juergen Gross 1 sibling, 1 reply; 9+ messages in thread From: Lars Kurth @ 2017-07-24 17:35 UTC (permalink / raw) To: Iurii Artemenko, Juergen Gross Cc: Wei Liu, Lars Kurth, Andrew Cooper, cardoe, Andrii Anisov, 'Jan Beulich', Ian Jackson, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 4914 bytes --] Hi Iurii, I wanted to double check, as we had previously looked into clang-format, and it showed some gaps with what it can be used for in Xen coding styles. Which is why we tried to get agreement - and got it - to upstream Xen related changes into clang–format == Indentation: ok == [+] Indent using spaces = ok with clang-format [+] Indent level = 4 spaces = ok with clang-format [+] Code within blocks is indented by one extra indent level = ok with clang-format [+] The enclosing braces of a block are indented the same as the code _outside_ the block = ok with clang-format == Whitespace: partly ok == [?] spread out logical statements = so no complete support (to do that we have add spaces before and after parenthesis but they apply to all others parenthesis as well) [?] spaces are placed between the keyword and the brackets surrounding the condition, between the 40 brackets and the condition itself = same as above [+] around binary operators (except the structure access operators, '.' and '->') = ok [-] There should be no trailing white space at the end of lines = no support == Line length: partly ok == [+] Lines should be less than 80 characters in length = ok with clang-format [-] Split at "sensible places" = no tool can do that [-] Trailing portions indented = no support in clang-format [-] User visible strings (e.g., printk() messages) should not be split = no support in clang-format (it splits everything) == Brackets: partly ok == [+] Usually placed on a line of their own = ok [-] Except for the do/while loop = no support [-] Brackets should be omitted for blocks with a single statement = no support == Comments: no support == [-] Only C style /* ... */ comments are to be used = no support [-] Multi-word comments should begin with a capital letter. = no support [-] Comments containing a single sentence may end with a full stop; = no support [-] Comments containing several sentences must have a full stop after each sentence = no support [-] Multi-line comment blocks should start and end with comment markers on separate lines and each line should begin with a leading '*'. = No support Emacs local variables = no support I was wondering how you deal with the gaps. I suppose these gaps could possibly be covered in clang-format-diff.py Of course this info may be out-of-date Regards Lars From: Iurii Artemenko <Iurii_Artemenko@epam.com<mailto:Iurii_Artemenko@epam.com>> Date: Monday, 24 July 2017 at 17:55 To: Juergen Gross <jgross@suse.com<mailto:jgross@suse.com>> Cc: Lars Kurth <lars.kurth@citrix.com<mailto:lars.kurth@citrix.com>>, Wei Liu <wei.liu2@citrix.com<mailto:wei.liu2@citrix.com>>, Andrew Cooper <Andrew.Cooper3@citrix.com<mailto:Andrew.Cooper3@citrix.com>>, Ian Jackson <Ian.Jackson@citrix.com<mailto:Ian.Jackson@citrix.com>>, 'Jan Beulich' <JBeulich@suse.com<mailto:JBeulich@suse.com>>, xen-devel <xen-devel@lists.xenproject.org<mailto:xen-devel@lists.xenproject.org>>, cardoe <cardoe@cardoe.com<mailto:cardoe@cardoe.com>>, Andrii Anisov <Andrii_Anisov@epam.com<mailto:Andrii_Anisov@epam.com>>, Lars Kurth <lars.kurth.xen@gmail.com<mailto:lars.kurth.xen@gmail.com>> Subject: Re: [Xen-devel] Xen checkpatch infrastructure design Hello Juergen, I've started to work on checkpatch-like python script. I make it based on clang-format-diff.py and it works as pre-commit hook. > The easiest way to accomplish that is a file in the repository's root > directory containing the necessary information. It will be named > "STYLES" and contains lines in the format: I will follow this approach. > Remains the question how to design the style checker itself. It could > be: > > (a) a monolithic script (perl, python, whatever) being capable of > handling all the different coding styles > (b) a main script checking the patch header and calling a code style > specific script for each source file modified by the patch It seems like specific script for style checking is not needed. Because clang-format tool does style checking by itself. All we need is just to provide appropriate coding style description file for each. Clang-format is a bit specific tool, so we can not specify explicitly file with coding style description. It just looks for a .clang-format file in one of a parent directories of a file being checked. As we got at least three coding-styles we have to substitute needed file in sources top directory for each check. It could be done by generating .clang-format file dynamically depending on style/path from the STYLES file. Another way could be using appropriate symlink on existing .clang-format file which is located somewhere in tools/clang-format/coding-style-file like: tools/clang-format/xen-style tools/clang-format/linux-style tools/clang-format/xl-style Regards Iurii [-- Attachment #1.2: Type: text/html, Size: 23540 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 17:35 ` Lars Kurth @ 2017-07-26 16:08 ` Iurii Artemenko 2017-07-26 16:50 ` Lars Kurth 0 siblings, 1 reply; 9+ messages in thread From: Iurii Artemenko @ 2017-07-26 16:08 UTC (permalink / raw) To: Lars Kurth, Juergen Gross Cc: Artem Mygaiev, Wei Liu, Lars Kurth, Andrew Cooper, cardoe, Andrii Anisov, 'Jan Beulich', Ian Jackson, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 870 bytes --] Hi Lars, > I was wondering how you deal with the gaps. I suppose these gaps could possibly be covered in clang-format-diff.py > Of course this info may be out-of-date I assumed that everything is fine with clang-format and started with python script. Now I have checked both clang-format-3.9 and 5.0, all of those gaps are still remaining. Also there are no corespondent changes expected in Clang-6.0 which currently in-progress. > I wanted to double check, as we had previously looked into clang-format, and it showed some gaps with what it can be used for in Xen coding styles. Which is why we tried to get > agreement - > and got it - to upstream Xen related changes into clang–format We did not plan any changes into the clang-format yet. We have to check with Artem our next steps. Artem is on vacations now. Regards Iurii [-- Attachment #1.2: Type: text/html, Size: 3215 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-26 16:08 ` Iurii Artemenko @ 2017-07-26 16:50 ` Lars Kurth 0 siblings, 0 replies; 9+ messages in thread From: Lars Kurth @ 2017-07-26 16:50 UTC (permalink / raw) To: Iurii Artemenko, Juergen Gross Cc: Artem Mygaiev, Wei Liu, Lars Kurth, Andrew Cooper, cardoe, Andrii Anisov, 'Jan Beulich', Ian Jackson, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 2302 bytes --] > We did not plan any changes into the clang-format yet. We have to check with Artem our next steps. I don't think this is a huge task. I think it is also something that is important enough for the Advisory Board to make funds available – assuming this can't be done otherwise (but I can't promise it would be approved and going this route will take fairly long and would require quite a bit of paperwork) > Artem is on vacations now. Let me know when you talked to Artem Lars From: Iurii Artemenko <Iurii_Artemenko@epam.com<mailto:Iurii_Artemenko@epam.com>> Date: Wednesday, 26 July 2017 at 17:08 To: Lars Kurth <lars.kurth@citrix.com<mailto:lars.kurth@citrix.com>>, Juergen Gross <jgross@suse.com<mailto:jgross@suse.com>> Cc: Wei Liu <wei.liu2@citrix.com<mailto:wei.liu2@citrix.com>>, Andrew Cooper <Andrew.Cooper3@citrix.com<mailto:Andrew.Cooper3@citrix.com>>, Ian Jackson <Ian.Jackson@citrix.com<mailto:Ian.Jackson@citrix.com>>, 'Jan Beulich' <JBeulich@suse.com<mailto:JBeulich@suse.com>>, xen-devel <xen-devel@lists.xenproject.org<mailto:xen-devel@lists.xenproject.org>>, cardoe <cardoe@cardoe.com<mailto:cardoe@cardoe.com>>, Andrii Anisov <Andrii_Anisov@epam.com<mailto:Andrii_Anisov@epam.com>>, Lars Kurth <lars.kurth.xen@gmail.com<mailto:lars.kurth.xen@gmail.com>>, Artem Mygaiev <Artem_Mygaiev@epam.com<mailto:Artem_Mygaiev@epam.com>> Subject: Re: [Xen-devel] Xen checkpatch infrastructure design Hi Lars, > I was wondering how you deal with the gaps. I suppose these gaps could possibly be covered in clang-format-diff.py > Of course this info may be out-of-date I assumed that everything is fine with clang-format and started with python script. Now I have checked both clang-format-3.9 and 5.0, all of those gaps are still remaining. Also there are no corespondent changes expected in Clang-6.0 which currently in-progress. > I wanted to double check, as we had previously looked into clang-format, and it showed some gaps with what it can be used for in Xen coding styles. Which is why we tried to get > agreement - > and got it - to upstream Xen related changes into clang–format We did not plan any changes into the clang-format yet. We have to check with Artem our next steps. Artem is on vacations now. Regards Iurii [-- Attachment #1.2: Type: text/html, Size: 6388 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 16:55 ` Iurii Artemenko 2017-07-24 17:35 ` Lars Kurth @ 2017-07-24 18:08 ` Juergen Gross 2017-07-26 15:36 ` Iurii Artemenko 1 sibling, 1 reply; 9+ messages in thread From: Juergen Gross @ 2017-07-24 18:08 UTC (permalink / raw) To: Iurii Artemenko Cc: Lars Kurth, Wei Liu, Lars Kurth, Andrew Cooper, Ian Jackson, Andrii Anisov, 'Jan Beulich', xen-devel, cardoe On 24/07/17 18:55, Iurii Artemenko wrote: > Hello Juergen, > > > I've started to work on checkpatch-like python script. I make it based > on clang-format-diff.py and it works as pre-commit hook. How does this work for a patch which is perfectly fine, while not touched parts of the patched file are not? The tool should only look at the added lines, not at those left unmodified. >> The easiest way to accomplish that is a file in the repository's root >> directory containing the necessary information. It will be named >> "STYLES" and contains lines in the format: > > I will follow this approach. Thanks. >> Remains the question how to design the style checker itself. It could >> be: >> >> (a) a monolithic script (perl, python, whatever) being capable of >> handling all the different coding styles >> (b) a main script checking the patch header and calling a code style >> specific script for each source file modified by the patch > > It seems like specific script for style checking is not needed. Because > clang-format tool does style checking by itself. > All we need is just to provide appropriate coding style description file > for each. > Clang-format is a bit specific tool, so we can not specify > explicitly file with coding style description. > It just looks for a .clang-format file in one of a parent directories of > a file being checked. > As we got at least three coding-styles we have to substitute needed file > in sources top directory for each check. > It could be done by generating .clang-format file dynamically depending > on style/path from the STYLES file. How would it work for two files located in the same directory but of different coding styles, both touched by the same patch? > Another way could be using appropriate symlink on existing .clang-format > file which is located somewhere in tools/clang-format/coding-style-file > like: > tools/clang-format/xen-style > tools/clang-format/linux-style > tools/clang-format/xl-style Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Xen checkpatch infrastructure design 2017-07-24 18:08 ` Juergen Gross @ 2017-07-26 15:36 ` Iurii Artemenko 0 siblings, 0 replies; 9+ messages in thread From: Iurii Artemenko @ 2017-07-26 15:36 UTC (permalink / raw) To: Juergen Gross Cc: Lars Kurth, Wei Liu, Lars Kurth, Andrew Cooper, Ian Jackson, Andrii Anisov, 'Jan Beulich', xen-devel, cardoe [-- Attachment #1.1: Type: text/plain, Size: 818 bytes --] Hello, Juergen > How does this work for a patch which is perfectly fine, while not > touched parts of the patched file are not? The tool should only > look at the added lines, not at those left unmodified. The tool looks at touched lines only since it works with unified diff and parse it. As for other files it could be done with additional parameter "/path" and format all containing files according to STYLES file. > How would it work for two files located in the same directory but of > different coding styles, both touched by the same patch? I suppose it should be mentioned in STYLES file like: xen ./xen linux ./xen/common/radix-tree.c libxl ./xen/common/other-file.c After diff is parsed we got a list of touched files so, just check for style to apply. Regards Iurii [-- Attachment #1.2: Type: text/html, Size: 7233 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-26 16:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-24 8:50 Xen checkpatch infrastructure design Juergen Gross 2017-07-24 11:44 ` Lars Kurth 2017-07-24 12:46 ` Andrii Anisov 2017-07-24 16:55 ` Iurii Artemenko 2017-07-24 17:35 ` Lars Kurth 2017-07-26 16:08 ` Iurii Artemenko 2017-07-26 16:50 ` Lars Kurth 2017-07-24 18:08 ` Juergen Gross 2017-07-26 15:36 ` Iurii Artemenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.