Integrate clang-format into checks, docs and tooling

[hoffie]

With the initial clang-format definition and initial run with the config having been merged (#1127), the related issue (#901) was auto-closed, although its scope was greater than what the (huge) PR did. Therefore, I'm opening this follow-up issue for tracking the remaining parts for the actual integration.

Has this feature been discussed and generally agreed?

Yes, here: #901 (comment)
Some details still to be decided upon.

Describe the solution you'd like

• Make a final decision about the type of Github action (shamelessly copied from @passing's Automatic code linting for pull requests #901 (comment))
  1. Enforce style (auto-update PRs -- not sure if/how this is possible and feels like)
  2. Check style and block merge on invalid style (but I assume that admins can override in any case, needs to be checked).
  3. Check style, but don't block merge on invalid style.
• Implement the Github action
• Check if and what tools (Makefile?) or docs (CONTRIBUTING?) should be updated/extended for local clang checks

[hoffie]

I'm adding my personal opinion as a comment:

1. Enforce style:
   • Advantage: Contributors do not need clang(-format), we don't have to remind them about applying it.
   • Advantage: We could use the latest clang-format (12) features
   • Disadvantage: I've never seen such an integration as part of a PR check. I don't know if and how it can be done and what consequences it would have (would contributors have to force-push when updating their branch?). Maybe @passing knows more about that?
   • If Enforcing only works as a Github Action (i.e. after merge), it would have the (huge) downside that code review will still happen on unfixed code, rendering some of the basic advantages useless
2. Check style and block merge on invalid style
   • Depending on how (1) works out in practice, this would be my preferred option (always assuming that admins can override in case of emergencies)
3. Check style, but don't block merge on invalid style.
   • Disadvantage: Contributors (or us) might get lax about it. I do not like this option and I've just included it for completeness.

@jamulussoftware/maindevelopers @passing What do you think?

[pljones]

For me,

Check style and block merge on invalid style

is really the best option. Any check written as a standard CI action can be overridden, as far as I understand.

[passing]

I'd prefer the "enforce style" option but only if we can clear out the disadvantages to a certain degree:

If Enforcing only works as a Github Action (i.e. after merge), it would have the (huge) downside that code review will still happen on unfixed code, rendering some of the basic advantages useless

that should not be a problem as you can configure the action to be triggered has soon as a PR is created:
https://github.com/passing/jamulus/blob/master/.github/workflows/clang-format.yml#L6-L7

I tested that here: https://github.com/passing/jamulus/pull/3/commits

• added some badly aligned code on the branch
• when the PR is created, the action kicks in and adds another commit when changes are needed

Disadvantage: I've never seen such an integration as part of a PR check. I don't know if and how it can be done and what consequences it would have (would contributors have to force-push when updating their branch?). Maybe @passing knows more about that?

In my test that did happen, so after the first commit has been linted, I had to either pull (and manually resolve locally) the linter changes or force-push consecutive changes

But I am not sure how that works in the same way when the PR is coming from another (forked) repository. It would be good if you can test that by making a PR to https://github.com/passing/jamulus as the action may not make the change in the other repo.

[hoffie]

I'm still a bit unsure how we'd do certain things. History-wise, I'd really want to avoid "Fix clang-format style" commits for every PR/commit as it'd clutter the history. It would also be annoying for git bisects. So this would probably mean that all PRs must be squash-merged (which may be beneficial for 90% of them anyway). That still leaves those PRs which should not be squash-merged.

It would be good if you can test that by making a PR to https://github.com/passing/jamulus as the action may not make the change in the other repo.

Done. You would need to approve the check on Github though as I'm a first-time contributor in your fork.

[passing]

I agree on the concerns regarding the commit history.
So basically providing a way to contributors to lint locally may be a good idea in any way, independent of having automatic linting or not.

Another approach could be to just accept style violations in contributions and do a manual one-time lint/commit on every release. That would reduce lint commits and also reduce contributors efforts. I don't really favor that, but may be a compromise worth thinking of.

Done. You would need to approve the check on Github though as I'm a first-time contributor in your fork.

somehow the action had some issue, need to have a look at it.

 [test-clang-format-dont-merge 694ba67e] Committing clang-format changes
   1 file changed, 846 insertions(+), 846 deletions(-)
   rewrite src/main.cpp (87%)
  Tagging commit...
  Pushing commits to repo...
  remote: Permission to passing/jamulus.git denied to github-actions[bot].
  fatal: unable to access 'https://github.com/passing/jamulus/': The requested URL returned error: 403
  Pushing tags to repo...
  remote: Permission to passing/jamulus.git denied to github-actions[bot].
  fatal: unable to access 'https://github.com/passing/jamulus/': The requested URL returned error: 403
Task completed.

[hoffie]

So basically providing a way to contributors to lint locally may be a good idea in any way, independent of having automatic linting or not.

Yes. The basis for that should be there now (.clang-format) and I hope some editors might pick this up already.

Another approach could be to just accept style violations in contributions and do a manual one-time lint/commit on every release. That would reduce lint commits and also reduce contributors efforts. I don't really favor that, but may be a compromise worth thinking of.

We should also consider the disadvantage in code reviews. The main reason for adopting a consistent coding style is keeping code readable. Code reviews require lots of code reading, so it would be really useful to have properly formatted code there, one way or the other.

Done. You would need to approve the check on Github though as I'm a first-time contributor in your fork.

somehow the action had some issue, need to have a look at it.

Ok, let me know if I should do something on my side.

[passing]

setting up a workflow that lints the code in PRs from forked repositories, doesn't seem to be easy and then still would require a contributor to enable the action:
https://github.com/stefanzweifel/git-auto-commit-action#using-the-action-in-forks-from-public-repositories
and I'm not sure if it can work at all with the workflow from https://github.com/DoozyX/clang-format-lint-action#inplace

So I'l try to setup an action that just reports wrong style

[passing]

@hoffie code checking is working on my own PR now: https://github.com/passing/jamulus/pull/3
You probably need to merge the changes I made to the workflow (https://github.com/passing/jamulus/blob/master/.github/workflows/clang-format.yml) to your fork to have the same on your PR

[ann0see]

@hoffie I think we can close this now?