Add static C++ analysis using flint

[PerryBhandal]

Facebook recently released their internal C++ linting tool called flint:

https://github.com/facebook/flint

BuildBot should add another step (after test running, but before build delivery) which runs flint. Initially builds can complete successfully even if they result in lint warnings. Eventually, assuming there isn't an excess of false positives, we should cause any builds that generate lint warnings to end in a warn state.

This should be implemented as a flag in build.sh so developers can easily run flint as well.

[PerryBhandal]

As an additional thought, the installation of flint (and its dependencies) should not be included in setup.sh. There's no need for non-dev users to install it. Instead, the first time someone runs build.sh with the run lint flag, it should notify them that flint is not yet installed and provide instructions for its installation.

[brtietz]

We could also consider using something like uncrustify to automatically enforce some of these style requirements: http://uncrustify.sourceforge.net/

On Sat, Sep 6, 2014 at 12:47 AM, Perry Bhandal notifications@github.com wrote:

As an additional thought, the installation of flint (and its dependencies) should not be included in setup.sh. There's no reason for non-dev users to install flint.

Instead, the first time someone runs build.sh with the run lint flag, it should notify them that flint is not yet installed, and provide instructions for its installation.

— Reply to this email directly or view it on GitHub https://github.com/NASA-Tensegrity-Robotics-Toolkit/NTRTsim/issues/70#issuecomment-54705573 .

[PerryBhandal]

That looks excellent -- it would be great to have that as well.

Do you have any thoughts on the best way to implement it?

If we do it all on BuildBot's end (i.e. uncrustify right at the beginning of a build before we run a compile) we'll know that it's always being done. The disadvantage is that we'll have to handle manual merges far more often. It would also pollute the GitHub history as each merge would likely be accompanied by a subsequent merge containing the 'uncrustified' code.

An alternate option is we could run uncrustify once manually on all code in every branch, and from then on we would expect developers to run it manually only on files they've changed. The main problem in that case is we can't be certain it's being run.

