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