Message ID | CAPM=9tyx=edsZ3ajuAUAv4vjfa=WNEzobqAsYbBTjCfLbuEeFQ@mail.gmail.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [git,pull] drm for 6.15-rc1 | expand |
On Thu, 27 Mar 2025 at 19:53, Dave Airlie <airlied@gmail.com> wrote: > > This is the main drm pull request for 6.15. Bit late, but my wife was > away getting a PhD and kids took over my writing summaries time, and > fd.o was offline last week which slowed me down a small bit. Grr. I did the pull, resolved the (trivial) conflicts, but I notice that this ended up containing the disgusting "hdrtest" crap that (a) slows down the build because it's done for a regular allmodconfig build rather than be some simple thing that you guys can run as needed (b) also leaves random 'hdrtest' turds around in the include directories People already complained separately about this. It should never have made it to me in this broken form. Why the heck is this testing being done as a regular part of the build? And dammit we don't add random turd files for dependencies that then make the source tree nasty. The thing that made me notice that it was still there was that "git status" complains about the stupid turds not being ignored. But more importantly, those turds also break filename completion! So no, adding it to gitignore doesn't actually fix the problem, it would just have made me not notice as quickly. This thing needs to *die*. If you want to do that hdrtest thing, do it as part of your *own* checks. Don't make everybody else see that disgusting thing and have those turds in their trees. I'll just disable it by marking it BROKEN for now. You guys can figure out what you want to do, but no, forcing others to see those things is not the answer. I would suggest you *not* make this part of the Kconfig setup and normal build at all, but be something where *you* can run it as part of your tests (ie do it as a "make drm-hdrtest" kind of thing, not as part of regular builds). Linus
The pull request you sent on Fri, 28 Mar 2025 12:53:20 +1000:
> https://gitlab.freedesktop.org/drm/kernel.git tags/drm-next-2025-03-28
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0c86b42439b6c11d758b3392a21117934fef00c1
Thank you!
On Fri, 28 Mar 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote: > If you want to do that hdrtest thing, do it as part of your *own* > checks. Don't make everybody else see that disgusting thing and have > those turds in their trees. > > I'll just disable it by marking it BROKEN for now. You guys can figure > out what you want to do, but no, forcing others to see those things is > not the answer. Fair. I hear you. > I would suggest you *not* make this part of the Kconfig setup and > normal build at all, but be something where *you* can run it as part > of your tests (ie do it as a "make drm-hdrtest" kind of thing, not as > part of regular builds). I would very much prefer for this to be part of the build, just hidden behind Kconfig. We're doing build-time checks, and kbuild gives us all the machinery to make it happen. Without the dependency tracking you'd have to check everything every time, and that's just going to mean people won't run it. I suggest a Kconfig knob to truly make this opt-in, only for developers who actually want it. Not enabled by allmodconfig or allyesconfig or even allnoconfig. Only if you manually enable it. And yes, that's how it should've been from the start. My bad. Below's a patch to make it happen. We'll probably want to add more checks like this in the future. We want to catch a whole bunch of build issues up front. We want to be clean of e.g. W=1 and kernel-doc issues pre-merge instead of doing extra rounds of fixes afterwards. BR, Jani. From 8c709510caab4b4ad6aa73cbcd972f32b58cad8d Mon Sep 17 00:00:00 2001 From: Jani Nikula <jani.nikula@intel.com> Date: Mon, 31 Mar 2025 12:25:45 +0300 Subject: [PATCH] drm: add config option for extra build-time checks Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Cc: Jani Nikula <jani.nikula@intel.com> The DRM subsystem contains additional build-time checks, primarily aimed at DRM developers and CI systems. The checks may be overzealous. They may slow down or fail the build altogether. They may create excessive dependency files in the build tree. They should not be enabled for regular builds, and certainly not forced on unsuspecting developers running an allyesconfig or allmodconfig build. Add config DRM_DISABLE_EXTRA_BUILD_CHECKS, enabled by default as well as by allyesconfig/allmodconfig, hiding the extra checks from anyone but people who intentionally opt-in for the checks. For example, to enable header tests: $ scripts/config --disable CONFIG_DRM_DISABLE_EXTRA_BUILD_CHECKS --enable CONFIG_DRM_HEADER_TEST $ make olddefconfig Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Closes: https://lore.kernel.org/r/CAHk-=wjcdfrDTjzm6J6T-3fxtVyBG7a_0BXc2=mgOuM6KPFnCg@mail.gmail.com Fixes: 62ae45687e43 ("drm: ensure drm headers are self-contained and pass kernel-doc") Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/Kconfig | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2cba2b6ebe1c..5a3fce9ef998 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -489,9 +489,26 @@ config DRM_PRIVACY_SCREEN bool default n +# Reversed option to disable on allyesconfig/allmodconfig builds +config DRM_DISABLE_EXTRA_BUILD_CHECKS + bool "Disable DRM subsystem extra build-time checks" + default y + help + The DRM subsystem contains additional build-time checks, primarily + aimed at DRM developers and CI systems. The checks may be + overzealous. They may slow down or fail the build altogether. They may + create excessive dependency files in the tree. They should not be + enabled for regular builds, and thus they are disabled by default. + +# Proxy config to allow simple "depends on DRM_EXTRA_BUILD_CHECKS" +config DRM_EXTRA_BUILD_CHECKS + bool + depends on DRM && EXPERT && DRM_DISABLE_EXTRA_BUILD_CHECKS=n + default !DRM_DISABLE_EXTRA_BUILD_CHECKS + config DRM_WERROR bool "Compile the drm subsystem with warnings as errors" - depends on DRM && EXPERT + depends on DRM_EXTRA_BUILD_CHECKS depends on !WERROR default n help @@ -505,7 +522,7 @@ config DRM_WERROR config DRM_HEADER_TEST bool "Ensure DRM headers are self-contained and pass kernel-doc" - depends on DRM && EXPERT && BROKEN + depends on DRM_EXTRA_BUILD_CHECKS default n help Ensure the DRM subsystem headers both under drivers/gpu/drm and
Hi Linus, On Mon, Mar 31, 2025 at 01:17:28PM +0300, Jani Nikula wrote: > On Fri, 28 Mar 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > If you want to do that hdrtest thing, do it as part of your *own* > > checks. Don't make everybody else see that disgusting thing and have > > those turds in their trees. > > > > I'll just disable it by marking it BROKEN for now. You guys can figure > > out what you want to do, but no, forcing others to see those things is > > not the answer. > > Fair. I hear you. Mea culpa also from the drm maintainer side. As Jani explains below, we really want to make this and similar things happen in drm, and we thought this much more limited version would address your concerns. Evidently not. We also got a heads-up from the Kbuild maintainer that you really don't like this, and I guess that wasn't clear enough in the pr cover letter. Plus at that point it was all already in drm-next, and we figured it's not worth the trouble of pulling it out and doing a separate topic branch pr so you can check it out separately. Hindsight and all that. Cheers, Sima > > > I would suggest you *not* make this part of the Kconfig setup and > > normal build at all, but be something where *you* can run it as part > > of your tests (ie do it as a "make drm-hdrtest" kind of thing, not as > > part of regular builds). > > I would very much prefer for this to be part of the build, just hidden > behind Kconfig. We're doing build-time checks, and kbuild gives us all > the machinery to make it happen. Without the dependency tracking you'd > have to check everything every time, and that's just going to mean > people won't run it. > > I suggest a Kconfig knob to truly make this opt-in, only for developers > who actually want it. Not enabled by allmodconfig or allyesconfig or > even allnoconfig. Only if you manually enable it. And yes, that's how it > should've been from the start. My bad. > > Below's a patch to make it happen. We'll probably want to add more > checks like this in the future. We want to catch a whole bunch of build > issues up front. We want to be clean of e.g. W=1 and kernel-doc issues > pre-merge instead of doing extra rounds of fixes afterwards. > > BR, > Jani. > > > > From 8c709510caab4b4ad6aa73cbcd972f32b58cad8d Mon Sep 17 00:00:00 2001 > From: Jani Nikula <jani.nikula@intel.com> > Date: Mon, 31 Mar 2025 12:25:45 +0300 > Subject: [PATCH] drm: add config option for extra build-time checks > Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo > Cc: Jani Nikula <jani.nikula@intel.com> > > The DRM subsystem contains additional build-time checks, primarily aimed > at DRM developers and CI systems. The checks may be overzealous. They > may slow down or fail the build altogether. They may create excessive > dependency files in the build tree. They should not be enabled for > regular builds, and certainly not forced on unsuspecting developers > running an allyesconfig or allmodconfig build. > > Add config DRM_DISABLE_EXTRA_BUILD_CHECKS, enabled by default as well as > by allyesconfig/allmodconfig, hiding the extra checks from anyone but > people who intentionally opt-in for the checks. > > For example, to enable header tests: > > $ scripts/config --disable CONFIG_DRM_DISABLE_EXTRA_BUILD_CHECKS --enable CONFIG_DRM_HEADER_TEST > $ make olddefconfig > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Closes: https://lore.kernel.org/r/CAHk-=wjcdfrDTjzm6J6T-3fxtVyBG7a_0BXc2=mgOuM6KPFnCg@mail.gmail.com > Fixes: 62ae45687e43 ("drm: ensure drm headers are self-contained and pass kernel-doc") > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/Kconfig | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 2cba2b6ebe1c..5a3fce9ef998 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -489,9 +489,26 @@ config DRM_PRIVACY_SCREEN > bool > default n > > +# Reversed option to disable on allyesconfig/allmodconfig builds > +config DRM_DISABLE_EXTRA_BUILD_CHECKS > + bool "Disable DRM subsystem extra build-time checks" > + default y > + help > + The DRM subsystem contains additional build-time checks, primarily > + aimed at DRM developers and CI systems. The checks may be > + overzealous. They may slow down or fail the build altogether. They may > + create excessive dependency files in the tree. They should not be > + enabled for regular builds, and thus they are disabled by default. > + > +# Proxy config to allow simple "depends on DRM_EXTRA_BUILD_CHECKS" > +config DRM_EXTRA_BUILD_CHECKS > + bool > + depends on DRM && EXPERT && DRM_DISABLE_EXTRA_BUILD_CHECKS=n > + default !DRM_DISABLE_EXTRA_BUILD_CHECKS > + > config DRM_WERROR > bool "Compile the drm subsystem with warnings as errors" > - depends on DRM && EXPERT > + depends on DRM_EXTRA_BUILD_CHECKS > depends on !WERROR > default n > help > @@ -505,7 +522,7 @@ config DRM_WERROR > > config DRM_HEADER_TEST > bool "Ensure DRM headers are self-contained and pass kernel-doc" > - depends on DRM && EXPERT && BROKEN > + depends on DRM_EXTRA_BUILD_CHECKS > default n > help > Ensure the DRM subsystem headers both under drivers/gpu/drm and > -- > 2.39.5 > > > -- > Jani Nikula, Intel
On Mon, Mar 31, 2025 at 01:03:38PM +0200, Simona Vetter wrote: > Hi Linus, > > On Mon, Mar 31, 2025 at 01:17:28PM +0300, Jani Nikula wrote: > > On Fri, 28 Mar 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > If you want to do that hdrtest thing, do it as part of your *own* > > > checks. Don't make everybody else see that disgusting thing and have > > > those turds in their trees. > > > > > > I'll just disable it by marking it BROKEN for now. You guys can figure > > > out what you want to do, but no, forcing others to see those things is > > > not the answer. > > > > Fair. I hear you. > > Mea culpa also from the drm maintainer side. As Jani explains below, we > really want to make this and similar things happen in drm, and we thought > this much more limited version would address your concerns. Please don't keep it fully isolated to DRM.. This new stuff did find an error in the fwctl UAPI headers around uuid_t that had gone unnoticed: https://lore.kernel.org/all/f6489337-67c7-48c8-b48a-58603ec15328@paulmck-laptop/raw I think that was a valuable report, you just need to find a way to make the tests it runs more acceptable.. FWIW, there is a "trick" I like to use for C header files, just ensure that some C file someplace includes each header file first in the #include list. It automatically makes the compiler check it is self contained naturally. You can get pretty far by paying attention to this detail and it costs nothing at build time. Jason
On Mon, 31 Mar 2025 at 03:17, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > I suggest a Kconfig knob to truly make this opt-in, only for developers > who actually want it. So honestly, the thing I *really* hated was the horrendous naming. I live in auto-complete. I basically never type out file-names, and I replace keyboards every once in a while because my TAB key has worn down (not really, but you get the idea). And *if* this feature is useful - and while I disagree about the whole "header files have to be self-sufficient" as a general rule, I can see it being useful for uapi headers - then dammit, the file naming needs to *DIE*. It needs to be taken out behind the shed and shot in the head, because it messes with TAB-completion. Having "this has been checked" turds that live in the same name-space as real files is wrong. In the kernel, we often hide them explicitly (ie using the dot prefix to make them hide, but also to make them not mess with auto-complete). That's an option. But some people hate the hiding, and if that's an issue, just put it all in a different subdirectory entirely. Yes, I realize that you guys may live in the whole "different subdirectory entirely" world of doing the whole build in a separate build directory, and might say "why are you working in the same tree as the generated files in the first place if auto-complete is so important to you". And to that I say "because I equally often look at the generated files". When they make *sense* to look at, not when they are auto-generated makefile checking crap. So please. This feature needs to be done completely differently. Linus
On Mon, 31 Mar 2025, Jason Gunthorpe <jgg@nvidia.com> wrote: > Please don't keep it fully isolated to DRM.. This new stuff did find > an error in the fwctl UAPI headers around uuid_t that had gone unnoticed: > > https://lore.kernel.org/all/f6489337-67c7-48c8-b48a-58603ec15328@paulmck-laptop/raw > > I think that was a valuable report, you just need to find a way to > make the tests it runs more acceptable.. The header checks have existed for uapi headers before, including the, uh, turds, but apparently adding them in drm broke the camel's back. > FWIW, there is a "trick" I like to use for C header files, just ensure > that some C file someplace includes each header file first in the > #include list. It automatically makes the compiler check it is self > contained naturally. You can get pretty far by paying attention to > this detail and it costs nothing at build time. It's a fairly good solution for a lot of cases, but it falls a bit short. I'd additionally like to ensure: - Header guards are in place - There are no kernel-doc warnings - Headers not associated 1:1 with a .c file are also checked Finally, the cost of having to keep checking the headers are in fact included first, and nagging about it in reviews, is not without cost. BR, Jani.
On Mon, 31 Mar 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Mon, 31 Mar 2025 at 03:17, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> I suggest a Kconfig knob to truly make this opt-in, only for developers >> who actually want it. > > So honestly, the thing I *really* hated was the horrendous naming. > > I live in auto-complete. I basically never type out file-names, and I > replace keyboards every once in a while because my TAB key has worn > down (not really, but you get the idea). > > And *if* this feature is useful - and while I disagree about the whole > "header files have to be self-sufficient" as a general rule, I can see > it being useful for uapi headers - then dammit, the file naming needs > to *DIE*. It needs to be taken out behind the shed and shot in the > head, because it messes with TAB-completion. > > Having "this has been checked" turds that live in the same name-space > as real files is wrong. > > In the kernel, we often hide them explicitly (ie using the dot prefix > to make them hide, but also to make them not mess with auto-complete). > That's an option. But some people hate the hiding, and if that's an > issue, just put it all in a different subdirectory entirely. > > Yes, I realize that you guys may live in the whole "different > subdirectory entirely" world of doing the whole build in a separate > build directory, and might say "why are you working in the same tree > as the generated files in the first place if auto-complete is so > important to you". > > And to that I say "because I equally often look at the generated > files". When they make *sense* to look at, not when they are > auto-generated makefile checking crap. > > So please. This feature needs to be done completely differently. I've polished, or, more accurately, hidden the disgusting turds [1]. I hope this is an acceptable approach. I've never claimed this feature is universally useful, certainly not for all of include/linux, but I think it has been more helpful than not in i915 and xe drivers. And now hopefully drm more widely. I wish kbuild had support for it in a way that drivers could opt-in *if* they wanted it, but without copy-pasting the boilerplate. BR, Jani. [1] https://lore.kernel.org/r/20250401121830.21696-1-jani.nikula@intel.com
On Tue, 1 Apr 2025 at 05:21, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > The header checks have existed for uapi headers before, including the, > uh, turds, but apparently adding them in drm broke the camel's back. The uapi header test things never caused any problems for me [*], because they don't actually pollute the source tree. Why? Because they all end up in that generated 'usr/include/' directory. So when I look at the source files, filename completion is entirely unaffected, and it all works fine. Look, I can complete something like include/uapi/asm-generic/poll.h perfectly fine, because there is *not* some generated turd that affects it all. Because for the uapi files those hdrtest files end up being in ./usr/include/asm-generic/poll.hdrtest and I never have any reason to really look at that subdirectory at all, since it's all generated. Or put another way - if I _were_ to look at it, it would be exactly because I want to see some generated file, in which case the 'hdrtest' turd would be part of it. (Although I cannot recall that ever having actually happened, to be honest - but looking at various header files is common, and I hit the drm case immediately) Would you mind taking more of that uapi approach than creating that hidden directory specific to the drm tree? Maybe this could *all* be generalized? Linus [*] I say "never caused any problems for me", but maybe it did way in the past and it was fixed and I just don't recall. I have definitely complained about pathname completion issues to people before.
On Wed, Apr 2, 2025 at 1:12 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, 1 Apr 2025 at 05:21, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > The header checks have existed for uapi headers before, including the, > > uh, turds, but apparently adding them in drm broke the camel's back. > > The uapi header test things never caused any problems for me [*], > because they don't actually pollute the source tree. > > Why? Because they all end up in that generated 'usr/include/' directory. > > So when I look at the source files, filename completion is entirely > unaffected, and it all works fine. > > Look, I can complete something like > > include/uapi/asm-generic/poll.h > > perfectly fine, because there is *not* some generated turd that affects it all. > > Because for the uapi files those hdrtest files end up being in > > ./usr/include/asm-generic/poll.hdrtest > > and I never have any reason to really look at that subdirectory at > all, since it's all generated. > > Or put another way - if I _were_ to look at it, it would be exactly > because I want to see some generated file, in which case the 'hdrtest' > turd would be part of it. > > (Although I cannot recall that ever having actually happened, to be > honest - but looking at various header files is common, and I hit the > drm case immediately) > > Would you mind taking more of that uapi approach than creating that > hidden directory specific to the drm tree? Maybe this could *all* be > generalized? > > Linus > > [*] I say "never caused any problems for me", but maybe it did way in > the past and it was fixed and I just don't recall. I have definitely > complained about pathname completion issues to people before. I know you dislike this, and I NACKed this before (but too late): https://lore.kernel.org/dri-devel/CAK7LNATHXwEkjJHP7b-ZmhzLfyyuOdsyimna-=r-sJk+DxigrA@mail.gmail.com/ Compile-testing UAPI headers is useful (and I believe this is the only case where it is useful) because userspace is independent of any CONFIG option, and we have no control over the include order in userspace programs. In contrast, kernel-space headers are conditionally parsed, depending on CONFIG options. You dislike this feature for the reason of TAB-completion, but I am negative about this feature from another perspective. We cannot avoid a false-positive: https://lore.kernel.org/all/20190718130835.GA28520@lst.de/ It is not <drm/*.h> but <linux/*.h> However, it is annoying to make every header self-contained "just because we are checking this". Actually, it is not a real problem when the relevant CONFIG option is disabled. I did not interrupt him because he was doing this checkunder drivers/gpu/drm/i915/. () But, I am opposed to expanding it to more-global include/drm/, or any other subsystem. In my opinion, the right fix is "git revert 62ae45687e43" If we continue this, maybe leave a caution in capital letters? diff --git a/include/Kbuild b/include/Kbuild index 5e76a599e2dd..4dff23ae51a4 100644 --- a/include/Kbuild +++ b/include/Kbuild @@ -1 +1,3 @@ +# The drm subsystem is strict about the self-containedness of header files. +# OTHER SUBSYSTEMS SHOULD NOT FOLLOW THIS PRACTICE. obj-$(CONFIG_DRM_HEADER_TEST) += drm/ -- Best Regards Masahiro Yamada
On Wed, Apr 02, 2025 at 03:46:34AM +0900, Masahiro Yamada wrote: > However, it is annoying to make every header self-contained > "just because we are checking this". From my POV itis not "just because we are checking this", I have a very deliberate reason for wanting headers to be self contained: I want the clangd code indexer and editor integration to work fully. When clangd is asked by the editor for a report on a header file it cannot know the missing implicit includes and it's functionality is degraded. It reports fake compilation errors, can't do all the indexing functions, and is generally a bad experience. To be clear the header is parsed and loaded into the database when some C file included it, just the actual editing of the header doesn't work well. This is a very big day-to-day negative for working on the code. I'm frequently annoyed by headers that are pointlessly not self contained. I'd really welcome someone doing a cleanup here. I agree it should not be a hard rule. I was reading the thread you linked to and I'm thinking the approach is not optimal. The only header files that should be checked are ones that are actually used as part of the build with the current kconfig. Christoph is right that there are endless cases where headers should never be parsed outside of specific kconfig settings. So, I'd suggest a better way to run this is first build the kernel, then mine the gcc -MD output (ie stored in the .XX.cmd files) to generate a list of headers that are actually part of the build, then only test those. That eliminates all the kconfig problems. Opt out any special headers that really have a good reason not to be stand alone. Jason
On Wed, 02 Apr 2025, Masahiro Yamada <masahiroy@kernel.org> wrote: > On Wed, Apr 2, 2025 at 1:12 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> On Tue, 1 Apr 2025 at 05:21, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> > >> > The header checks have existed for uapi headers before, including the, >> > uh, turds, but apparently adding them in drm broke the camel's back. >> >> The uapi header test things never caused any problems for me [*], >> because they don't actually pollute the source tree. >> >> Why? Because they all end up in that generated 'usr/include/' directory. >> >> So when I look at the source files, filename completion is entirely >> unaffected, and it all works fine. >> >> Look, I can complete something like >> >> include/uapi/asm-generic/poll.h >> >> perfectly fine, because there is *not* some generated turd that affects it all. >> >> Because for the uapi files those hdrtest files end up being in >> >> ./usr/include/asm-generic/poll.hdrtest >> >> and I never have any reason to really look at that subdirectory at >> all, since it's all generated. >> >> Or put another way - if I _were_ to look at it, it would be exactly >> because I want to see some generated file, in which case the 'hdrtest' >> turd would be part of it. >> >> (Although I cannot recall that ever having actually happened, to be >> honest - but looking at various header files is common, and I hit the >> drm case immediately) >> >> Would you mind taking more of that uapi approach than creating that >> hidden directory specific to the drm tree? Maybe this could *all* be >> generalized? >> >> Linus >> >> [*] I say "never caused any problems for me", but maybe it did way in >> the past and it was fixed and I just don't recall. I have definitely >> complained about pathname completion issues to people before. > > I know you dislike this, and I NACKed this before (but too late): > https://lore.kernel.org/dri-devel/CAK7LNATHXwEkjJHP7b-ZmhzLfyyuOdsyimna-=r-sJk+DxigrA@mail.gmail.com/ > > Compile-testing UAPI headers is useful > (and I believe this is the only case where it is useful) > because userspace is independent of any CONFIG option, > and we have no control over the include order in > userspace programs. > > In contrast, kernel-space headers are conditionally parsed, > depending on CONFIG options. > > You dislike this feature for the reason of TAB-completion, > but I am negative about this feature from another perspective. > > We cannot avoid a false-positive: > https://lore.kernel.org/all/20190718130835.GA28520@lst.de/ > > It is not <drm/*.h> but <linux/*.h> > However, it is annoying to make every header self-contained > "just because we are checking this". As I explained earlier in the thread, it's not *just* about the headers being self-contained. It's *also* about checking header guards and kernel-doc, maybe other things in the future. If it's possible to do opt-in build checks for this, and catch all of these pre-merge, why shouldn't we? We can catch a whole class of build issues and bypass the subsequent fixes with this, and make life easier for us. > Actually, it is not a real problem when the relevant > CONFIG option is disabled. > I did not interrupt him because he was doing this > checkunder drivers/gpu/drm/i915/. > () > But, I am opposed to expanding it to more-global include/drm/, > or any other subsystem. > > In my opinion, the right fix is > "git revert 62ae45687e43" > > > If we continue this, maybe leave a caution > in capital letters? Or maybe let the subsystem and driver maintainers decide? I don't think there's a *single* header under include/drm where them being self-contained depends on a config option. I'm sure there are plenty in include/linux, but I wouldn't say they *all* do. Maybe help and support us in generalizing this in scripts/Makefile.build to avoid the boilerplate, and make Linus happy, instead of trying to block us from having nice things? Please? BR, Jani. > > > diff --git a/include/Kbuild b/include/Kbuild > index 5e76a599e2dd..4dff23ae51a4 100644 > --- a/include/Kbuild > +++ b/include/Kbuild > @@ -1 +1,3 @@ > +# The drm subsystem is strict about the self-containedness of header files. > +# OTHER SUBSYSTEMS SHOULD NOT FOLLOW THIS PRACTICE. > obj-$(CONFIG_DRM_HEADER_TEST) += drm/ > > > > -- > Best Regards > > Masahiro Yamada
On Wed, Apr 2, 2025 at 4:15 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Apr 02, 2025 at 03:46:34AM +0900, Masahiro Yamada wrote: > > However, it is annoying to make every header self-contained > > "just because we are checking this". > > From my POV itis not "just because we are checking this", I have a > very deliberate reason for wanting headers to be self contained: > > I want the clangd code indexer and editor integration to work fully. > > When clangd is asked by the editor for a report on a header file it > cannot know the missing implicit includes and it's functionality is > degraded. It reports fake compilation errors, can't do all the > indexing functions, and is generally a bad experience. To be clear the > header is parsed and loaded into the database when some C file > included it, just the actual editing of the header doesn't work well. > > This is a very big day-to-day negative for working on the code. I'm > frequently annoyed by headers that are pointlessly not self > contained. I'd really welcome someone doing a cleanup here. > > I agree it should not be a hard rule. I was reading the thread you > linked to and I'm thinking the approach is not optimal. > > The only header files that should be checked are ones that are > actually used as part of the build with the current kconfig. Christoph > is right that there are endless cases where headers should never be > parsed outside of specific kconfig settings. > > So, I'd suggest a better way to run this is first build the kernel, > then mine the gcc -MD output (ie stored in the .XX.cmd files) to > generate a list of headers that are actually part of the build, then > only test those. That eliminates all the kconfig problems. Opt out any > special headers that really have a good reason not to be stand alone. Sounds much better.
On Tue, 01 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote: > So, I'd suggest a better way to run this is first build the kernel, > then mine the gcc -MD output (ie stored in the .XX.cmd files) to > generate a list of headers that are actually part of the build, then > only test those. That eliminates all the kconfig problems. Opt out any > special headers that really have a good reason not to be stand alone. I think we'd want the drm headers pass the checks independent of configs (apart from CONFIG_DRM). One size doesn't fit all. BR, Jani.
On Tue, Apr 01, 2025 at 10:42:35PM +0300, Jani Nikula wrote: > On Tue, 01 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote: > > So, I'd suggest a better way to run this is first build the kernel, > > then mine the gcc -MD output (ie stored in the .XX.cmd files) to > > generate a list of headers that are actually part of the build, then > > only test those. That eliminates all the kconfig problems. Opt out any > > special headers that really have a good reason not to be stand alone. > > I think we'd want the drm headers pass the checks independent of configs > (apart from CONFIG_DRM). One size doesn't fit all. Why? That demand is just making it impossible to make shared infrastructure, and I don't think DRM should go off and build its own stuff just for DRM in a way that nobody else can use it. If you really, really, care then you can have your makefile codegen an "allheaders.c" that #includes drm/*.h and compile that. Jason