About meson clang-tidy issue on some repos

About meson clang-tidy issue on some repos

[Lei Yu]

This email describes a repo-ci issue on some repos related to
clang-tidy and requests for comments to fix this issue.
An example of such an issue is
https://jenkins.openbmc.org/job/ci-repository/100565/console

# Background

1. At first, openbmc-build-scripts uses `run-clang-tidy` to run and
fix the clang-tidy issues. This works ok.
   `run-clang-tidy` will parse the `compile_commands.json` and
**only** run clang-tidy on the specific files to be built.
2. Later, build-scripts changes it to run `ninja clang-tidy` [1]
    This effectively changes the files to run clang-tidy, that in
meson, it will use `git ls-files` and a suffix filter to filter all
the c/c++ like files to run clang-tidy.
    The benefit of this change is that it will run header files as
well, and potentially find more issues.
    However, it does not call "fix" and thus does not automatically
fix the issues found by clang-tidy.
3. Then meson introduces `clang-tidy-fix`, that does call
`run-clang-tidy -fix` internally, and thus it could "fix" the code.
build-scrtips is changed to call `clang-tidy-fix. [2]
    At this point, it introduces a side-effect that `run-clang-tidy`
will filter the files from the `compile_commands.json`, and the hpp
files will be filtered out.
4. Now with meson 1.7.0, it changes the internal `clang-tidy-fix`
behaviors, and it will check all the c/c++ like files again, with
`-fix` support.

# The issue

With the current meson calling "clang-tidy-fix", it introduces another
issue in that it runs **all** the c/c++ like files of the repo, even
if it's not configured to be compiled.

For example, `phosphor-debug-collector` has the
`openpower-dumps-extension` feature disabled by default, and its
`dump-extensions.cpp` includes `openpower_dumps_config.h` that is
generated when the feature is enabled. By default, it's missing, and
it gets a clang-tidy failure on checking this file.

# How to fix

I would like to ask how to fix such issues from OpenBMC's perspective.

Several potential options to discuss:
1. To fix meson's clang-tidy to run only files from `compile_commands.json`.
    Drawback: this way, it will not run clang-tidy on hpp files anymore.
2. Change back build-scripts to call `run-clang-tidy`. This is
effectively the same as option 1.
3. Keep current behavior, and avoid any repo that has a similar case
as `phosphor-debug-collector`.
(https://gerrit.openbmc.org/c/openbmc/phosphor-debug-collector/+/78065
does fix this from the repo)

[1]: https://github.com/openbmc/openbmc-build-scripts/commit/1eb1994bbcceb70d575458dc7a968c0f26b5b6e7
[2]: https://github.com/openbmc/openbmc-build-scripts/commit/ac9c9c7


--
BRs,
Lei YU

Re: About meson clang-tidy issue on some repos

[Patrick Williams]

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

On Tue, Feb 11, 2025 at 04:58:40PM +0800, Lei Yu wrote:
> This email describes a repo-ci issue on some repos related to
> clang-tidy and requests for comments to fix this issue.
> An example of such an issue is
> https://jenkins.openbmc.org/job/ci-repository/100565/console
> 
> # Background
> 
> 1. At first, openbmc-build-scripts uses `run-clang-tidy` to run and
> fix the clang-tidy issues. This works ok.
>    `run-clang-tidy` will parse the `compile_commands.json` and
> **only** run clang-tidy on the specific files to be built.
> 2. Later, build-scripts changes it to run `ninja clang-tidy` [1]
>     This effectively changes the files to run clang-tidy, that in
> meson, it will use `git ls-files` and a suffix filter to filter all
> the c/c++ like files to run clang-tidy.
>     The benefit of this change is that it will run header files as
> well, and potentially find more issues.
>     However, it does not call "fix" and thus does not automatically
> fix the issues found by clang-tidy.
> 3. Then meson introduces `clang-tidy-fix`, that does call
> `run-clang-tidy -fix` internally, and thus it could "fix" the code.
> build-scrtips is changed to call `clang-tidy-fix. [2]
>     At this point, it introduces a side-effect that `run-clang-tidy`
> will filter the files from the `compile_commands.json`, and the hpp
> files will be filtered out.
> 4. Now with meson 1.7.0, it changes the internal `clang-tidy-fix`
> behaviors, and it will check all the c/c++ like files again, with
> `-fix` support.
> 
> # The issue
> 
> With the current meson calling "clang-tidy-fix", it introduces another
> issue in that it runs **all** the c/c++ like files of the repo, even
> if it's not configured to be compiled.
> 
> For example, `phosphor-debug-collector` has the
> `openpower-dumps-extension` feature disabled by default, and its
> `dump-extensions.cpp` includes `openpower_dumps_config.h` that is
> generated when the feature is enabled. By default, it's missing, and
> it gets a clang-tidy failure on checking this file.

