machinekit-cnc
machinekit-cnc copied to clipboard
machinekit
Coding style and static analysis
We should enforce a specific coding style for every language in the project. This can e.g. be done by using astyle. My code editor (Kate) always shows mixed indentations (tabs and spaces) everywhere. This makes the code unreadable on some platforms and is possible error source.
Furthermore every piece of code that is commit should be checked with suitable static code analysis tool. For C and C++ cppcheck and clang might be suitable. For Python pep8 is the way to go. This ensures that common error will not make its way into the project.
For Python it is also a good idea to use a tool that checks Python3 compatibility as well. The IDE http://ninja-ide.org/ does that. There might be a command line tool behind this as well.
I think encourage is more practical the require. automatic warnings of coding style seems a good idea. I too find tabs an spaces more then annoying - do you suggest we clean up that problem first? I would be disappointed if a project was rejected for inclusion just be cause someone style was not the same as ours. eg for instance Classicladder. I think consistent style inside each project (eg Classicladder, AXIS etc) would be better then requiring all of MK the same. And don't look at my code please :)
@c-morley, agreed, encouragement is more realistic than requirement, and @strahlex, some of those tools could be used to help automate stylistic issues to make review easier for Maintainers.
@c-morley, I don't quite understand about rejecting a 'project'; what does 'project' mean here? IMO, external projects like Classicladder need to be unbundled from MK if possible (I realize that's not immediately possible in this case) to comply with DFSG and other distro packaging guidelines, and GUIs need to be spun out for other reasons (there's another ticket addressing this).
In any case, it's fine with me to allow different styles within different projects (and IMO that's another indicator a project should be spun out!).
@zulton: by project I meant any self contained program. eg stepconf, classicladder, AXIS gui, gladeVCP etc. Classicladder is a fairly extensively modified version and many versions behind of the parent program.
@c-morley: Sounds like those are candidates for spinning out (git subtree, build system, packaging) of the core Machinekit code? @luminize probably knows more than most anyone else in the MK community about git subtrees. I wonder if that wouldn't make the lives of folk like you easier too, if there were one GitHub repo that could be built against either of MK or LCNC projects.
Is there any tool that cleans up tabs and spaces? I think it makes sense to check for such problems before merging a patch. Most editors do not display tabs and spaces differently and thats really annoying.
I would not force a specific coding style on anyone, but providing a standard astyle file for C,C++ and Python would make sense. Especially for those who do not program frequently and need some guidelines anyway.
Static analysis is whole different thing and should be enforced if possible. Well, it can be annoying and too strict (MISRA C), but it could prevent many ugly bugs introduced in projects.
yes: Emacs indent-region.
Please use the settings for C++ (put in your .emacs):
(setq c-default-style '((other . "gnu"))) (setq c-basic-offset 4)
If you re-indent a file, please make it a separate commit - do not mix editing with a whitespace-only change, which will make it harder to resolve merge conflicts.
I recently created a PR to the Qt project and came across a high sophisticated sanity checking system. Something like this could be useful for us too: https://wiki.qt.io/Early_Warning_System
E.g. the use to sanitize commit messages and do some basic checks like indentation: https://github.com/qtproject/qtrepotools/tree/master/git-hooks
Having a check for code-style in CI could have some benefits. Could we use the qtrepotools somehow to reject PRs that do not adhere to the style guide? Otherwise: do you know which tools we'd need for this?
Also, to adhere to the style guide, the current code needs to be cleaned up. What is the best approach for this? One large commit at some point?
Also, to adhere to the style guide, the current code needs to be cleaned up. What is the best approach for this? One large commit at some point?
Can we generate a list where stuff is bad? Then we make small packets, because this is cleaning up, not developing, people can chime in, and work in parallel. Like a work-packet which can be done in 30 mins so somebody can finish a part, make a PR and get to sleep with a satisfied feeling.
parallellism and small bites are the key IMO.
@luminize I think a lot can be done automatically, like tabs/spaces, but I'm not sure whether that's a good idea for the current code-base.
@bobvanderlinden I'd love to integrate a coding style check into CI.
The main concern I'm aware of for mass cleanups is making it hard to perform merges. If we did do something like this, it should definitely be limited to the Machinekit core code (RTAPI + HAL), which just a small number of people contribute to.
Mass cleanups should be kept out of the CNC application code, since we're preparing to spin it out and leave maintenance for other projects. Any changes to that code to work with MK core should be easy to pick off into other LCNC forks.
There is GitHub extension for doing automatic code analysis for pull requests: https://github.com/integrations/code-climate
@strahlex Code Climate doesn't seem to support C/C++ (yet?). It does do Python though, so maybe Travis and Code Climate could be combined? It could also be very usable for related Python and JS projects too.
Just have seen flake8 can be used to check Python PEP8 rules for CI.
Frama-C acts both as static code analysis tool and as theorem prover if required. Could be useful for checking C HAL components e.g.
I just added Machinekit to Coverity Scan: https://scan.coverity.com/projects/machinekit-machinekit
Let's see if it proves useful.
On 13 Mar 2017, at 09:48, Alexander Rössler [email protected] wrote:
I just added Machinekit to Coverity Scan: https://scan.coverity.com/projects/machinekit-machinekit
Let's see if it proves usefull
Interesting. I wonder if a lot of checking for sizes in buffer copy for example is left out on purpose of just bad programming. http://cwe.mitre.org/top25/#CWE-120
@luminize In a lot of cases we are not dealing with user input, so not checking the data size might be safe. However, I cannot think of any good reason why we shouldn't write the copy functions in a safe way as it is basically for free to do so.
Using editorconfig is pretty easy to achieve: https://github.com/qtquickvcp/QtQuickVcp/blob/master/.editorconfig
On 07 Apr 2017, at 18:37, Alexander Rössler wrote: Using editorconfig is pretty easy to achieve: https://github.com/qtquickvcp/QtQuickVcp/blob/master/.editorconfig
Yeah, i'm using it in one of my own projects. Let's put one in the main MK repo for the languages in use, and people who use editorconfig will be served.
Let's make it optional to use it, and not mandatory.
Then we can close the coding style part.
Yo, what's the status on this? I can't find any editor config files anywere, and I've noticed that indentation is really inconsistent. For now I'll follow the CodingStyle file in src
@MicroTransactionsMatterToo , status is such that nobody was offended enough by this issue to come up with concrete solution.
Personally, I was looking into creating post pull hook which would amend every commit in the request with code style enforcement and then run it by other linter functions and static analysis. Problem is - it creates new hashes, so that one creates a bit of burden. But not much, given that the need to rebase the code onto Machinekit-* master will still be there. That way, the code would be updated to new styles piece by piece and not in Big Bang.
CodingStyle is nice up to the point the editor ignores it. (So I am proponent of forceful solution.)
@cerna Right. I'll do that. EditConfig only provides for a few formatting options, so I might just write a few different config files for different editors in my free time
@cerna Right, I've taken a look and clang-format seems the best option, since it's pretty likely to be installed on most linux distros. I can write either a pre-commit hook or post pull hook. There's an option now to version control git hooks so a pre-commit hook would apply the given formatting rules to any changed/new files at commit without people having to re-add the hook, but I dunno whether that'd be ideal. What are your thoughts? Another option is GNU indent, but that's it's own package, where as clang-format comes with clang by default
@MicroTransactionsMatterToo , yes, clang-format is - I think - pretty standard for this and fully adequate.
I am not exactly expert on git hooks (understanding of the century). So let me get it straight and sum it up. So far, the workflow is one forks the machinekit/Machinekit-* repository on GitHub to user/Machinekit-*, then clones the user/Machinekit-* locally into /Machinekit-* on which the development is actually done. You would create Client-Side pre-commit git hook on the /Machinekit-* repository to enforce the code formatting, right? Something like this. So every developer would have to have a Clang on his machine.
However, the last time I checked, the git hooks were not version controlled and did not propagate from remote->local repository, so
(...)There's an option now to version control git hooks so a pre-commit hook would apply the given formatting rules to any changed/new files at commit without people having to re-add the hook,(...)
from this I take it is no longer such? Nowadays, git hooks in remote repository do propagate when cloning or fetching/pulling and are version controlled? Because that would be great!
The post-pull git hook would be Server-side one on machinekit/Machinekit-* repository, right? And do you mean that it would only check the formatting of proposed pull request or it should do some changes to commits?
Thanks.
@cerna Git has added a mechanism for version controlling commit hooks. You can set a git hook repository explicitly that is outside of ~/.git/hooks. The post-pull hook was something you suggested, I don't think it'd be ideal