Re: About meson clang-tidy issue on some repos

[Patrick Williams <patrick@stwcx.xyz>]

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