kbuild: add support for ensuring headers are self-contained

Quoting Jani Nikula (2019-05-16 20:48:16)

scripts/Makefile.lib extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) · line 70

Quoted 10 lines

scripts/Makefile.lib +2 -0

index 8a1f64f17740..c2839de06485 100644
@@ -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

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(+)

Documentation/kbuild/makefiles.txt +7 -0

index 03c065855eaf..73df58e5ea0c 100644
@@ -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
 

init/Kconfig +9 -0

index 4592bf7997c0..d91b157201b1 100644
@@ -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

scripts/Makefile.build +10 -0

index 76ca30cc4791..4d4bf698467a 100644
@@ -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)
 # ---------------------------------------------------------------------------
 

scripts/Makefile.lib +3 -1

index 8a1f64f17740..c2839de06485 100644
@@ -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

Quoted 94 lines · 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 <[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(+)

Documentation/kbuild/makefiles.txt +7 -0

index 03c065855eaf..73df58e5ea0c 100644
@@ -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
 

init/Kconfig +9 -0

index 4592bf7997c0..d91b157201b1 100644
@@ -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

scripts/Makefile.build +10 -0

index 76ca30cc4791..4d4bf698467a 100644
@@ -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)
 # ---------------------------------------------------------------------------
 

scripts/Makefile.lib +3 -1

index 8a1f64f17740..c2839de06485 100644
@@ -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))
--

Quoted 2 lines

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".

Quoted 5 lines

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.

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(+)

Documentation/kbuild/makefiles.txt +0 -0

index 03c065855eaf..73df58e5ea0c 100644

Hi Jani

Quoted 7 lines

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.

Quoted 17 lines

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(+)

Documentation/kbuild/makefiles.txt +0 -0

index 03c065855eaf..73df58e5ea0c 100644

Documentation/kbuild/makefiles.txt When kbuild executes, the following steps are followed (roughly): · line 1045

Quoted 10 lines

Documentation/kbuild/makefiles.txt +7 -0

@@ -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

scripts/Makefile.build +0 -1

index 4d4bf698467a..ca132ab3a551 100644
@@ -295,11 +295,10 @@ $(obj)/%.lst: $(src)/%.c FORCE
 # ---------------------------------------------------------------------------
 
 quiet_cmd_header_test = HDRTEST $@
-      cmd_header_test = echo "\#include \"$(<F)\"" > $@

Following the obj-y pattern,
I want to make header-test-y relative to $(obj).

I prefer this:

quiet_cmd_header_test = HDRTEST $@

      cmd_header_test = echo "\#include \"$*.h\"" > $@

$(obj)/%.header_test.c:

        $(call cmd,header_test)

Hi Masahiro/Jani.

Quoted 3 lines

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.

Quoted 7 lines

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.