Fix #39, Clang-format-10 fails in Travis CI

[astrogeco]

Describe the contribution
Fix #39

Testing performed
Ran CI

Expected behavior changes
CI builds
No more warnings in config section of travis

System(s) tested on
CI - Ubuntu Bionic

Contributor Info - All information REQUIRED for consideration of pull request
Gerardo E. Cruz-Ortiz NASA/GSFC

[jphickey]

Currently this is failing on some whitespace differences.

We should really reconsider whether this should be "enforcing" or not... particularly when the original code is reasonably readable to start with.

I've also seen clang-format make it worse/less readable when it changes around line breaks. We really shouldn't be enforcing the exact output of clang-format and nothing else, it isn't a perfect tool. Run it in an "advisory" mode but not an enforcing mode.

[astrogeco]

@jphickey after fixing the ci pipeline in this PR I noticed your changes for ci_lab that are currently sitting in ic-20200304 don't meet the code standards. Can you fix it?

[skliper]

Likely need similar changes to cFS (current and enhanced CI)

[jphickey]

@jphickey after fixing the ci pipeline in this PR I noticed your changes for ci_lab that are currently sitting in ic-20200304 don't meet the code standards. Can you fix it?

@astrogeco you'll need to update this PR to include commit e61830f (I pushed this to `ic-20200304, but I cannot push to your fork). I merged this in a test branch and it "passed" CI.

BUT...
I said it before and I'll say it again, enforcing clang-format is going to be a major headache, and I recommend re-discussing this and the issues it poses.

In many cases changing or removing/adding one line causes the so-called "correct" format (whitespace) of adjacent lines to change, even though the original changeset did not even touch those lines. This perpetual reformatting of source code based on nearby but unrelated changes is going to cause significant, repeated, and unnecessary merge conflicts in the future.

Not only that, it means our so-called "code standard" is solely based on the implementation of a third party tool, and the very existence of this PR is reason why this is a problem (case in point - our CI scripts broke due to an upstream repo change, out of our control). Also if clang-format-11 formats a line differently than clang-format-10 does, we are suddenly "noncompliant" all over again.

I'm marking my review as "request changes" because I firmly believe that enforcing exact compliance with clang-format is going to be problematic. This shouldn't restrict your ability to merge it anyway, if you so choose.

[astrogeco]

I think this is a good topic to bring up at the CCB. So I'll delay until then.

[jphickey]

I think this is a good topic to bring up at the CCB. So I'll delay until then.

To illustrate the point, consider this line of code in master, originally at line 92, which "passed" CI:

ci_lab/fsw/src/ci_lab_msg.h

Line 92 in 612bdfc

uint8 TlmHeader[CFE_SB_TLM_HDR_SIZE];

The changeset actually did not touch this specific line, it still existed exactly as it was in the original ic-20200304 branch but its line number changed to 72:

ci_lab/fsw/src/ci_lab_msg.h

Line 78 in 87c736e

uint8 TlmHeader[CFE_SB_TLM_HDR_SIZE];

However, this unchanged line is now flagged as noncompliant, and clang-format wants to add more space, due to a change in the line after it. This is how the same looks in the updated version which is now "passing" CI.

ci_lab/fsw/src/ci_lab_msg.h

Line 77 in e61830f

uint8 TlmHeader[CFE_SB_TLM_HDR_SIZE];

Note that this pull request did some substantial refactoring to begin with, so it is probably a bad example, but hopefully this helps understand how perpetually running code beautifier tools will have unintended consequences for branching and merging, as a "correctly formatted line" is suddenly a moving target.

[skliper]

I think this is a good topic to bring up at the CCB. So I'll delay until then.

I disagree. It's white space, enforced with a one-liner, ensures compliance with the coding style requirement (critical for certification), and we've covered it multiple times in the past. It doesn't matter if the style changes, simply apply the latest as enforced per CI when checking in. This is a solved problem, and I would prefer we dedicate the CCB to more important topics.

[jphickey]

I strongly disagree that this is a "solved problem"

simply apply the latest as enforced per CI when checking in

While yes someone can just "run the tool" - the point is that the tool creates lots of whitespace changes which are generally detrimental to diff / merge / rebase operations. The definition of "correct" whitespace is also dependent on all the other lines around it, meaning if change (a) and change (b) are both compliant, it is very easy to get cases where the merge of (a+b) becomes suddenly non-compliant.

My position is that we should employ the tool in an advisory mode (i.e. have it help us find areas where the style is actually bad) and use it to help fix them, but NOT with every commit, due to the (very real) issues it causes for merging/diffing/etc.

[astrogeco]

@skliper that's a good point. I didn't realize this has been discussed before. So I'll take it off the list and merge. Let's disagree and commit to it I think the benefits outweigh the small annoyances with line numbers etc. We can't make an informed judgement until we roll out clang-format across the repos and start working with it. If it becomes a problem we can always disable it.

[skliper]

@jphickey I do understand the extra effort and care required to comply with style enforcement. I also understand how an inexperienced or undisciplined contributor could mix white-space with actual changes. Luckily github (and every other diff tool I frequently use) provides an option to ignore white space changes during review.

The enforcement isn't targeted towards experienced FSW developers. Unfortunately past, current, and future contributors don't all have that experience or practice consistent style implementation. The overhead of compliance and concerns you bring up were considered in the original trade and the benefits of enforcement outweigh the lack of.

Will the enforce style change? Yes, it's a given based on the dependency and version chosen and no that does not break the process. Version 10 was picked since it resolved significant style issues related to more established versions, with the understanding it is less stable in the short term. As it solidifies and releases progress we could lock down on a version to minimize style changes if style changes somehow become a significant impact on progress.

Suggestions on improvements to the process will certainly be considered and I welcome them. That said, expect less standards/guidelines/requirements compliant solutions to be discouraged.

[skliper]

As we roll out the enforcement, I'm certainly open to re-evaluating the definition file if we run into un-workable auto-formatting cases. I've yet to see an example of terrible auto-formatting in our code base, or anything close to the lack of formatting in the current repos.

[jphickey]

@skiper it is not an issue with simply viewing diffs, we all know how to hide whitespace-only changes. It is merging and rebasing and pretty much everything else where this will create undue conflicts.

It's fine - I get the hint, we will leave this enforced. But in the future, particularly with the larger codebases, we WILL see instances where:

1. The robot-formatter doesn't do it right, and actually makes it uglier/less readable than the human formatted version. (This is from personal experience just with the some CFE apps I support; I had an open mind to this at first, and ran a few through the tool, but had to manually fixup some oddball things it did. Overall it was OK and a net cleanup, but some C++ code got very oddly handled).

2. Merge conflicts are created in code that otherwise would merge cleanly. Also you'll find that even in code that does merge cleanly, it causes the format to become invalid, requiring more manual intervention.

My position is, and will remain, that humans reviewing the code should make the authoritative decision on what is most readable and acceptably formatted per the guidelines. While I concur a robot consistently applies the rules, its role should be to inform the human reviewers of areas where the formatting is bad, not to be the absolute authority and prevent merging of code. (because robots DO make mistakes).

Again, not saying don't use the tool, it certainly makes some substantial improvements in many areas. I'm just saying we should apply it strategically, with human guidance, at certain points during development, not with every commit. You'll get the same level of compliance in released versions without as many intermediate merging issues.