2019-05-16 23:26:03

by Jani Nikula

[permalink] [raw]
Subject: [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 <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Michal Marek <[email protected]>
Signed-off-by: Jani Nikula <[email protected]>
---
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


2019-05-17 09:55:37

by Chris Wilson

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

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

2019-05-18 05:25:44

by Masahiro Yamada

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

On Fri, May 17, 2019 at 4:48 AM Jani Nikula <[email protected]> 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 <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Michal Marek <[email protected]>
> Signed-off-by: Jani Nikula <[email protected]>
> ---
> 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

2019-05-18 05:55:04

by Masahiro Yamada

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

On Fri, May 17, 2019 at 5:35 PM Chris Wilson <[email protected]> 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

2019-05-20 13:31:13

by Jani Nikula

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

On Sat, 18 May 2019, Masahiro Yamada <[email protected]> wrote:
> On Fri, May 17, 2019 at 4:48 AM Jani Nikula <[email protected]> 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 <[email protected]>
>> Cc: Masahiro Yamada <[email protected]>
>> Cc: Michal Marek <[email protected]>
>> Signed-off-by: Jani Nikula <[email protected]>
>> ---
>> 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

2019-05-24 17:41:44

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [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 <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Michal Marek <[email protected]>
> Signed-off-by: Jani Nikula <[email protected]>
> ---
> 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)
# ---------------------------------------------------------------------------

2019-06-03 17:54:40

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [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 <[email protected]> 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 <[email protected]>
> > Cc: Masahiro Yamada <[email protected]>
> > Cc: Michal Marek <[email protected]>
> > Signed-off-by: Jani Nikula <[email protected]>
> > ---
> > 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

2019-06-03 17:55:57

by Masahiro Yamada

[permalink] [raw]
Subject: 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 <[email protected]> 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

2019-06-03 18:03:37

by Sam Ravnborg

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

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

2019-06-04 00:23:55

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [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 <[email protected]> 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