quality-of-life fixes: clang-format, clang-static-analyzer, pre-commit

[ghost]

Description of Changes

This introduces a large set of quality-of-life improvements.

    3a5a984 ci(github): run CodeChecker on PRs
    ecedcc0 ci(github): run pre-commit on PRs
    81c9a71 misc(git): introduce .git-blame-ignore-revs
    b40dc51 ci(pre-commit): add actionlint
    99e3f12 style(tree): apply mdformat fixes
    a517e34 ci(pre-commit): add mdformat
    c022660 style(tree): apply clang format changes
    a25887e ci(pre-commit): introduce clang-format
    190df1d ci(pre-commit): add editorconfig file
    8e8adf9 ci(pre-commit): introduce conventional commit hook
    0d59cb3 ci(pre-commit): introduce shellcheck
    46a1f43 style(tree): apply pre-commit whitespace/eol fixes
    e5a2c86 ci(pre-commit): add eof/whitespace checks
    b120d73 ci(pre-commit): introduce pre-commit

1. Introduces pre-commit hooks which run on every pull request under GitHub
   actions, as well as locally when setup by developers. This can catch a large
   variety of style nits before they reach PR. When attempting to commit a
   violating file, the fix will be automatically made and developers just need
   to restage the file.

   Checker	Description
   trailing-whitespace	Delete trailing whitespace, also covered by clang-format.
   end-of-file-fixer	Ensure that there is a new line at the end of all files.
   check-merge-conflict	Looks for strings like ’<<<’ and prevents their commit.
   detect-private-key	Looks for anything that looks like a private key and prevents its commit.
   mixed-line-ending	Prevents CRLF/LF mixture.
   check-added-large-files	Prevents accidental staging of output files
   check-shebang-scripts-are-executable	If it has a shebang, it should have +x
   check-symlinks	Symlinks should point to a file that is tracked by git.
   check-xml	XML should parse.
   shellcheck	Runs shellcheck over any executable sh/bash script.
   mdformat	Runs mdformat, applies consistent markdown style, and applies fixes to markdown which minimize diffs.
   conventional-pre-commit	Enforces the usage of a standardized commit message. This can be used to generate changelogs and helps identify breaking commits.
   clang-format	Runs clang-format over the code on each commit. See below section for details.
   actionlint	Ensures that all github actions in the repo parse properly.

   The runtime is reasonably acceptable.

        trim trailing whitespace.................................................Passed
        fix end of files.........................................................Passed
        check for merge conflicts................................................Passed
        detect private key.......................................................Passed
        mixed line ending........................................................Passed
        check for added large files..............................................Passed
        check that scripts with shebangs are executable..........................Passed
        check for broken symlinks............................(no files to check)Skipped
        check xml................................................................Passed
        shellcheck...............................................................Passed
        mdformat.................................................................Passed
        clang-format.............................................................Passed
        Lint GitHub Actions workflow files.......................................Passed
        pre-commit run --all-files  9.56s user 4.32s system 322% cpu 4.308 total

If the checks get in your way when making temporary commits, one can always
pass --no-verify to git and run git rebase --interactive origin/master --exec 'pre-commit run --all-files' when preparing for PR.

2. Introduces a clang-format file to the repository, and makes the style more consistent.

   See commit message of a25887e ci(pre-commit): introduce clang-format for
   details on how the specific style was chosen. To avoid making all git history
   useless as a part of this, .git-blame-ignore-revs is introduced, which can be
   configured locally to hide these commits from git blame. GitHub has support
   for reading this file and hiding it from the blame view, as well.

3. Introduces static analysis which runs on every pull request under GitHub
   actions. An example run can be found here. This runs cppcheck, clang-tidy,
   and clang-static-analyzer internally via CodeChecker. Note that CodeChecker
   is referenced on the upstream LLVM docs as a replacement for their older
   python scripts and is fully open source. For now, PRs are not blocked on
   reports from these tools. A follow-up PR will fix some of these issues, after
   which, we can remove the remaining warnings that are not useful and fail the
   analyzer when it introduces regressions.

There are several opinionated changes in this PR that need further discussion
and it is marked as WIP for that reason.

[rajachan]

High-level comment: While the rules files additions are good, I don't think we should have commits that fix everything in one shot. We can adopt the format rules for subsequent changes and incrementally change parts of code as we touch them, but a blanket fix makes diving into history harder than just git blaming and tracking down changes from there.

[ghost]

High-level comment: While the rules files additions are good, I don't think we should have commits that fix everything in one shot. We can adopt the format rules for subsequent changes and incrementally change parts of code as we touch them, but a blanket fix makes diving into history harder than just git blaming and tracking down changes from there.

I tried to alleviate some of this pain with .git-blame-ignore-revs, are you saying that you feel it's still too noisy with that configured? Here's blame on ofi_nccl_net.c, for example.

AFAICT, the only case where those listed commits still appear in git blame is where it added lines, eg: with the include reordering, line wrapping, or brace insertion. I agree that it's still more lines than I would like to see.

[bwbarrett]

I don't like whitespace refactoring commits. They break git blame. I'm happy to have filters to make sure we don't screw up whitespace again, but no silly whitespace fixup commits, please.

[bwbarrett]

nah; we just shouldn't do these commits.

[ghost]

ack, fine with this.

[ghost]

I want to reraise this -- this is a good feature that can help us make changes without clobbering history. I've dropped the application of clang-format on this PR, but I didn't drop all of linters/fixers and some of those are better ignored. Note that github supports this file and blame should work correctly if you go to the top of this page and click the branch, or this link: https://github.com/aws-nslick/aws-ofi-nccl/tree/qol

[bwbarrett]

I'm not actually sure why we need GNU_SOURCE defined, but I definitely don't want to make it the default.

[ghost]

We definitely need it for asprintf, potentially others.

I agree that we should avoid gnu extensions, but so long as they're already in the codebase, I feel it's more reasonable to commit to compiling with a consistent set of feature macros until we can get back to _POSIX_C_SOURCE >= 200809L compatibility.

[ghost]

I feel strongly that this change is prudent because of potential ABI breakage when files are compiled with different feature test macros.

I also went a step further and removed config.h for the same reason -- we have had bugs in the past few months where files did not include it, or didn't include it at the top, and this solves that for good -- no file is evaluated without bringing those macros into scope.

[a-szegel]

bot:aws:retest

[a-szegel]

Please merge master.