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

[Patrick Williams <patrick@stwcx.xyz>]

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