Formatting

[wintersteiger]

This fixes a few code formatting violations in recently added code. Many of the header files are also not formatted correctly (not fixed here). Do we have consensus on the style and can we start to enforce it?

It's impossible for me to tell which files are supposed to be OE-formatted which one's aren't. Can someone mark those by adding clang-format exceptions at the top of those files or give me a list of files so I can add them?

[davidchisnall]

I think everything in src should be formatted with the clang-format thing. It's really annoying trying to review diffs where people have mixed formatting changes in with real changes and we have several people that don't read their diffs before committing so end up including formatting changes.

[wintersteiger]

Great, I'll format all of the files then and look into adding a commit hook!

[wintersteiger]

@letmaik had already set the hooks up, thanks! This enables them and src/* is auto-formatted.

[letmaik]

@wintersteiger Thanks for doing this, there's just one more thing missing to also enable it in CI, see the scripts/check-ci script. Looking at the CI logs there also seem to be some unformatted files still, probably tests.

[davidchisnall]

If we're going to enable this in CI (and I recommend that we do), please also add a build-system target that runs the same command that CI runs to check.

[letmaik]

@davidchisnall Currently we use a git pre-commit hook to run the checks. Adding a build system target would allow you to run the checks before trying to commit. It's the same though as just running scripts/check-ci. So just to confirm, do you still want a new target?

[davidchisnall]

Does the pre-commit hook fix the issue? If so, I'm happy. If not, if contributors get an error trying to commit and the instructions are any more complex than 'run this build target to fix the issue' then we're adding annoying levels of friction for new contributors.

It's also annoying that the pre-commit hooks depend on some Python goo. I didn't notice this when they went in (I thought they were just linting for the Python code, not something everyone runs on every commit) and so I got weird failures that I didn't have some Python packages installed the next time I tried to commit after pulling them down, which made me cranky.

[letmaik]

@davidchisnall I understand your concerns and we can definitely change this. The default was to just do what OE does. We can also do what CCF does if that fits us better (no pre-commit hook, but CI checks in a separate job that doesn't block early testing while formatting isn't right yet, plus a notice in CI to run command xyz to fix issues / run the linter).

[davidchisnall]

Snmalloc also does the latter, and it's much easier for new contributors: if CI fails in the formatting step, they're told 'run ninja clang-format in your build directory and commit the changes' and then everything works. It would be even nicer if CI could do that commit for you in CI jobs...

[letmaik]

If everyone agrees, then this is what's missing here:

• Remove pre-commit script
• Change pre-commit install code in build system to instead remove the potentially installed pre-commit script (to avoid devs having to do it manually in the transition period)
• Change CI to run checks in a separate job, like CCF is doing it
• Add target to build system that formats code and runs all checks
• If the checks fail in CI, print notice that advises to run that build system target

If think for now, auto-committing in CI is too much, especially because most linter failures can't be auto-corrected.

[wintersteiger]

That sounds great, thanks @letmaik! And yes, I think I didn't format the tests.