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

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

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 4134 bytes --]

On Tue, Mar 29, 2022 at 12:36:33PM +1030, Andrew Jeffery wrote:
> ## 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.

I entirely agree that this is ultimately the problem.  The code has
evolved as people add new checks and it has a lot of looking for magic
files to make determinations on what to do.  Unless you know all the
knobs it's hard to discover what is even possible.

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

Do we still have this situation?  (I think this repo is now hiomap which
does seem to have two different clang style files).  Can we just do away
with the "C code should be written in a kernel style" and use the same
format between all the userspace code?

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

I believe (a.) is still the case but not (b.).  We could add yet another
special case to detect the two .clang-format files.

> Hence, we support 1, 2 and 4, but not (yet) 3.
> ## 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?

I'm somewhat surprised still that the difference between 3/4 is hard to
detect.  Is hiomap the only repository expecting behavior 4?

In my opinion if you have a .clang-format, we should run clang-format; if we
don't find .clang-format, we should not run clang-format.  And that
should go for any formatting tool.  I believe we should always treat the
`format-code[.sh]` as yet-another-formatting-option and run it in
addition to everything else that we discover.

There has been talk previously about making something like
`.openbmc/config.json` as a further configuration file where we could
enable / disable all these check.  I think it would be worthwhile as a
way to eliminate many of the "search for special file X" checks we have
where we simply touch an empty file, but I suspect we really shouldn't
be using the "touch a magic empty file" mechanism anyhow.

