| Message ID | 20190621163931.19397-1-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: compile-test global headers to ensure they are self-contained | expand |
Hi Masahiro. On Sat, Jun 22, 2019 at 01:39:31AM +0900, Masahiro Yamada wrote: > Make as many headers self-contained as possible so that they can be > included without relying on a specific include order. It is very nice finally to get some infrastructure to validate header files. But to avoid too many conflicts while including more and more headers that are selfcontained we really need something that is more distributed. So for example all header files in include/drm/* could be in one Makefile, incl. sub-directories, but the same Makefile would not include the files in include/soc/ If you just show how ot do it, others can follow-up with the relevant directories. Sam
Hi Sam, On Sat, Jun 22, 2019 at 2:51 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Masahiro. > > On Sat, Jun 22, 2019 at 01:39:31AM +0900, Masahiro Yamada wrote: > > Make as many headers self-contained as possible so that they can be > > included without relying on a specific include order. > It is very nice finally to get some infrastructure to validate header > files. > > But to avoid too many conflicts while including more and more headers > that are selfcontained we really need something that is more > distributed. > So for example all header files in include/drm/* could be in one > Makefile, incl. sub-directories, but the same Makefile would not include > the files in include/soc/ > > If you just show how ot do it, others can follow-up with the > relevant directories. At first, I tried to split Makefile per directory, and add header-test-y one by one. I think you expect they look like this: include/Makefile: subdir-y += drm subdir-y += linux subdir-y += media include/drm/Makefile: header-test-y += drm_cache.h header-test-y += drm_file.h header-test-y += drm_util.h ... include/linux/Makefile: header-test-y += io.h header-test-y += list.h header-test-y += kernel.h header-test-y += types.h ... This is a straightforward way, but I see some disadvantages. Currently, there are more than 4000 headers under include/. So, to cover (almost) all of them, we must list out 4000 entries. When somebody adds a new header, he will be very likely to forget to add header-test-y for it. So, newly added headers will always fall off the coverage. So, I am trying to take an opposite approach. Add all headers in include/ by wildcard, then filter-out one that cannot be self-contained. In my analysis, 70% of headers are already conf-contained. After some fixups, 95% of headers can become self-contained. At this moment, the wildcard only covers some directories or patterns, but my plan is to extend the wildcard gradually. Please feel free to suggest alternative ideas. -- Best Regards Masahiro Yamada
Hi Masahiro > At first, I tried to split Makefile per directory, > and add header-test-y one by one. > > I think you expect they look like this: > > > include/Makefile: > > subdir-y += drm > subdir-y += linux > subdir-y += media So far we agree. > include/drm/Makefile: > > header-test-y += drm_cache.h > header-test-y += drm_file.h > header-test-y += drm_util.h > ... On this level it would be better to do: header-test-y += $(call find_all_header_files) # drm files that are not self-contained header-test-n += drm_legacy.h Likewise for all subdirs below include/ and include/linux should maybe be split up a little too. Maybe include/uapi/ would need to be slipt a little. Then we have a manageable number of Makefiles. New header files are automatically checked and we have a list of files to fix. If for example drm/ have too much failures to a start then we can add that directory to the higler level Makefile later. The above is more or less what you already started, but the difference is that we have it pushed down one or two directories. The header-test-n logic could be moved to the generic part, and a helper could be made to find all header files. Then the Makefiles would be more or less trival, with all the Kbuild magic hidden away. > In my analysis, 70% of headers are already conf-contained. > After some fixups, 95% of headers can become self-contained. Sounds good. And we do not want 100%, but close... Sam
Hi Masahiro > > ... > On this level it would be better to do: > header-test-y += $(call find_all_header_files) > > # drm files that are not self-contained > header-test-n += drm_legacy.h I made a quick hack on top of your patch to demonstrate the ideas. See patch below. When all header files below include/drm are self-contained it will be a single line: header-test-y += $(all_headers_with_subdir) But for now I skipped the subdirs as they was not in too good shape. My gmake foo escapted me tonight, so $(call all_headers_with_subdir) did not really do what I wanted. Which is why I use the macro direct. The main part is that we have support in the generic part to find the header files and to filter out header files we do not want to check. Later we may extend the checking to something more than that they can build. We could check for CONFIG_ symbols in uapi/ and more. Another note. Maybe we should name the files Kbuild, as this is what we usually do in include/* But I also sometimes regret that I introduced this second name. Back then the idea was to globally rename Makefile => Kbuild. But I never advocated this, as the pain was much bigger than the gain. Another thing to be cleaned up one day maybe... Sam diff --git a/include/Makefile b/include/Makefile index 68a76ac732c3..09e854d504f6 100644 --- a/include/Makefile +++ b/include/Makefile @@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only +subdir-y += drm + # extend the test coverage when existing errors are fixed header-test += linux/w*.h diff --git a/include/drm/Makefile b/include/drm/Makefile new file mode 100644 index 000000000000..61a762964ef0 --- /dev/null +++ b/include/drm/Makefile @@ -0,0 +1,21 @@ +# Verify all header files + +header-test-y += $(all_headers) + +# Blacklist header files that do not yet pass the test +# Keep list sorted +header-test-n += ati_pcigart.h +header-test-n += drm_audio_component.h +header-test-n += drm_auth.h +header-test-n += drm_debugfs.h +header-test-n += drm_debugfs_crc.h +header-test-n += drm_displayid.h +header-test-n += drm_fb_cma_helper.h +header-test-n += drm_fixed.h +header-test-n += drm_format_helper.h +header-test-n += drm_lease.h +header-test-n += drm_legacy.h +header-test-n += drm_plane_helper.h +header-test-n += drm_rect.h +header-test-n += i915_component.h +header-test-n += intel-gtt.h diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 0ae07e83d393..df29c65c6490 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -66,6 +66,20 @@ define filechk fi endef +###### +# Support functions for header-test +define all_headers + $(patsubst $(srctree)/$(src)/%,%, \ + $(wildcard $(srctree)/$(src)/*.h)) +endef + +define all_headers_with_subdir + $(patsubst $(srctree)/$(src)/%,%, \ + $(shell find $(srctree)/$(src)/ '*.h')) +endef + + + ###### # gcc support functions # See documentation in Documentation/kbuild/makefiles.txt diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 3e630fcaffd1..e2f765e9d1e1 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -67,6 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) endif # Test self-contained headers +header-test-y := $(filter-out $(header-test-n), $(header-test-y)) extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.hdrtest.o,$(header-test-y)) # Add subdir path
> > When all header files below include/drm are self-contained it will be a > single line: > > header-test-y += $(all_headers_with_subdir) In reality it will likely be the above, and then a list of header-test-n += foo.h For the header files that we for one or the other reason do not want to make self-contained. It would be nice to have the list of ignored files close to their home and not a full list in one Makefile in include/ > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 3e630fcaffd1..e2f765e9d1e1 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -67,6 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) > endif > > # Test self-contained headers > +header-test-y := $(filter-out $(header-test-n), $(header-test-y)) This part should include the logic to filter out duplicates too. I think we may do something wrong if the same header is listed twice. We could also extend this with a check that all files in header-test-n exits. Sam
Hi Sam, On Tue, Jun 25, 2019 at 3:11 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > When all header files below include/drm are self-contained it will be a > > single line: > > > > header-test-y += $(all_headers_with_subdir) > In reality it will likely be the above, and then a list of > > header-test-n += foo.h > > For the header files that we for one or the other reason do not want to > make self-contained. > It would be nice to have the list of ignored files close to their home > and not a full list in one Makefile in include/ > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > > index 3e630fcaffd1..e2f765e9d1e1 100644 > > --- a/scripts/Makefile.lib > > +++ b/scripts/Makefile.lib > > @@ -67,6 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) > > endif > > > > # Test self-contained headers > > +header-test-y := $(filter-out $(header-test-n), $(header-test-y)) > This part should include the logic to filter out duplicates too. > I think we may do something wrong if the same header is listed twice. > > We could also extend this with a check that all files in header-test-n > exits. > > Sam Thanks for your comments. Some followups: [1] I prefer 'header-test-' to 'header-test-n' for excluding headers. In some places, it will be useful to be able to write like this: header-test-$(CONFIG_FOO) += foo.h [2] I proposed somewhat generalized header-test-pattern-y instead of providing both of 'all_headers' and 'all_headers_with_subdir' BTW, "all headers" should be added with care. scripts/Makefile.asm-generic and scripts/Makefile.headersinst cater to removing stale headers. But, we do not explicitly clean other headers. We always be careful about potential matching to stale headers. [3] I tried both 'one big single Makefile' and 'each Makefile in sub-directories' I am slightly in favor of the former. Maybe I could be wrong, and we may switch to the other approach. But, I'd like to start with a single Makefile, and see how bad it is. I sent v2: https://patchwork.kernel.org/project/linux-kbuild/list/?series=138507 -- Best Regards Masahiro Yamada
diff --git a/Makefile b/Makefile index c23f5e8381ad..82c1722dd9e9 100644 --- a/Makefile +++ b/Makefile @@ -610,6 +610,7 @@ drivers-y := drivers/ sound/ drivers-$(CONFIG_SAMPLES) += samples/ net-y := net/ libs-y := lib/ +libs-$(CONFIG_HEADER_TEST) += include/ core-y := usr/ virt-y := virt/ endif # KBUILD_EXTMOD diff --git a/include/Makefile b/include/Makefile new file mode 100644 index 000000000000..68a76ac732c3 --- /dev/null +++ b/include/Makefile @@ -0,0 +1,31 @@ +# SPDX-License-Identifier: GPL-2.0-only + +# extend the test coverage when existing errors are fixed + +header-test += linux/w*.h +header-test += linux/x*.h +header-test += linux/y*.h +header-test += ras/*.h +header-test += soc/at91/*.h +header-test += soc/bcm2835/*.h +header-test += soc/mediatek/*.h +header-test += soc/sa1100/*.h + +all-headers = $(patsubst $(srctree)/include/%,%,\ + $(wildcard $(addprefix $(srctree)/include/, $(header-test)))) + +# Do not include directly +no-header-test += linux/compiler-clang.h +no-header-test += linux/compiler-gcc.h +no-header-test += linux/patchkey.h +no-header-test += linux/rwlock_api_smp.h +no-header-test += linux/spinlock_types_up.h +no-header-test += linux/spinlock_up.h +no-header-test += linux/wimax/debug.h +no-header-test += rdma/uverbs_named_ioctl.h + +# Conditionally included +no-header-test += linux/byteorder/big_endian.h +no-header-test += linux/byteorder/little_endian.h + +header-test-y = $(filter-out $(no-header-test), $(all-headers))
Make as many headers self-contained as possible so that they can be included without relying on a specific include order. This commit compiles only a few headers, but it is a good start point. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Makefile | 1 + include/Makefile | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 include/Makefile -- 2.17.1