Can we just fix the issues?  We merged the fix for the phosphor-debug-collector
one already today.

> 
> # How to fix
> 
> I would like to ask how to fix such issues from OpenBMC's perspective.
> 
> Several potential options to discuss:
> 1. To fix meson's clang-tidy to run only files from `compile_commands.json`.
>     Drawback: this way, it will not run clang-tidy on hpp files anymore.

I think we/you should discuss this issue upstream with Meson.  I'm also
surprised they made this change, not because of the hpp, but because of
dependency issues like the one I fixed in phosphor-debug-collector.

> 2. Change back build-scripts to call `run-clang-tidy`. This is
> effectively the same as option 1.
> 3. Keep current behavior, and avoid any repo that has a similar case
> as `phosphor-debug-collector`.
> (https://gerrit.openbmc.org/c/openbmc/phosphor-debug-collector/+/78065
> does fix this from the repo)

How about we just fix them?  This should be a one-time effort, right?
> 
> [1]: https://github.com/openbmc/openbmc-build-scripts/commit/1eb1994bbcceb70d575458dc7a968c0f26b5b6e7
> [2]: https://github.com/openbmc/openbmc-build-scripts/commit/ac9c9c7
> 
> 
> --
> BRs,
> Lei YU

-- 
Patrick Williams

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

Re: About meson clang-tidy issue on some repos

[Lei Yu]

On Wed, Feb 12, 2025 at 9:42 AM Patrick Williams <patrick@stwcx.xyz> wrote:

> > # The issue
> >
> > With the current meson calling "clang-tidy-fix", it introduces another
> > issue in that it runs **all** the c/c++ like files of the repo, even
> > if it's not configured to be compiled.
> >
> > For example, `phosphor-debug-collector` has the
> > `openpower-dumps-extension` feature disabled by default, and its
> > `dump-extensions.cpp` includes `openpower_dumps_config.h` that is
> > generated when the feature is enabled. By default, it's missing, and
> > it gets a clang-tidy failure on checking this file.
>
> Can we just fix the issues?  We merged the fix for the phosphor-debug-collector
> one already today.

It's not a "fix" but a "workaround". It's always possible for a repo
to have a disabled-by-default config.
Instead, it's more like a bug in meson clang-tidy, that it is not
expected to check the code that is not configured to be built.

>
> >
> > # How to fix
> >
> > I would like to ask how to fix such issues from OpenBMC's perspective.
> >
> > Several potential options to discuss:
> > 1. To fix meson's clang-tidy to run only files from `compile_commands.json`.
> >     Drawback: this way, it will not run clang-tidy on hpp files anymore.
>
> I think we/you should discuss this issue upstream with Meson.  I'm also
> surprised they made this change, not because of the hpp, but because of
> dependency issues like the one I fixed in phosphor-debug-collector.

Agreed. It's a meson clang-tidy issue.
What is your comment on this? Which files are expected to be checked
by clang-tidy?
a. **all** the c-like files in a repo
b. or just the files to be built
c. or ideally, the c-like files (including .h/hpp) configured to be built.

-- 
BRs,
Lei YU

Re: About meson clang-tidy issue on some repos

[Patrick Williams]

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

On Wed, Feb 12, 2025 at 02:23:28PM +0800, Lei Yu wrote:
> On Wed, Feb 12, 2025 at 9:42 AM Patrick Williams <patrick@stwcx.xyz> wrote:

> It's not a "fix" but a "workaround". It's always possible for a repo
> to have a disabled-by-default config.
> Instead, it's more like a bug in meson clang-tidy, that it is not
> expected to check the code that is not configured to be built.

No disagreement.  I also see this as a bug in meson, but don't really
care enough to debate that with them.  If we can fix all our code to
still be successful, we might as well do that.

> Agreed. It's a meson clang-tidy issue.
> What is your comment on this? Which files are expected to be checked
> by clang-tidy?
> a. **all** the c-like files in a repo
> b. or just the files to be built
> c. or ideally, the c-like files (including .h/hpp) configured to be built.

(b), just the files to be built.  I don't understand how you run clang-tidy
on code that doesn't have compile_commands.json entries.  The whole
point of that compile_commands.json is to pass all the same -D
directives, etc. to clang-tidy as you would to the compiler.  There is
no compile_commands.json entries for header files.

-- 
Patrick Williams

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