Revisit "format-check" workflow - is it needed?

[jphickey]

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
The cFS CCB should revisit the cost/benefit of enforcing the "format-check" workflow on every push/pull. The original idea was that it would keep the white space formatting consistent, but in practice there are a lot of nuisances that result from this:

• Perpetually changing the alignment of variable names (in declarations) and the equal sign (in groups of assignments) causes extra merge conflicts. This is because a single added variable or assignment might change a group of lines rather than just the single line that added it. If another change is made in parallel nearby that causes lines to be grouped that were not grouped before, this is now a merge conflict, whereas without the formatting change it would be a trivial auto merge.
• Changes to the tool (outside our control) cause changes to the expected formatting of lines. For example clang-format-10, which is what the workflow was originally based on, formats slightly differently than clang-format-12 and clang-format-14 do, and there is no way (via config file) to get an identical output from the newer versions. Clang-format-10 is no longer installable in the most recent "LTS" distros - This means that as developer transition to newer dev environments, we will not be able to reproduce the expected formatting easily.
• The ultimate objective is to make source code most readable for humans, but the tool does not know context, and sometimes "strict adherence" to a set of rules yields a less readable result than what a human would write. A prime example is when a table initializer is grouped into logical rows and columns based on its context.

For examples of this, see the "CFE_TBL_CmdHandlerTbl" from an old version of CFE, before clang-format enforcement:
https://github.com/nasa/cFE/blob/8811c92094512975e14fd545df355705141560b2/fsw/cfe-core/src/tbl/cfe_tbl_task.c#L66-L85

Compared to the same table in the current version after clang-format messes with the column alignments, start and end braces, etc:
https://github.com/nasa/cFE/blob/98f78e8604c19415fd1e199eae94196a781539b8/modules/tbl/fsw/src/cfe_tbl_task.c#L68-L85

Other examples include the ES object table and any event registration table, where what was originally a logically aligned table became all disorganized.

• Lastly, we have a few instances of what was a perfectly "compliant" source (see https://github.com/nasa/FM/actions/runs/4166519343/jobs/7210962821) suddenly starts being non-compliant (see https://github.com/nasa/FM/actions/runs/4450946126/jobs/7817016323)

Note that the file that failed here (fm_child_tests.c) did NOT change between these two runs. Something else changed, but its the same version of clang-format and the same .clang-format file from what I can tell.

Expected behavior
Tools should be a help, not a hinderance. Keeping up with clang-format nuisance errors might not be the best use of developer time?

Code snips
Provided inline

System observed on:
Ubuntu, Debian

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Some options going forward:

• Run the workflow but don't fail on diffs - put in an advisory role that just tracks the compliance level, and keep an eye on it as new code is merged.
• Occasionally as part of the release process, run the clang-format tool and manually select the chunks that actually do look more readable than it was.

[skliper]

Whatever is chosen, hopefully it doesn't break the use case of applying format to format files. In other words I often lazy-edit, search replace, etc and don't bother w/ formatting. Then apply the format. Saves significant time, especially in early development for me. If I've got to go back and revert all the places where there were human in the loop decisions to not apply the formatting that breaks my workflow (or if main isn't kept as always format compliant). You can apply comment guards if you really want to avoid formatting something (messy, but supports both hand formatting and applying auto formatting to everything). I don't really care about individual merge requests, but hopefully main is kept in sync w/ some way to auto-format (whatever version folks want to use as long as it's available in a reasonable set of modern distributions).

[skliper]

In other words, merges to main could just apply the formatting instead of as a check in every merge request. Either way works for me.

[skliper]

If there's an update to 13, sounds like it'll align columns: https://stackoverflow.com/a/70782265

[CDKnightNASA]

I'm running into issues in trying to match exactly what this workflow is expecting. One option would be to run clang-format as part of the workflow and generate an artifact of the output for those files that are flagged so that us devs can just download the reformatted and splat it in place.

[chillfig]

Thank you! I found that the workflow currently archives:

cFS/.github/workflows/format-check.yml

Lines 58 to 62 in 6cded58

	- name: Archive Static Analysis Artifacts
	uses: actions/upload-artifact@v3
	with:
	name: style_differences
	path: style_differences.txt

To find the archived claang-format style differences artifact called "style_differences.txt":

1. click the "Checks" tab of a github pull request
2. click on the "Format Check" workflow from the left-side menu
3. click and download "style_differences" in the "Artifacts" section

[CDKnightNASA]

To find the archived claang-format style differences artifact called "style_differences.txt":

What I'm looking for is the raw output of a reformatted file, if the differences reported are acceptable to me, I could just splat the whole file with the one from the workflow. But thanks for pointing out that the diff is available, trying to decode indention changes in the workflow output in the web UI is insufficient. :)

[CDKnightNASA]

Also something to consider: // clang-format off (and on). This would be good to demonstrate in, say, the Sample Apps table file. 'course, we have to be cautious this doesn't creep into every line of our code.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code