core: Add coding style enforcement and update selected code areas

[a-szegel]

Lets enforce a style guide across the project to save everyone time (no more giving or getting formatting comments).

Adds the clang format file from linux kernel.
Adds CI to make sure future PR's are formatted (tested on this PR, it works).
Formatted all files in the project.

To format the entire project:

find foo/bar/ -iname *.h -o -iname *.c | xargs clang-format -i

To format just the files you have changed:

git-clang-format

[shefty]

I'm guessing these are kernel macros that can be removed from this file.

[j-xiong]

Agree. We can add our own foreach macros defined in ofi_list.h.

[a-szegel]

done

[shefty]

Some providers will never pass these checks.

[a-szegel]

@shefty

I formatted every provider in this PR with find foo/bar/ -iname *.h -o -iname *.c | xargs clang-format -i, and added a github workflow to enforce new PR's follow the style guidelines. If a commit fails, they can just run git-clang-format to only modify changed files.

Do we like this idea?

[a-szegel]

Err.. apparently I messed it up a bit... but will fix

[shefty]

I don't want to reformat all existing providers or force those providers to a specific coding style. This needs to be more selective.

I want loosely enforced style for any of the core code, plus specific providers that are close to following the standard (tcp, ucx, shm, util, udp, verbs, rxm, rxd, mrail). I would not modify psmX, opx, bqg, or gni.

And there's 0 chance of anyone reviewing a 100k line patch.

[shefty]

Btw, these sort of changes end up making it difficult to cherry-pick patches to stable branches.

[a-szegel]

@shefty Would you be ok with me adding this... and only modifying include and source?

[a-szegel]

and we can have separate PR's for each provider who wants to opt-in... we can choose which directories the CI checker looks in too (so we can keep that)

[a-szegel]

I think the long term benefits of not wasting time on style comments outweigh the temporary pain of a few hard re-bases.

[a-szegel]

something is wrong with how I setup the clang format file, or how I am calling clang-format

[a-szegel]

after spinning up an ubuntu instance and running clang, format with no changes... it is the job that is broken

[a-szegel]

CI was using clang format version 13, I was using clang format version 14... apparently version matters

[lrbison]

Can we include with GPL-2.0 license?

[shefty]

No - we need dual license or one usable with BSD., such as MIT.

[shefty]

Let's leave this as-is and discuss in next week's ofiwg call to get more input.

[sydidelot]

I think the long term benefits of not wasting time on style comments outweigh the temporary pain of a few hard re-bases.

Mon opinion probably doesn't matter much, but the fact that the PR breaks cherry-picking is definitely a big concern. It's not only about "a few rebases" as you say.

• Backporting hotfixes becomes difficult after your PR. When I find a bug in Libfabric, my first reaction is usually to check if a newer version of libfabric has a fix and backport it if possible. Updating to a newer version of Libfabric is not always an option.
• Git blame doesn't give you the original author/commit for a given line. (yes, I find git blame/log really useful and a good source of documentation)

If your concern is about detecting coding style issues during reviews, why not implement a GitHub action that would do a quick check on the lines changed by the PR when it's submitted?

[j-xiong]

I agree with @sydidelot and @shefty. Furthermore, I don't think giving formatting comments at reviewing time is a bad thing. It helps people proactively adopt the recommended coding style instead of relying on automatic tools.

[shefty]

From the ofiwg: There's not support for widespread updates to the code formatting. People are okay with having a check per PR to verify the formatting, as long as we can ignore the results and still commit changes. (So, we don't end up with weird formatting trying to force a coding style on part of a function that has a different style.)

[a-szegel]

Can I get rid of the formatting commit and keep the CI + the file? What do we want to do about the license?

[shefty]

Yes, the consensus was that that would be acceptable. We would need a new version of the file without the GPL only license, which means creating our own, basically from scratch, unless you can find another project or base template that's usable.

[shefty]

We can generate a template format file using a command similar to this:

clang-format -style=llvm -dump-config > ~/tmp/.clang-format

There are a few options for -style, but I don't know which one is the closest to what we want as a starting point: llvm, gnu, mozilla, google, etc.

We could start with a fairly minimal set and expand as needed.

[a-szegel]

I am going to make SM2 the first provider to follow these rules.... Will re-open this when I am ready