Defining the behaviour of code formatting in openbmc-build-scripts

[Andrew Jeffery]

On Wed, 30 Mar 2022, at 02:48, Bills, Jason M wrote:
> On 3/29/2022 9:29 AM, Ed Tanous wrote:
>> On Mon, Mar 28, 2022 at 7:08 PM Andrew Jeffery <andrew at aj.id.au> wrote:
>>>
>>> Hello,
>>>
>>> Review of https://gerrit.openbmc-project.xyz/c/openbmc/entity-manager/+/52406
>>> sparked some discussion about what we actually want from the code-formatting
>>> support in openbmc-build-scripts going forward.
>>>
>>> ## General statements
>>>
>>> Enforcing code formatting in CI is effective in that it ensures style from
>>> multiple contributors is consistent with minimal maintainer intervention.
>>> Sometimes the automated formatting might be questionable, but the judgement is
>>> that on the whole the consistency is a better trade-off than occasional
>>> enforced untidiness. There are also escape hatches to disable formatting in
>>> extreme circumstances. Automated code formatting is desirable.
>>>
>>> clang-format is at the heart of OpenBMC's code formatting support as the bulk
>>> of the OpenBMC codebase is written in C++.
>>>
>>> ## The problem
>>>
>>> "Like all Vogon ships, it looked as if it had been not so much designed, as
>>> congealed." - Douglas Adams, The Salmon of Doubt
>> 
>> Don't panic.
>> 
>>>
>>>
>>> Code formatting support in openbmc-build-scripts has evolved over time and
>>> no-one has ever written down what we actually want. The lack of concrete
>>> requirements has lead to an counter-intuitive and convoluted implementation
>>> that has ended in some repositories (e.g. entity-manager) not having their code
>>> formatted as expected.
>>>
>>> There are 4 possible behaviours that openbmc-build-scripts could take against
>>> an application repository:
>>>
>>> 1. All code formatting is disabled: No further action
>> 
>> Long term, is there a use case for this?  I realize it's required
>> temporarily while we make all the repos consistent, but it seems like
>> it should ideally be a temporary situation for portions of the
>> codebase, and something that eventually we can just enforce globally
>> in the project.
>> 
>>>
>>> 2. No custom code formatting: Just do whatever the default actions are as
>>>     implemented in openbmc-build-scripts
>>>
>>> 3. Some custom code formatting: Actions to run in addition to the default
>>>     actions implemented in openbmc-build-scripts
>>>
>>> 4. Only custom code formatting: Actions to run in-place of the default actions
>>>     implemented in openbmc-build-scripts (which must not be run)
>>>
>>> Currently openbmc-build-scripts only implements 1, 2 and 4, while
>>> entity-manager had assumed 3 was supported.
>>>
>>> ## Background (AKA why don't we have support for 3?)
>>>
>>> Code formatting support in openbmc-build-scripts began with an implementation
>>> of 1. However, along the way we introduced the phosphor-mboxd repository which
>>> due to some unfortunate history has a mixed C and C++ codebase. The C code is
>>> written in kernel style, while it was desired that the C++ be written in
>>> OpenBMC style.
>>>
>>> At the time the problem arose, clang-format had two issues:
>>>
>>> a. It treats C and C++ as the same language, and so maintaining a code
>>>     formatting split across those language boundaries requires two separate
>>>     clang-format configuration files
>> 
>> Is the core of the issue here that we have different formatting rules
>> for C than C++?  clang-format is entirely whitespace changes, so could
>> we solve this simply by just reformatting all the C code to the common
>> project style, and we wouldn't need this anymore?  It seems like
>> regardless of C vs C++ we should be consistent.
> I agree with this. I have some C code that I just let format according 
> to our project style, and it makes formatting much simpler.

There is this:

https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#c

Though it does give you the flexibility to do what you're doing.

Andrew