A final option is to force build.sh to run uncrustify on all code that has changed since the last build. We could achieve this by having build.sh write out a unix timestamp (in the env directory so it's easily ignored) on each build. During each subsequent build it would determine which files were modified after the previous build's timestamp, and it would run uncrustify on those files only. The main disadvantage with this option is each build will necessitate a reload of all source files in a dev's IDE/text editor.

[brtietz]

I'd prefer the build.sh option. The advantages of doing it pre-commit are obvious, and I think we'd have to re-load the changed files in our editors even if we ran it from the command line, so that doesn't sound like a significant problem to me.

On Sat, Sep 6, 2014 at 8:36 AM, Perry Bhandal notifications@github.com wrote:

That looks excellent -- it would be great to have that as well.

Do you have any thoughts on the best way to implement it?

If we do it all on BuildBot's end (e.g. right at the beginning of a build before we run a compile) we'll know that it's always being done. The disadvantage is that we'll have to handle manual merges far more regularly. It would also pollute the GitHub history as each merge would likely be accompanied by a subsequent merge containing the 'uncrustified' code.

An alternate option is we could run uncrustify once manually on all code in every branch, and from then on we would expect developers to run it manually only on files they've changed. The main problem in that case is we can't be certain it's being run.

A final option is to force build.sh to run uncrustify on all code that has changed since the last build. We could achieve this by having build.sh write out a unix timestamp (in the env directory so it's easily ignored) on each build. During each subsequent build it would determine which files were modified after the previous build's timestamp, and it would run uncrustify on those files only. The main disadvantage with this option is each build will necessitate a reload of all source files in a dev's IDE/text editor.

— Reply to this email directly or view it on GitHub https://github.com/NASA-Tensegrity-Robotics-Toolkit/NTRTsim/issues/70#issuecomment-54715639 .

[PerryBhandal]

Sounds good, I'll implement it in build.sh.

I can also add something to build.sh so that if uncrustify modifies any files, it will print a message informing them that they'll need to reload them.

[PerryBhandal]

Would it be worthwhile to add an additional custom linter which enforces NTRT specific rules?

We could have it enforce the requirement of a license and doxygen header, for example.

[PerryBhandal]

A slight modification to the build.sh approach: we shouldn't use timestamps to determine whether a file has changed since the last build. The problem then is that if one build generates a lint warning, a subsequent build will not (even if the warning hasn't been addressed). Instead, we should use the output from git status to determine which files have been modified relative to head, and run linting/uncrustifying on them.

This isn't a perfect solution, as it would effectively require build.sh be run before each commit. If any files are modified between the final build.sh and a commit, new warnings won't be seen until BuildBot runs lint project-wide (and, if warnings exist, cause a build to finish in a warn state). On the whole this doesn't seem too problematic, we should be doing a final build call before commits anyway.

[PerryBhandal]

I'm going to refrain from adding uncrustifier. I ran it on a bunch of files in NTRT last night, and the changes it made weren't too significant. It's also fairly invasive no matter how we implement it. In Vim, for example, my cursor would jump to the top of the file after each reload. I'm not sure that the gain from it is worth the trouble.

Flint still seems worthwhile, as I'm assuming it deals primarily with correctness. I'll hopefully have some time to integrate it and test it out in the next few days. Once I do, I'll post a subset of the warnings I get when I run it on NTRT. If it provides useful information, we'll merge it into master. If it doesn't, we'll abandon it.

[vsunspiral]

I like the idea of having tools to enforce a common style - but I don't think they need to be run on each build or commit. These are "clean up" tools that we could just setup as build options and developers can use as needed. Just let it run over the whole code base too, make it easy..... After all, even if folks are checking in non-conforming code, any person can just run this once and commit in the refactored code......

---

Vytas SunSpiral
PI - Dynamic Tensegrity Robotics Lab cell- 510-847-4600 Office: 650-604-4363 N269 Rm. 100

Stinger Ghaffarian Technologies Intelligent Robotics Group NASA Ames Research Center

I will not tiptoe cautiously through life only to arrive safely at death.

On Sep 7, 2014, at 8:51 AM, Perry Bhandal notifications@github.com wrote:

I'm going to refrain from adding uncrustifier. I ran it on a bunch of files in NTRT last night, and the changes it made weren't too significant. It's also fairly invasive no matter how we implement it. In Vim, for example, my cursor would jump to the top of the file after each reload. I'm not sure that the gain from it is worth the trouble.

Flint still seems worthwhile, as I'm assuming it deals primarily with correctness. I'll hopefully have some time to integrate it and test it out in the next few days. Once I do, I'll post a subset of the warnings I get when I run it on NTRT. If it provides useful information, we'll merge it into master. If it doesn't, we'll abandon it.

— Reply to this email directly or view it on GitHub.

[PerryBhandal]

While running it across the entire code base would be easy, my concern is the conflict nightmare it would create come merge time. Let's say you make a bunch of changes in a new branch, and I run uncrustify on master in the intervening period: when you try to merge your changes in you will likely have to address conflicts in nearly every file that you've modified. The conflcits woud be trivial to address, but I imagine it could be a bit frustrating to have to manually handle conflcts every time we try to merge into master.

[vsunspiral]

as long as uncrustify is run somewhat regularly, all stable parts of the code base will be untouched. It would only be new contributions that would be cleaned up, and hopefully most developers will run uncrustify themselves before merging in a new branch, so really running it over master would only cause changes in new code which had recently been contributed without the developer themselves having running uncrustify before committing. In which case, they are the ones most likely to suffer from merge conflicts, and it would teach them a lesson.

vytas

On Sep 7, 2014, at 2:48 PM, Perry Bhandal notifications@github.com wrote:

While running it across the entire code base would be easy, my concern is the conflict nightmare it would create come merge time. Let's say you make a bunch of changes in a new branch, and I run uncrustify on master in the intervening period: when you try to merge your changes in you will likely have to address conflicts in nearly every file that you've modified. The conflcits will be trivial to address, but I imagine it could be a bit frustrating to have to manually handle conflcts every time we try to merge into master.

— Reply to this email directly or view it on GitHub.

---

Vytas SunSpiral
Dynamic Tensegrity Robotics Lab cell- 510-847-4600 Office: 650-604-4363 N269 Rm. 100

Stinger Ghaffarian Technologies Intelligent Robotics Group NASA Ames Research Center

I will not tiptoe cautiously through life only to arrive safely at death.

[PerryBhandal]

That is actually a valid point. Fair enough!

How would you like me to implement it? I agree with you that running it on each build is unnecessary, and it brings up some of those annoyance issues I mentioned before. Ideally I'd like to divorce it from build.sh so that it's always seen as a step separate from building. We could have a script called prepForCommit which does the following:

• On all files that have diverged from master we: 1) Verify that core NTRT rules are met (each source file has a doxygen header, and a license header. We can add more to this later.). 2) Run uncrustify on all files.

If we add Flint, I think that should likely be a part of build.sh (as a separate flag, presumably). The reason being that I assume flint's warnings will be focused on correctness, rather than beautification. We likely don't want to risk any code ever reaching the repo with warnings that aren't either addressed or explicitly suppressed.

Perry

On Mon, Sep 8, 2014 at 12:52 PM, Vytas SunSpiral notifications@github.com wrote:

as long as uncrustify is run somewhat regularly, all stable parts of the code base will be untouched. It would only be new contributions that would be cleaned up, and hopefully most developers will run uncrustify themselves before merging in a new branch, so really running it over master would only cause changes in new code which had recently been contributed without the developer themselves having running uncrustify before committing. In which case, they are the ones most likely to suffer from merge conflicts, and it would teach them a lesson.

vytas

On Sep 7, 2014, at 2:48 PM, Perry Bhandal notifications@github.com wrote:

While running it across the entire code base would be easy, my concern is the conflict nightmare it would create come merge time. Let's say you make a bunch of changes in a new branch, and I run uncrustify on master in the intervening period: when you try to merge your changes in you will likely have to address conflicts in nearly every file that you've modified. The conflcits will be trivial to address, but I imagine it could be a bit frustrating to have to manually handle conflcts every time we try to merge into master.

— Reply to this email directly or view it on GitHub.

---

Vytas SunSpiral Dynamic Tensegrity Robotics Lab cell- 510-847-4600 Office: 650-604-4363 N269 Rm. 100

Stinger Ghaffarian Technologies Intelligent Robotics Group NASA Ames Research Center

I will not tiptoe cautiously through life only to arrive safely at death.

— Reply to this email directly or view it on GitHub https://github.com/NASA-Tensegrity-Robotics-Toolkit/NTRTsim/issues/70#issuecomment-54850781 .