I've been playing locally with a relatively new tool that seems pretty
promising: https://trunk.io/ .  They have something like the JS
packages-lock.js` that allows you to enable and version-pin all your
style checks and linters and it automatically downloads pristine copies
of those tools (without installing them globally on your system).  They are
missing a few features that would allow it to integrate nicely into our Docker
images, but hopefully it is getting there soon.

Ideally I'd like to see some automation that:

    1. Leverages trunk.io to run all the linters / style checks that
       trunk.io supports, which is way more than we do now.

    2. Create a common set of configs for all these linters and tooling
       detect if some repository has deviated from the common set.

    3. Run a nightly Dependabot-like fix-up commit for any repositories
       with an out-of-date linter version.

This is a longer term project than what you're probably after for this
immediate problem though.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andrew Jeffery]

On Tue, 29 Mar 2022, at 15:56, Patrick Williams wrote:
> On Tue, Mar 29, 2022 at 12:36:33PM +1030, Andrew Jeffery wrote:
>> ## 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.
>
> I entirely agree that this is ultimately the problem.  The code has
> evolved as people add new checks and it has a lot of looking for magic
> files to make determinations on what to do.  Unless you know all the
> knobs it's hard to discover what is even possible.

Right.

>
>> 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.
>
> Do we still have this situation?  (I think this repo is now hiomap which
> does seem to have two different clang style files).  Can we just do away
> with the "C code should be written in a kernel style" and use the same
> format between all the userspace code?

This was just some context for what we have, I wasn't looking to solve 
this particular problem, so I'm going to leave this discussion for 
another time.

>
>> 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"
>
> I believe (a.) is still the case but not (b.). 

Yep.

> We could add yet another
> special case to detect the two .clang-format files.

Let's try to avoid this.

>
>> Hence, we support 1, 2 and 4, but not (yet) 3.
>> ## 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?
>
> I'm somewhat surprised still that the difference between 3/4 is hard to
> detect.  Is hiomap the only repository expecting behavior 4?

I don't know, but I'm not sure it matters.

>
> In my opinion if you have a .clang-format, we should run clang-format; if we
> don't find .clang-format, we should not run clang-format.  And that
> should go for any formatting tool.

I think this is the crux of it.

>  I believe we should always treat the
> `format-code[.sh]` as yet-another-formatting-option and run it in
> addition to everything else that we discover.

Thanks for the clarity here. I kinda got through figuring out what had 
happened and ran out of energy to think about neat ways to resolve it.

>
> There has been talk previously about making something like
> `.openbmc/config.json` as a further configuration file where we could
> enable / disable all these check.  I think it would be worthwhile as a
> way to eliminate many of the "search for special file X" checks we have
> where we simply touch an empty file, but I suspect we really shouldn't
> be using the "touch a magic empty file" mechanism anyhow.

This is related but is starting to feel a little tangential. I think we 
can get away without trying to switch things to a json config for now?

Thanks for the feedback. Should I push a patch to capture this as a 
design doc?

Andrew

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

[Ed Tanous]

On Mon, Mar 28, 2022 at 7:08 PM Andrew Jeffery <andrew@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

Re: 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@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

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

On Tue, Mar 29, 2022 at 09:22:01PM +1030, Andrew Jeffery wrote:
> On Tue, 29 Mar 2022, at 15:56, Patrick Williams wrote:
> > There has been talk previously about making something like
> > `.openbmc/config.json` as a further configuration file where we could
> > enable / disable all these check.  I think it would be worthwhile as a
> > way to eliminate many of the "search for special file X" checks we have
> > where we simply touch an empty file, but I suspect we really shouldn't
> > be using the "touch a magic empty file" mechanism anyhow.
> 
> This is related but is starting to feel a little tangential. I think we 
> can get away without trying to switch things to a json config for now?

I wasn't sure the scope of what you wanted to tackle right now so I was
giving you the full-spectrum of my thoughts.  We could certainly make a
tactical solution that resolves the issue in EM, but I think we still
have a mess on our hands w.r.t. specifying what linters and formatters
are ran in CI.  Random dot files, deviations in formatting rules between
languages, and undocumented combinations of testing are all artifacts of
the mess, but that mess doesn't necessarily need to be cleaned up today.

(*) I'll make the argument once again that having to support autotools,
CMake, and Meson in our CI additionally makes this mess even bigger.
You'll notice there are actually deviations in what gets tested and how
between the 3.

> Thanks for the feedback. Should I push a patch to capture this as a 
> design doc?

If all we're doing is making it so that `format-code` always runs, I
don't think this warrants a design document.  If you're looking to
tackle something larger that probably does belong in a design document.
Documenting the as-is shouldn't be in a design document, in my opinion,
and trying to frame it as a design is probably going to generate a lot
more discussion on ideal vs present than is useful if we're not going to
make much investment in the implementation.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andrew Jeffery]

On Wed, 30 Mar 2022, at 01:59, Ed Tanous wrote:
> On Mon, Mar 28, 2022 at 7:08 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>>
>> 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.

IIRC Lei added the behaviour so if you ran the docker container locally 
for testing you could disable code formatting.

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

No, because we have other formatters too.

Anyway, this is just the background section that justifies how we got 
to where we are. Changing the recommendation for C code formatting 
wasn't something I was looking to debate here.

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

We could, but that's not what's currently documented:

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

Andrew

Re: 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@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

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

[Andrew Jeffery]

On Wed, 30 Mar 2022, at 03:28, Patrick Williams wrote:
> On Tue, Mar 29, 2022 at 09:22:01PM +1030, Andrew Jeffery wrote:
>> On Tue, 29 Mar 2022, at 15:56, Patrick Williams wrote:
>> > There has been talk previously about making something like
>> > `.openbmc/config.json` as a further configuration file where we could
>> > enable / disable all these check.  I think it would be worthwhile as a
>> > way to eliminate many of the "search for special file X" checks we have
>> > where we simply touch an empty file, but I suspect we really shouldn't
>> > be using the "touch a magic empty file" mechanism anyhow.
>> 
>> This is related but is starting to feel a little tangential. I think we 
>> can get away without trying to switch things to a json config for now?
>
> I wasn't sure the scope of what you wanted to tackle right now so I was
> giving you the full-spectrum of my thoughts. 

Yep, that's fine, I was just trying to indicate which bits of the 
conversation I hoping to avoid focus on. However:

> We could certainly make a
> tactical solution that resolves the issue in EM, but I think we still
> have a mess on our hands w.r.t. specifying what linters and formatters
> are ran in CI.  Random dot files, deviations in formatting rules between
> languages, and undocumented combinations of testing are all artifacts of
> the mess

Can you be more concrete in your concerns here? Can we enumerate them so
we can make the call on whether to tackle them or not? Reading this my
thoughts were:

* Random dot files: Are we labeling e.g. .clang-format as "random"? Or
  specifically other things, not that? What specific files are we concerned
  about?

* Deviations from formatting rules between languages: Is this aimed at C
  vs C++? You can't really apply C++ formatting to e.g. Python, so I'm
  not sure what distinctions your making here

* Undocumented combinations of testing: Is this a comparative statement
  between support for the different build systems? Or something else?

Andrew