Add clang-format git integration and clang-format installation script

[sherry-yuan]

User may have different clang-format version locally, and therefore running their local clang-format may cause workflow runs to fail. It will be nice if the clang-format can be pushed directly to forked repo's branch to avoid copying clang format changes.

Or an alternative is to document which clang-format version to run.

[pcolberg]

The clang-format pull-request workflow runs in the context of the origin/target, i.e., this repository, and I would think that the GITHUB_TOKEN does not have write permissions to the fork/source repository either. Even if it were permitted, I would be cautious about pushing commits from a workflow, for security but also for usability concerns. The workflow commits are not automatically reflected in the local repository of the user, which may lead to surprises, e.g., pull conflicts or force-push accidents when further local changes were committed after the initial push.

How about turning the proposal for documentation into a script that helps with the semi-automatic installation of clang-format? We could test this very simple script as part of the clang container build, e.g., a script that takes the installation directory as the only argument and maybe prints copy-pasteable instructions for the common case of extending PATH in ~/.bashrc (which the contributor obviously knows, it would be merely for their convenience).

In addition, we could provide an opt-in pre-commit git hook that automatically runs clang-format (from PATH) before creating a commit, which could be installed by running a setup script that symlinks the hook into ~/.git/hooks. This would be opt-in since clang-format may already be run automatically as part of the contributor's editor.

[pcolberg]

Regarding the pre-commit hook, this actually seems to go against the intended use, which is to check rather than modify the code to be committed. When the pre-commit hook is invoked through git commit, the contributor has most likely already staged their changes (and potentially only partially using git add -p), such that automated modifications could again lead to surprises.

LLVM provides a clang-format git integration, would this be useful?

[sherry-yuan]

How about turning the proposal for documentation into a script that helps with the semi-automatic installation of clang-format?

Yes this is a great idea! And yes, the concern make sense, surprises&potential merge conflict will not make it easier.

LLVM provides a clang-format git integration, would this be useful?

Yes it seems to do exactly what we need, thank you!

[sherry-yuan]

Edit: nevermind, given this might be used in the workflow, we probably want to keep it in script just to be consistent.

I think it make sense to have a separate folder containing tools like this (distinct from script). Should we call it tools (given we have made a decision to not add any other tools repo to this repo's folder)? What do you think @pcolberg ?

[pcolberg]

Edit: nevermind, given this might be used in the workflow, we probably want to keep it in script just to be consistent.

Yes, keeping these helper scripts under scripts is fine for now. To summarize, the proposal is for the script to be used both manually by the contributor to install a matching clang-format on their (Linux x86_64) machine, and automatically by the clang container build. A new section in CONTRIBUTING.md can point to this script and also the git clang-format script.

[pcolberg]

I have thought a bit more and your initial proposal of documenting our use of clang-format, the extact version, and links to the upstream binaries for various OS, might serve a wider number of users. The clang container uses the binaries for Ubuntu 20.04, whereas users might need binaries for older distributions; at that point simply providing the link to the corresponding LLVM release page seems as effective and a script provides little benefit. What do you think?

[sherry-yuan]

The clang container uses the binaries for Ubuntu 20.04, whereas users might need binaries for older distributions; at that point simply providing the link to the corresponding LLVM release page seems as effective and a script provides little benefit. What do you think?

I think that is a good idea, there are so many variants of OS, it is probably nice for user to decide which ones they want to install for themselves. I think the documentation will need to point out that for fedora, centos, suse os, use gnu-ubuntu. (since this is not clear at first glance)