Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 08 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
> On Tue, Apr 08, 2025 at 11:27:58AM +0300, Jani Nikula wrote:
>> On Mon, 07 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
>> > On Mon, Apr 07, 2025 at 10:17:40AM +0300, Jani Nikula wrote:
>> >
>> >> Even with Jason's idea [1], you *still* have to start small and opt-in
>> >> (i.e. the patch series at hand). You can't just start off by testing
>> >> every header in one go, because it's a flag day switch. 
>> >
>> > You'd add something like 'make header_check' that does not run
>> > automatically. Making it run automatically after everything is fixed
>> > to keep it fixed would be the flag day change. It is how we have
>> > managed to introduce other warning levels in the past.
>> 
>> That approach does not help *me* or drm, i915 and xe in the least. They
>> are already fixed, and we want a way to keep them fixed. This is how all
>> of this got started.
>
> I imagine you'd include a way to have the 'make header_check' run on
> some subset of files only, then use that in your CI for the interm.
>
>> Your goal may be to make everything self-contained, but AFAICS there is
>> no agreement on that goal. As long as there's no buy-in to this, it's
>> not possible fix everything, it's an unreachable goal.
>
> I didn't see that. I saw technical problems with the implementation
> that was presented. I'd be shocked if there was broad opposition to
> adding missing includes and forward declaration to most headers. It is
> a pretty basic C thing. :\

Unless I'm mistaken, both Linus and Masahiro have said they disagree
with headers having to be self-contained as a general rule, regardless
of the issues with kconfig and the build artifacts.

We actually had header checks back in 2019 but it was reverted basically
without discussion with commit fcbb8461fd23 ("kbuild: remove header
compile test"). Sure, there were issues, but still removed without an
attempt to address the issues. Since then it's been skunkworks in
i915. There's a reason this has felt like an uphill battle, and why I'm
reluctant to putting effort into much more than small incremental steps
at a time.

> Until someone sends a series trying to add missing includes and
> forward declarations we can't really know..
>
>> Arguably the situation is similar to W=1 builds. We can't run W=1 in our
>> CI, because of failures outside of the drivers we maintain. 
>
> You can run W=1 using a subdirectory build just for your drivers.

I don't think there's a way to build the entire kernel while limiting
W=1 warnings to a subdirectory, is there? Mixing W=1 and regular builds
causes everything to be rebuilt due to dependencies. It's not only for
CI, it's also for developers.

>> Even if I put in the effort to generalize this the way you prefer, I
>> guess a few kernel releases from now, it still would not do what we have
>> already in place in i915 and xe. And, no offense, but I think your
>> proposal is technically vague to start with. I really don't know where
>> the goal posts are.
>
> Well, I spent a little bit and wrote a mock up and did some looking at
> how much work is here. Focusing on allnoconfig as a starting point,
> 293 out of 1858 headers failed to build, and with some fiddling I got
> it down to 150, a couple of hours would get patches made for the vast
> majority of it.
>
> https://github.com/jgunthorpe/linux/commits/hdrcheck/
>
> I don't see the same dire view as you do, it seems reasonable and doable.

Thanks for the proof-of-concept. It's just that I don't see how that
could be bolted to kbuild, with dependency tracking. I don't want to
have to rebuild the world every time something changes.

Say, I'm refactoring stuff, and I want to ensure headers are okay every
step of the way. git rebase -i origin -x 'make header_check'. How do you
only check the headers whose dependencies were changed since the
previous commit? That requires looking at the .cmd again.


BR,
Jani.


-- 
Jani Nikula, Intel