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

[Andrew Jeffery]

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

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

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

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