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

[Bills, Jason M]

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.

> 
>>
>> b. clang-format's -style=file historically required that the style file in
>>     question be called ".clang-format"
>>
>> These two constraints together required that phosphor-mboxd ship two separate
>> configuration files, and to explicitly symlink them as .clang-format before
>> invoking the clang-format command. As such it was easier to reason about code
>> formatting if the support in openbmc-build-scripts transferred the entire
>> formatting responsibility to repository-specific format-code script rather than
>> attempting to do anything of its own accord.
>>
>> Hence, we support 1, 2 and 4, but not (yet) 3.
>>
>> ## Analysis
>>
>> The current behaviour of openbmc-build-scripts is as follows, from
>> scripts/unit-test.py:
>>
>> 1. Check that code formatting was requested. If not, no further code-formatting
>>     action is taken
>>
>> 2. Check for `format-code` and `format-code.sh` in the root of the target
>>     repository and add them to the formatter list
>>
>> 3. If no custom scripts were found in 1, add the default format-code.sh
>>     implementation[1] to the formatter list 4. Execute the scripts in the
>>     formatter list
>>
>> [1] https://github.com/openbmc/openbmc-build-scripts/blob/0ea75ec9efb7ffacb88f63e38fa7823fe8b124a7/scripts/format-code.sh
>>
>> This algorithm is implemented here:
>>
>> https://github.com/openbmc/openbmc-build-scripts/blob/0ea75ec9efb7ffacb88f63e38fa7823fe8b124a7/scripts/unit-test.py#L1215-L1226
>>
>> Confusingly, inside the default format-code.sh implementation there's also an
>> invocation of the custom format-code.sh script if it exists, but the default
>> format-code.sh implementation won't be executed if the custom format-code.sh
>> file exists (due to the implementation of scripts/unit-test.py). So this code
>> is dead:
>>
>> https://github.com/openbmc/openbmc-build-scripts/blob/ac5915f07d3b796f224c85477763ca7fe893dcc2/scripts/format-code.sh#L136-L141
>>
>> The a consequence of all this is that the entity-manager codebase isn't being
>> formatted with clang-format because it ships a custom format-code script that
>> doesn't invoke it.
>>
>> ## Proposal
>>
>> I don't really have one. Does anyone have thoughts on how we differentiate
>> between cases 3 and 4? Use different file names? Invoke the script and ask it
>> what it expects?
>>
>> Interested in your thoughts.
>>
>> Andrew