[RFC 1/3] kbuild: add support for ensuring headers are self-contained

[RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Jani Nikula]

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 <chris@chris-wilson.co.uk>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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 \"$(<F)\"" > $@
+
+# 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

[RFC 2/3] drm/i915: ensure headers remain self-contained

[Jani Nikula]

Use the new header test facility.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 1787e1299b1b..05d01a3830d0 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -204,3 +204,6 @@ i915-y += intel_lpe_audio.o
 
 obj-$(CONFIG_DRM_I915) += i915.o
 obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o
+
+header-test-y := \
+	i915_drv.h
-- 
2.20.1

[RFC 3/3] DO NOT MERGE: drm/i915: add failing header to header-test-y

[Jani Nikula]

Demonstrate build failure on a header that is not self-contained.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 05d01a3830d0..fcebf453c9ed 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -206,4 +206,5 @@ obj-$(CONFIG_DRM_I915) += i915.o
 obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o
 
 header-test-y := \
+	i915_fixed.h \
 	i915_drv.h
-- 
2.20.1

Re: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Chris Wilson]

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 +=...

And it should also be put into the global gitignore I think.
-Chris

Re: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Masahiro Yamada]

On Fri, May 17, 2019 at 4:48 AM Jani Nikula <jani.nikula@intel.com> 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 <chris@chris-wilson.co.uk>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  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 \"$(<F)\"" > $@
> +
> +# 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

Re: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Masahiro Yamada]

On Fri, May 17, 2019 at 5:35 PM Chris Wilson <chris@chris-wilson.co.uk> 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

Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Jani Nikula]

On Sat, 18 May 2019, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> On Fri, May 17, 2019 at 4:48 AM Jani Nikula <jani.nikula@intel.com> 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 <chris@chris-wilson.co.uk>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Michal Marek <michal.lkml@markovi.net>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  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 \"$(<F)\"" > $@
>> +
>> +# 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: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Sam Ravnborg]

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 <chris@chris-wilson.co.uk>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  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 \"$(<F)\"" > $@
+      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)
 # ---------------------------------------------------------------------------

Re: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Masahiro Yamada]

Hi Sam,


On Sat, May 25, 2019 at 2:40 AM Sam Ravnborg <sam@ravnborg.org> 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 <chris@chris-wilson.co.uk>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: Michal Marek <michal.lkml@markovi.net>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  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 \"$(<F)\"" > $@
> +      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

Re: [Intel-gfx] [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Masahiro Yamada]

Hi Jani,

On Mon, May 20, 2019 at 6:16 PM Jani Nikula <jani.nikula@intel.com> 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: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Sam Ravnborg]

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.

	Sam

Re: [RFC 1/3] kbuild: add support for ensuring headers are self-contained

[Masahiro Yamada]

Hi Sam, Jani,

On Tue, Jun 4, 2019 at 2:33 AM Sam Ravnborg <sam@ravnborg.org> 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