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

[Ed Tanous]

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.

>
> 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