chore: Add build support for clang-format in Travis CI and git hooks.

[Teklad]

Just adding the file in so it can be used by travis and other such things later.

[Teklad]

Okay.... so I've added a small script to help set up a pre-commit hook which uses git clang-format. This will help keep most of our basic formatting stuff intact (i.e. spaces instead of tabs, etc).... regardless of what editor we use.

This has limitations compared to the full clang-format suite... but it works good for ensuring future formatting. Eventually we'll have to run the full clang-format script and screw up git-blame... but this should hold it off for a bit.

[danieleds]

I assume git clang-format is not "compatible by default" with the Travis check, which looks at all files instead?

[Teklad]

I honestly couldn't say on that one. The hooks are the client side half. It can only help keep us from making a lot of basic mistakes. Really probably should check all changed files on the Travis side so we can maintain compliance. It would use clang directly. git clang-format is reliable for mostly line by line changes but not 100% if someone tries some kind of ridiculous newline dance.

…

On Thu, May 10, 2018, 11:12 AM Daniel Di Sarli ***@***.***> wrote: I assume git clang-format is not "compatible by default" with the Travis check, which looks at all files instead? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#664 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACvJZfqbrg8sjyOOYyw6ZZ0eqWKXmH40ks5txGbegaJpZM4T5391> .

[Teklad]

So I've been debating on this for a bit. For the git blame issue could we use a pseudo user so we can isolate the initial formatting out of git easily?

For Travis.... should we have the build fail if clang-format detects issues with code formatting? I don't think Travis has a way to show warning flags, does it?

[danieleds]

So I've been debating on this for a bit. For the git blame issue could we use a pseudo user so we can isolate the initial formatting out of git easily?

Nice idea. But what I fear is the need to reformat all our code each time we decide to enforce some other rule. In addition to messing up git blame, it can create a proliferation of merge conflicts.

That's why I would go towards an incremental approach, that is reformatting the code only when we work on it.

For Travis.... should we have the build fail if clang-format detects issues with code formatting? I don't think Travis has a way to show warning flags, does it?

Hmm, I don't know. GitHub recently introduced a check API, so maybe that could be useful.

A build fail wouldn't be that bad, if we can override it and merge anyway.

[Teklad]

Alright. I set up Travis CI on my account so I can do some testing. I'll use git clang-format so Travis can check against changed content rather than entire files, which should give decent results so long as nobody tries some kind of mad scientist stuff.

I'll look into the check API and see if it might be useful to us. It would be nice to have a more detailed build process outside of just sifting through logs.

The git hook in this PR is really convenient for auto-formatting new/changed code with each commit. In order to use it all you have to do is cd into the "tools" directory and run ./setup-hooks.sh. I need to make some small modifications so it bases the path on script source directory, but I figured it'd be really useful for those regular mistakes we all make (i.e. if (){, if() {, some_pointer *varname).

On a side note, you wouldn't mind if I used clang-format in a limited manner to sort our include files in a separate PR at the very least would you? I've got regex setup which basically tells clang to use project headers, Qt headers, C++ headers, and anything else in that order.

[danieleds]

On a side note, you wouldn't mind if I used clang-format in a limited manner to sort our include files in a separate PR at the very least would you? I've got regex setup which basically tells clang to use project headers, Qt headers, C++ headers, and anything else in that order.

sure, I hate our current random order

[Teklad]

Sometimes I really wonder if Travis was never meant to run docker. Its limited support won't let me use the same dock across different build stages so I can't really separate the format check from the actual build without using "docker login". I'm not really sure how well that will work out on a public repository though.

Anyways, the way I have it set up is it'll check formatting prior to building, so you'll see a diff with recommended changes near the top of the Travis CI build logs. This should make for easy copy/paste recommendations for the most part.

The supplied git hook in this PR will take care of a majority of the formatting automatically for those who bother to enable it.

I went ahead and converted everything over to use a Dockerfile so future changes like this aren't such a massive headache. This should make build system changes a lot easier since it's more contained.

For reference: https://docs.travis-ci.com/user/build-stages/share-docker-image/

[danieleds]

The Dockerfile is very nice.

Do formatting errors make the build fail? If so, like I think it should, then I'd move that to after the build. So that we always know if the build succeeds or not.
Everything's good of course if formatting errors only generate a warning to the GitHub UI.

[danieleds]

Anyway, do you think it's ready to be tested in our repo?

[danieleds]

Actually, can you first push just the Dockerfile changeset? So that we get a clear separation for the formatting stuff

[danieleds]

I can't really separate the format check from the actual build

Did you already try a build matrix?

[danieleds]

I promise I'll stop with the messages. We really need a chat channel.

[Teklad]

Soft-fail.... it'll continue on and finish the entire build process, so position doesn't seem to matter here.

I've not tried a build matrix yet, but I'm assuming there's probably going to be some kind of issues. Travis kinda has docker just bolted on more or less.

I'll go ahead and make a PR for just the docker stuff. This PR is also done, lets go ahead and get the docker merged first though.

Also, IRC is typically pretty handy for live chat type things, lol.

[Teklad]

I've almost got this how I want it... managed to get build stages to work by using the normal Travis container for checking formatting. Docker isn't really necessary in that step since we're not compiling anything.

A couple of remaining tasks and I think this will be complete.

[Teklad]

So this little side project here turned out a lot better than I'd initially hoped.

First and foremost, the .travis.yml file has been completely reworked. As a result of this, the build/format process is managed by script.sh entirely so we no longer have to follow YAML formatting when making changes to the build procedure.

The format checking job is allowed to fail without failing the entire build. This will allow us to see the needed information without having to screw around with Travis every time the formatting on some code is wrong in the event we want to go ahead and accept a PR.

Most importantly, I managed to get all of this done without slowing down the build at all.

As a side note, we are allowed to upload a prebuilt docker image to dockerhub with all of our build libraries pre-installed on a VM. This might be a good idea later on when I find time.

Anyways, I'm pretty happy with these results and believe they should last us well into the future. This PR is finally ready for merging at your leisure.