Coding style and static analysis

[ArcEye]

Issue by machinekoder Tue Dec 2 10:10:02 2014 Originally opened as https://github.com/machinekit/machinekit/issues/391

---

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.

[ArcEye]

Comment by machinekoder Tue Dec 2 10:12:30 2014

---

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.

[ArcEye]

Comment by zultron Tue Dec 2 15:37:54 2014

---

I'm all in favor of requiring consistent coding style.

[ArcEye]

Comment by c-morley Wed Dec 10 10:00:39 2014

---

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 :)

[ArcEye]

Comment by zultron Thu Dec 11 19:22:07 2014

---

@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!).

[ArcEye]

Comment by c-morley Fri Dec 12 04:27:09 2014

---

@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.

[ArcEye]

Comment by zultron Sat Dec 13 03:54:00 2014

---

@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.

[ArcEye]

Comment by machinekoder Sat Jan 31 11:11:01 2015

---

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.

[ArcEye]

Comment by mhaberler Sun Feb 1 07:34:11 2015

---

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.

[ArcEye]

Comment by machinekoder Thu Jul 9 06:55:50 2015

---

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

[ArcEye]

Comment by machinekoder Thu Jul 9 06:58:17 2015

---

E.g. the use to sanitize commit messages and do some basic checks like indentation: https://github.com/qtproject/qtrepotools/tree/master/git-hooks

[ArcEye]

Comment by bobvanderlinden Sat Nov 28 16:57:28 2015

---

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?

[ArcEye]

Comment by luminize Sat Nov 28 17:04:28 2015

---

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.

[ArcEye]

Comment by bobvanderlinden Sat Nov 28 18:05:43 2015

---

@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.

[ArcEye]

Comment by zultron Sun Nov 29 00:19:49 2015

---

@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.

[ArcEye]

Comment by machinekoder Tue Dec 29 20:33:51 2015

---

There is GitHub extension for doing automatic code analysis for pull requests: https://github.com/integrations/code-climate

[ArcEye]

Comment by bobvanderlinden Tue Dec 29 20:47:29 2015

---

@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.

[ArcEye]

Comment by machinekoder Mon May 2 14:26:59 2016

---

Might be useful too: http://editorconfig.org/

[ArcEye]

Comment by machinekoder Thu Jun 30 06:19:33 2016

---

Just have seen flake8 can be used to check Python PEP8 rules for CI.

[ArcEye]

Comment by machinekoder Thu Jun 30 06:20:51 2016

---

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.

[ArcEye]

Comment by machinekoder Mon Mar 13 08:48:58 2017

---

I just added Machinekit to Coverity Scan: https://scan.coverity.com/projects/machinekit-machinekit

Let's see if it proves useful.

[ArcEye]

Comment by luminize Wed Mar 15 13:47:46 2017

---

On 13 Mar 2017, at 09:48, Alexander Rössler notifications@github.com 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

[ArcEye]

Comment by machinekoder Wed Mar 15 14:30:32 2017

---

@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.

[ArcEye]

Comment by machinekoder Fri Apr 7 16:36:59 2017

---

Using editorconfig is pretty easy to achieve: https://github.com/qtquickvcp/QtQuickVcp/blob/master/.editorconfig

[ArcEye]

Comment by luminize Fri Apr 7 16:50:51 2017

---

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.

[MicroTransactionsMatterToo]

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

[cerna]

@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.)

[MicroTransactionsMatterToo]

@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

[MicroTransactionsMatterToo]

@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

[cerna]

@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.

[MicroTransactionsMatterToo]

@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

[MicroTransactionsMatterToo]

But yeah, basically as you described

[cerna]

The post-pull hook was something you suggested, I don't think it'd be ideal

Right, I am just making sure we are on the same page.

Git has added a mechanism for version controlling commit hooks. You can set a git hook repository explicitly that is outside of ~/.git/hooks.

OK, truth be told, everything is better than how it is now. And I (at least personally) am proponent of not-perfect solutions which moves us few steps towards perfect.

[cerna]

Looked into the git hooks a little bit more and it looks like the automatic hook propagation from the remote to local repository on git clone is not possible because it is considered a possible security issue (cannot say I absolutely agree, but it is how it is).

So potential developer would have to something akin to:

git clone https://github.com/userID/machinekit-hal.git mkhal
cd mkhal
git config --local core.hooksPaths config/githooks

Which is not that bad, but you have to know about it and it is one step more. (My general experience is that users don't read and as such many will not do this.)

Reading githooks.com I discovered that this is probably common problem and there exist many libraries, frameworks and projects for git hook management, some even with CI (Travis) support and CMake support. Given that some looks like very easy to set up and include pre-made formaters (its popular option), maybe its viable option? (Would it help in any way with build process, @zultron ?)

And OK, having one script doing it all server-side on main Machinekit repository was idiotic idea. Industry standard is to have main client-side pre-commit script which will do the heavy-lifting and then second one server-side, which will test every pull request and reject those which fail. That how we should do it.

[MicroTransactionsMatterToo]

@cerna Looks like you're right, which kinda makes the core.hooksPath useless outside of being able to keep the hook scripts in the version control. I think it'd be wise to keep extra tools out of the repo. Once @kinsamanka is done implementing CMake into both repositories we can look into fully integrating it into the build process, which we could either use to set the core.hooksPath as part of the build process. As for Travis support, I don't see why Travis would need to run the pre-commit hooks, since Travis doesn't commit anything. As for checking for compliance I'll have to look into it, since clang-format just overwrites the files with the new formatting.

[cerna]

(...)I think it'd be wise to keep extra tools out of the repo(...)

Given the fact that we want to have machinekit-hal and machinekit-cnc (and in future maybe some other repository), you are probably right. But we want to have some files/settings coupled with the repository: the .clang-format file, the .editorconfig file, some other configuration file for prettyfiers, repository specific file some analytics tool, whatever. In other words, what hierarchy is good and what is too much? (If we were to have special repository for git hooks.)

(...)As for Travis support, I don't see why Travis would need to run the pre-commit hooks, since Travis doesn't commit anything(...)

Well, you use the Travis for the Continuous Integration and the fact, that it is capable of running tasks on your behalf. You have scripts and settings. If you use these scripts as a pre-commits hook handlers on the local repository side to perform some work, you can use the exact same scripts to perform tests on other machines that the work was already done. Given the common inputs, it should reach the same outputs. So we could use the formatting as a test on the CI: If the formatting script does nothing, then the work was already done in pre-commit stage and the test pass, otherwise the formatting is wrong and the pull request should fail and @luminize will (by the C4 rules) know not to merge.

[cerna]

And to move forward, lets talk about the formatting.

This is the one I like: .clang-format. Comments? Opposition?

[MicroTransactionsMatterToo]

@cerna I've implemented the hooks and the clang format stuff. It only implements the formatting stuff, so naming and code style stuff isn't enforced (Couldn't find any decent tool to enforce this stuff that wouldn't require writing our own checks). Should I open it as a pull request or something?

[cerna]

@MicroTransactionsMatterToo

I've implemented the hooks and the clang format stuff.

Great.

It only implements the formatting stuff, so naming and code style stuff isn't enforced

That is OK, I think. Small steps. I would rather have code formatting in place for all used languages first. (Python and shell scripting.) Based on how it works, then we can start war on how to name symbols.

Should I open it as a pull request or something?

Yes. Pull request.