Coding style checks in CI

For a few weeks now, we’ve had coding style and thinko checks running in CI for GLib, long enough to see that it’s working OK (check out this pipeline!) and is perhaps time to share with others.

There are two jobs in the checks, both of which run in a new style-check stage of our CI pipeline, which runs before anything else. One job checks the coding style of a merge request, using clang-format. The other job checks for any lines which introduce TODO comments (or similar). These jobs are intended to be fast, but also to not fail the pipeline if they produce warnings. Coding style is subjective, and nobody has yet come up with a mechanical style description which doesn’t have ugly corner cases. So the intention of these jobs is to help remind people of the coding style, to avoid reviewers having to check coding style manually, and hence to only end up thinking about it or discussing it when the CI says the style is wrong.

The first job, style-check-diff, uses a script to work out the diff from the merge request’s branch point, and then passes that to clang-format-diff.py, a script from LLVM which uses clang-format to reformat only the changed lines in a diff. If any reformatting occurs, that means the merge request differs from the GLib coding style (as described by .clang-format) and should be warned about.

There are some limitations to clang-format: it can’t completely describe how the GLib coding style aligns function arguments. That’s unfortunate, because GNOME Builder aligns function arguments by default (and it’s nice behaviour). Hopefully somebody will implement support in clang-format for this sometime, so that it can accurately describe the coding style which is used by a large number of GNOME projects.

These coding style checks are work by Pavlo Solntsev and Emmanuel Fleury. Thanks!

The second job, check-todos, also uses a script to work out the diff from the merge request’s branch point. It passes that to a little Python program which checks for banned words in the commit message and added lines in the diff. The aim is to prevent any comments containing TODO or WIP from accidentally getting merged, as these strings are often used by developers to indicate something they need to come back to before they’re finished — and it’s easy to miss them!

I’ve been using the convention that comments containing FIXME are OK to be merged, as they indicate something that will need updating in future, but can be merged as-is for now (such as a workaround).

Feedback, improvements, and copying welcome. These scripts should run on any CI image which has git, clang and Python.

6 thoughts on “Coding style checks in CI

  1. Philip Chimento

    For GJS, I took the position that not fighting the available tools was more important than having function arguments aligned in GNOME style, and just set up a CI job (and commit hooks; https://github.com/barisione/clang-format-hooks) to ensure that all newly written code is in Google C++ style with a limited number of local modifications to the style rules. (That was a bit easier to justify anyway, because GNOME style fits C++ code less well.)

    Reply
  2. Michael Catanzaro

    Epiphany has a code style checker that follows GNOME parameter alignment rules. We're using uncrustify rather than clang-format, plus a custom script to line up the parameters that could probably be used in tandem with clang-format: https://gitlab.gnome.org/GNOME/epiphany/blob/08fb6aad863da563cd05cffe19bc7946d1db9d64/data/lineup-parameters

    Our CI will actually give you a fatal error if you fail the style checker, but this is not a problem because, in practice, there are no false positives. uncrustify doesn't change anything it's not supposed to.

    Reply
  3. Daniel Stone

    Have you considered using GitLab CI's allowed-to-fail job on the marker? When the check fails, this would have the overall pipeline status set as 'passed with warnings', rather than succeeded and needing to look into the job log.

    Reply
      1. Daniel Stone

        Colour me confused, then; it looks from the `run-style-check-diff.sh` use of `set +e` followed by `echo` (which will always return 0) as the final command, that the script will never fail ... ?

        Reply

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.