Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
Hi Sam, Jani, On Tue, Jun 4, 2019 at 2:33 AM Sam Ravnborg wrote: > > Hi Masahiro/Jani. > > > > > Following the obj-y pattern, > > I want to make header-test-y relative to $(obj). > > I also considered this and agree this is better. > > Otherwise we end up with a spaghetti of dependencies across the tree. > > What I made just fit the purpose I had that day, > which is no excuse for bad design. > > > I prefer this: > > > > quiet_cmd_header_test = HDRTEST $@ > > cmd_header_test = echo "\#include \"$*.h\"" > $@ > > > > $(obj)/%.header_test.c: > > $(call cmd,header_test) > > Even better - good. > > We call it HDRTEST - so why not just go for that name: > > hdrtest-y += headerfile.h > > ?? > > The current proposal with "header-test-y" hurts the eye a little with > two '-', and all other variables uses only one '-' as is today. > (generic-y, obj-y etc). > > This is bikeshedding but is was itcing me a little. I do not have a strong opinion. I leave it to Jani. Either is fine with me. There are variables that contain two '-'. 'no-clean-files', 'subdir-ccflags-y', etc. -- Best Regards Masahiro Yamada ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
Hi Jani, On Mon, May 20, 2019 at 6:16 PM Jani Nikula wrote: > > > > > I will take a little time to considier > > how far we can extend the idea about > > "headers should be self-contained". > > Thanks! Please let me know if/when you need further action from me, I > won't post new versions until then. Could you send v2 with the following changes ? [1] Could you rename *.header_test.c to *.hdrtest.c ? (I just thought .header_test.c was a bit too long.) [2] %.hdrtest.c should not depend on the header This will avoid unnecessary regeneration of *.hdrtest.c quiet_cmd_header_test = HDRTEST $@ cmd_header_test = echo "\#include \"$*.h\"" > $@ $(obj)/%.hdrtest.c: $(call cmd,header_test) [3] Please add '*.hdrtest.c' to .gitignore, Documentation/dontdiff [4] Please add '*.hdrtest.c' to 'make clean' (around line 1640 of top Makefile) -- Best Regards Masahiro Yamada
Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
Hi Sam, On Sat, May 25, 2019 at 2:40 AM Sam Ravnborg wrote: > > Hi Jani > > > Sometimes it's useful to be able to explicitly ensure certain headers > > remain self-contained, i.e. that they are compilable as standalone > > units, by including and/or forward declaring everything they depend on. > > > > Add special target header-test-y where individual Makefiles can add > > headers to be tested if CONFIG_HEADER_TEST is enabled. This will > > generate a dummy C file per header that gets built as part of extra-y. > > Very useful, thanks. > I have cooked up something ad-hoc a couple of times but having it as a > standard feature in the build system is much better. > The we can let some of our infrastructure pick up an issues > automatically. > > > > > Cc: Chris Wilson > > Cc: Masahiro Yamada > > Cc: Michal Marek > > Signed-off-by: Jani Nikula > > --- > > Documentation/kbuild/makefiles.txt | 7 +++ > > init/Kconfig | 9 + > > scripts/Makefile.build | 10 ++ > > scripts/Makefile.lib | 3 +++ > > 4 files changed, 29 insertions(+) > > > > diff --git a/Documentation/kbuild/makefiles.txt > > b/Documentation/kbuild/makefiles.txt > > index 03c065855eaf..73df58e5ea0c 100644 > > --- a/Documentation/kbuild/makefiles.txt > > +++ b/Documentation/kbuild/makefiles.txt > > @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are > > followed (roughly): > > In this example, extra-y is used to list object files that > > shall be built, but shall not be linked as part of built-in.a. > > > > +header-test-y > > + > > + header-test-y specifies headers (*.h) in the current directory that > > + should be compile tested to ensure they are self-contained, > > + i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled, > > + this autogenerates dummy sources to include the headers, and builds > > them > > + as part of extra-y. > Do we want to restrict this to current directory only? > Sometimes we could use this for headers in include/ but let it > trigger for the relevant subsystem. > So for example drivers/gpu/drm/Makefile will include the rules > for all headers in include/drm/* > > The alternative would be Makefiles (of Kbuild files) > scattered in the directories with headers and then some > infrastructure to visit those. > > Follow patch extend the header-test feature to work with > headers in include/ Following the obj-y pattern, I want to make header-test-y relative to $(obj). > Example: > # Header files from this directory > header-test-y += drm_crtc_helper_internal.h > header-test-y += drm_crtc_internal.h These are described in drivers/gpu/drm/Makefile. > .. > . > # Header files from include/drm > header-test-y += drm/amd_asic_type.h > header-test-y += drm/ati_pcigart.h These are described in $(srctree)/include/Makefile. > ... > > > In the patch $* is used to get the "stem" from the pattern. > This is the filname of the header file without extension. > > > Sam > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 4d4bf698467a..ca132ab3a551 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -295,11 +295,10 @@ $(obj)/%.lst: $(src)/%.c FORCE > # --- > > quiet_cmd_header_test = HDRTEST $@ > - cmd_header_test = echo "\#include \"$( $@ > + cmd_header_test = echo "\#include <$(2).h>" > $@ > > -# FIXME: would be nice to be able to limit this implicit rule to > header-test-y > -$(obj)/%.header_test.c: $(src)/%.h FORCE > - $(call if_changed,header_test) > +$(obj)/%.header_test.c: > + $(call cmd,header_test,$*) > > # Compile assembler sources (.S) > # --- > Agree, this is much better, and it is what scripts/Makefile.asm-generic does. But, you do not need to pass '$*' via the argument. I prefer this: quiet_cmd_header_test = HDRTEST $@ cmd_header_test = echo "\#include \"$*.h\"" > $@ $(obj)/%.header_test.c: $(call cmd,header_test) -- Best Regards Masahiro Yamada ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
Hi Jani > Sometimes it's useful to be able to explicitly ensure certain headers > remain self-contained, i.e. that they are compilable as standalone > units, by including and/or forward declaring everything they depend on. > > Add special target header-test-y where individual Makefiles can add > headers to be tested if CONFIG_HEADER_TEST is enabled. This will > generate a dummy C file per header that gets built as part of extra-y. Very useful, thanks. I have cooked up something ad-hoc a couple of times but having it as a standard feature in the build system is much better. The we can let some of our infrastructure pick up an issues automatically. > > Cc: Chris Wilson > Cc: Masahiro Yamada > Cc: Michal Marek > Signed-off-by: Jani Nikula > --- > Documentation/kbuild/makefiles.txt | 7 +++ > init/Kconfig | 9 + > scripts/Makefile.build | 10 ++ > scripts/Makefile.lib | 3 +++ > 4 files changed, 29 insertions(+) > > diff --git a/Documentation/kbuild/makefiles.txt > b/Documentation/kbuild/makefiles.txt > index 03c065855eaf..73df58e5ea0c 100644 > --- a/Documentation/kbuild/makefiles.txt > +++ b/Documentation/kbuild/makefiles.txt > @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are followed > (roughly): > In this example, extra-y is used to list object files that > shall be built, but shall not be linked as part of built-in.a. > > +header-test-y > + > + header-test-y specifies headers (*.h) in the current directory that > + should be compile tested to ensure they are self-contained, > + i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled, > + this autogenerates dummy sources to include the headers, and builds them > + as part of extra-y. Do we want to restrict this to current directory only? Sometimes we could use this for headers in include/ but let it trigger for the relevant subsystem. So for example drivers/gpu/drm/Makefile will include the rules for all headers in include/drm/* The alternative would be Makefiles (of Kbuild files) scattered in the directories with headers and then some infrastructure to visit those. Follow patch extend the header-test feature to work with headers in include/ Example: # Header files from this directory header-test-y += drm_crtc_helper_internal.h header-test-y += drm_crtc_internal.h .. . # Header files from include/drm header-test-y += drm/amd_asic_type.h header-test-y += drm/ati_pcigart.h ... In the patch $* is used to get the "stem" from the pattern. This is the filname of the header file without extension. Sam diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 4d4bf698467a..ca132ab3a551 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -295,11 +295,10 @@ $(obj)/%.lst: $(src)/%.c FORCE # --- quiet_cmd_header_test = HDRTEST $@ - cmd_header_test = echo "\#include \"$( $@ + cmd_header_test = echo "\#include <$(2).h>" > $@ -# FIXME: would be nice to be able to limit this implicit rule to header-test-y -$(obj)/%.header_test.c: $(src)/%.h FORCE - $(call if_changed,header_test) +$(obj)/%.header_test.c: + $(call cmd,header_test,$*) # Compile assembler sources (.S) # --- ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
On Sat, 18 May 2019, Masahiro Yamada wrote: > On Fri, May 17, 2019 at 4:48 AM Jani Nikula wrote: >> >> Sometimes it's useful to be able to explicitly ensure certain headers >> remain self-contained, i.e. that they are compilable as standalone >> units, by including and/or forward declaring everything they depend on. >> >> Add special target header-test-y where individual Makefiles can add >> headers to be tested if CONFIG_HEADER_TEST is enabled. This will >> generate a dummy C file per header that gets built as part of extra-y. >> >> Cc: Chris Wilson >> Cc: Masahiro Yamada >> Cc: Michal Marek >> Signed-off-by: Jani Nikula >> --- >> Documentation/kbuild/makefiles.txt | 7 +++ >> init/Kconfig | 9 + >> scripts/Makefile.build | 10 ++ >> scripts/Makefile.lib | 3 +++ >> 4 files changed, 29 insertions(+) >> >> diff --git a/Documentation/kbuild/makefiles.txt >> b/Documentation/kbuild/makefiles.txt >> index 03c065855eaf..73df58e5ea0c 100644 >> --- a/Documentation/kbuild/makefiles.txt >> +++ b/Documentation/kbuild/makefiles.txt >> @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are >> followed (roughly): >> In this example, extra-y is used to list object files that >> shall be built, but shall not be linked as part of built-in.a. >> >> +header-test-y >> + >> + header-test-y specifies headers (*.h) in the current directory that >> + should be compile tested to ensure they are self-contained, >> + i.e. compilable as standalone units. If CONFIG_HEADER_TEST is >> enabled, >> + this autogenerates dummy sources to include the headers, and builds >> them >> + as part of extra-y. >> >> --- 6.7 Commands useful for building a boot image >> >> diff --git a/init/Kconfig b/init/Kconfig >> index 4592bf7997c0..d91b157201b1 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -95,6 +95,15 @@ config COMPILE_TEST >> here. If you are a user/distributor, say N here to exclude useless >> drivers to be distributed. >> >> +config HEADER_TEST >> + bool "Compile test headers that should be standalone compilable" >> + help >> + Compile test headers listed in header-test-y target to ensure they >> are >> + self-contained, i.e. compilable as standalone units. >> + >> + If you are a developer or tester and want to ensure the requested >> + headers are self-contained, say Y here. Otherwise, choose N. >> + >> config LOCALVERSION >> string "Local version - append to kernel release" >> help >> diff --git a/scripts/Makefile.build b/scripts/Makefile.build >> index 76ca30cc4791..4d4bf698467a 100644 >> --- a/scripts/Makefile.build >> +++ b/scripts/Makefile.build >> @@ -291,6 +291,16 @@ quiet_cmd_cc_lst_c = MKLST $@ >> $(obj)/%.lst: $(src)/%.c FORCE >> $(call if_changed_dep,cc_lst_c) >> >> +# Dummy C sources for header test (header-test-y target) >> +# >> --- >> + >> +quiet_cmd_header_test = HDRTEST $@ >> + cmd_header_test = echo "\#include \"$( $@ >> + >> +# FIXME: would be nice to be able to limit this implicit rule to >> header-test-y >> +$(obj)/%.header_test.c: $(src)/%.h FORCE >> + $(call if_changed,header_test) >> + >> # Compile assembler sources (.S) >> # >> --- >> >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib >> index 8a1f64f17740..c2839de06485 100644 >> --- a/scripts/Makefile.lib >> +++ b/scripts/Makefile.lib >> @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) >> extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) >> endif >> >> +# Test self-contained headers >> +extra-$(CONFIG_HEADER_TEST) += $(patsubst >> %.h,%.header_test.o,$(header-test-y)) >> + >> # Add subdir path >> >> extra-y:= $(addprefix $(obj)/,$(extra-y)) >> -- >> 2.20.1 >> > > > Thanks, probably we should do this. > > At least, this check will be useful > for uapi headers since the kernel does not > test the self-containedness of > exported headers, (then turned out be problematic > later in user-space). > > I will take a little time to considier > how far we can extend the idea about > "headers should be self-contained". Thanks! Please let me know if/when you need further action from me, I won't post new versions until then. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
On Fri, May 17, 2019 at 5:35 PM Chris Wilson wrote: > > Quoting Jani Nikula (2019-05-16 20:48:16) > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > > index 8a1f64f17740..c2839de06485 100644 > > --- a/scripts/Makefile.lib > > +++ b/scripts/Makefile.lib > > @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) > > extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) > > endif > > > > +# Test self-contained headers > > +extra-$(CONFIG_HEADER_TEST) += $(patsubst > > %.h,%.header_test.o,$(header-test-y)) > > This didn't get pulled into clean-files. > > clean-files-$(CONFIG_HEADER_TEST) += $(patsubst > %.h,%.header_test.c,$(header-test-y)) > ? > > Not enough. Nor is clean-files-y +=... The correct syntax is 'clean-files +=' instead of 'clean-files-y +=' If Kbuild supports this in the core Makefiles, we can add "-o -name '*.header_test.c'" to the top Makefile. > And it should also be put into the global gitignore I think. Documentation/dontdiff too. > -Chris -- Best Regards Masahiro Yamada ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
On Fri, May 17, 2019 at 4:48 AM Jani Nikula wrote: > > Sometimes it's useful to be able to explicitly ensure certain headers > remain self-contained, i.e. that they are compilable as standalone > units, by including and/or forward declaring everything they depend on. > > Add special target header-test-y where individual Makefiles can add > headers to be tested if CONFIG_HEADER_TEST is enabled. This will > generate a dummy C file per header that gets built as part of extra-y. > > Cc: Chris Wilson > Cc: Masahiro Yamada > Cc: Michal Marek > Signed-off-by: Jani Nikula > --- > Documentation/kbuild/makefiles.txt | 7 +++ > init/Kconfig | 9 + > scripts/Makefile.build | 10 ++ > scripts/Makefile.lib | 3 +++ > 4 files changed, 29 insertions(+) > > diff --git a/Documentation/kbuild/makefiles.txt > b/Documentation/kbuild/makefiles.txt > index 03c065855eaf..73df58e5ea0c 100644 > --- a/Documentation/kbuild/makefiles.txt > +++ b/Documentation/kbuild/makefiles.txt > @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are followed > (roughly): > In this example, extra-y is used to list object files that > shall be built, but shall not be linked as part of built-in.a. > > +header-test-y > + > + header-test-y specifies headers (*.h) in the current directory that > + should be compile tested to ensure they are self-contained, > + i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled, > + this autogenerates dummy sources to include the headers, and builds > them > + as part of extra-y. > > --- 6.7 Commands useful for building a boot image > > diff --git a/init/Kconfig b/init/Kconfig > index 4592bf7997c0..d91b157201b1 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -95,6 +95,15 @@ config COMPILE_TEST > here. If you are a user/distributor, say N here to exclude useless > drivers to be distributed. > > +config HEADER_TEST > + bool "Compile test headers that should be standalone compilable" > + help > + Compile test headers listed in header-test-y target to ensure they > are > + self-contained, i.e. compilable as standalone units. > + > + If you are a developer or tester and want to ensure the requested > + headers are self-contained, say Y here. Otherwise, choose N. > + > config LOCALVERSION > string "Local version - append to kernel release" > help > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 76ca30cc4791..4d4bf698467a 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -291,6 +291,16 @@ quiet_cmd_cc_lst_c = MKLST $@ > $(obj)/%.lst: $(src)/%.c FORCE > $(call if_changed_dep,cc_lst_c) > > +# Dummy C sources for header test (header-test-y target) > +# --- > + > +quiet_cmd_header_test = HDRTEST $@ > + cmd_header_test = echo "\#include \"$( $@ > + > +# FIXME: would be nice to be able to limit this implicit rule to > header-test-y > +$(obj)/%.header_test.c: $(src)/%.h FORCE > + $(call if_changed,header_test) > + > # Compile assembler sources (.S) > # --- > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 8a1f64f17740..c2839de06485 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) > extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) > endif > > +# Test self-contained headers > +extra-$(CONFIG_HEADER_TEST) += $(patsubst > %.h,%.header_test.o,$(header-test-y)) > + > # Add subdir path > > extra-y:= $(addprefix $(obj)/,$(extra-y)) > -- > 2.20.1 > Thanks, probably we should do this. At least, this check will be useful for uapi headers since the kernel does not test the self-containedness of exported headers, (then turned out be problematic later in user-space). I will take a little time to considier how far we can extend the idea about "headers should be self-contained". Thank you. -- Best Regards Masahiro Yamada ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained
Sometimes it's useful to be able to explicitly ensure certain headers remain self-contained, i.e. that they are compilable as standalone units, by including and/or forward declaring everything they depend on. Add special target header-test-y where individual Makefiles can add headers to be tested if CONFIG_HEADER_TEST is enabled. This will generate a dummy C file per header that gets built as part of extra-y. Cc: Chris Wilson Cc: Masahiro Yamada Cc: Michal Marek Signed-off-by: Jani Nikula --- Documentation/kbuild/makefiles.txt | 7 +++ init/Kconfig | 9 + scripts/Makefile.build | 10 ++ scripts/Makefile.lib | 3 +++ 4 files changed, 29 insertions(+) diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt index 03c065855eaf..73df58e5ea0c 100644 --- a/Documentation/kbuild/makefiles.txt +++ b/Documentation/kbuild/makefiles.txt @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are followed (roughly): In this example, extra-y is used to list object files that shall be built, but shall not be linked as part of built-in.a. +header-test-y + + header-test-y specifies headers (*.h) in the current directory that + should be compile tested to ensure they are self-contained, + i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled, + this autogenerates dummy sources to include the headers, and builds them + as part of extra-y. --- 6.7 Commands useful for building a boot image diff --git a/init/Kconfig b/init/Kconfig index 4592bf7997c0..d91b157201b1 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -95,6 +95,15 @@ config COMPILE_TEST here. If you are a user/distributor, say N here to exclude useless drivers to be distributed. +config HEADER_TEST + bool "Compile test headers that should be standalone compilable" + help + Compile test headers listed in header-test-y target to ensure they are + self-contained, i.e. compilable as standalone units. + + If you are a developer or tester and want to ensure the requested + headers are self-contained, say Y here. Otherwise, choose N. + config LOCALVERSION string "Local version - append to kernel release" help diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 76ca30cc4791..4d4bf698467a 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -291,6 +291,16 @@ quiet_cmd_cc_lst_c = MKLST $@ $(obj)/%.lst: $(src)/%.c FORCE $(call if_changed_dep,cc_lst_c) +# Dummy C sources for header test (header-test-y target) +# --- + +quiet_cmd_header_test = HDRTEST $@ + cmd_header_test = echo "\#include \"$( $@ + +# FIXME: would be nice to be able to limit this implicit rule to header-test-y +$(obj)/%.header_test.c: $(src)/%.h FORCE + $(call if_changed,header_test) + # Compile assembler sources (.S) # --- diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 8a1f64f17740..c2839de06485 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) endif +# Test self-contained headers +extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.o,$(header-test-y)) + # Add subdir path extra-y:= $(addprefix $(obj)/,$(extra-y)) -- 2.20.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
