[v6,6/8] buildtools/chkincs: add app to verify header includes
Checks
Commit Message
To verify that all DPDK headers are ok for inclusion directly in a C file,
and are not missing any other pre-requisite headers, we can auto-generate
for each header an empty C file that includes that header. Compiling these
files will throw errors if any header has unmet dependencies.
To ensure ongoing compliance, we enable this build test as part of the
default x86 build in "test-meson-builds.sh".
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
MAINTAINERS | 4 +++
buildtools/chkincs/gen_c_file_for_header.py | 12 +++++++
buildtools/chkincs/main.c | 4 +++
buildtools/chkincs/meson.build | 40 +++++++++++++++++++++
devtools/test-meson-builds.sh | 2 +-
doc/guides/rel_notes/release_21_02.rst | 8 +++++
meson.build | 5 +++
7 files changed, 74 insertions(+), 1 deletion(-)
create mode 100755 buildtools/chkincs/gen_c_file_for_header.py
create mode 100644 buildtools/chkincs/main.c
create mode 100644 buildtools/chkincs/meson.build
Comments
On Wed, Jan 27, 2021 at 6:37 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> To verify that all DPDK headers are ok for inclusion directly in a C file,
> and are not missing any other pre-requisite headers, we can auto-generate
> for each header an empty C file that includes that header. Compiling these
> files will throw errors if any header has unmet dependencies.
>
> To ensure ongoing compliance, we enable this build test as part of the
> default x86 build in "test-meson-builds.sh".
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> MAINTAINERS | 4 +++
> buildtools/chkincs/gen_c_file_for_header.py | 12 +++++++
> buildtools/chkincs/main.c | 4 +++
> buildtools/chkincs/meson.build | 40 +++++++++++++++++++++
> devtools/test-meson-builds.sh | 2 +-
> doc/guides/rel_notes/release_21_02.rst | 8 +++++
> meson.build | 5 +++
> 7 files changed, 74 insertions(+), 1 deletion(-)
> create mode 100755 buildtools/chkincs/gen_c_file_for_header.py
> create mode 100644 buildtools/chkincs/main.c
> create mode 100644 buildtools/chkincs/meson.build
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1a12916f56..d4f9ebe46d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1562,6 +1562,10 @@ F: app/test/test_resource.c
> F: app/test/virtual_pmd.c
> F: app/test/virtual_pmd.h
>
> +Header build sanity checking
> +M: Bruce Richardson <bruce.richardson@intel.com>
> +F: buildtools/chkincs/
> +
This can be squashed in the generic "Build System" block.
> Sample packet helper functions for unit test
> M: Reshma Pattan <reshma.pattan@intel.com>
> F: app/test/sample_packet_forward.c
> diff --git a/buildtools/chkincs/gen_c_file_for_header.py b/buildtools/chkincs/gen_c_file_for_header.py
> new file mode 100755
> index 0000000000..ed46948aea
> --- /dev/null
> +++ b/buildtools/chkincs/gen_c_file_for_header.py
> @@ -0,0 +1,12 @@
> +#! /usr/bin/env python3
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2021 Intel Corporation
> +
> +from sys import argv
> +from os.path import abspath
> +
> +(h_file, c_file) = argv[1:]
> +
> +contents = '#include "' + abspath(h_file) + '"'
> +with open(c_file, 'w') as cf:
> + cf.write(contents)
> diff --git a/buildtools/chkincs/main.c b/buildtools/chkincs/main.c
> new file mode 100644
> index 0000000000..d25bb8852a
> --- /dev/null
> +++ b/buildtools/chkincs/main.c
> @@ -0,0 +1,4 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Intel Corporation
> + */
> +int main(void) { return 0; }
> diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
> new file mode 100644
> index 0000000000..5dc43283e0
> --- /dev/null
> +++ b/buildtools/chkincs/meson.build
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2021 Intel Corporation
> +
> +if not get_option('check_includes')
> + build = false
> + subdir_done()
> +endif
> +
> +if is_windows
> + # for windows, the shebang line in the script won't work.
> + error('option "check_includes" is not supported on windows')
> +endif
> +
> +gen_c_file_for_header = find_program('gen_c_file_for_header.py')
> +gen_c_files = generator(gen_c_file_for_header,
> + output: '@BASENAME@.c',
> + arguments: ['@INPUT@', '@OUTPUT@'])
> +
> +cflags = machine_args
> +cflags += '-Wno-unused-function' # needed if we include generic headers
> +cflags += '-DALLOW_EXPERIMENTAL_API'
> +
> +# some ethdev headers depend on bus headers
> +includes = include_directories('../../drivers/bus/pci',
> + '../../drivers/bus/vdev')
ethdev headers are fine now, afaics.
So this comment can be changed to a more vague "some driver headers
depend on bus headers".
> +
> +sources = files('main.c')
> +sources += gen_c_files.process(dpdk_chkinc_headers)
> +
> +deps = []
> +foreach l:enabled_libs
> + deps += get_variable('static_rte_' + l)
> +endforeach
> +
> +executable('chkincs', sources,
> + c_args: cflags,
> + include_directories: includes,
> + dependencies: deps,
> + link_whole: dpdk_static_libraries + dpdk_drivers,
> + install: false)
> diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
> index efa91e0e40..07b5e6aeca 100755
> --- a/devtools/test-meson-builds.sh
> +++ b/devtools/test-meson-builds.sh
> @@ -227,7 +227,7 @@ default_machine='nehalem'
> if ! check_cc_flags "-march=$default_machine" ; then
> default_machine='corei7'
> fi
> -build build-x86-default cc skipABI \
> +build build-x86-default cc skipABI -Dcheck_includes=true\
Space before \ to be consistent with the rest of the file.
> -Dlibdir=lib -Dmachine=$default_machine $use_shared
>
> # 32-bit with default compiler
> diff --git a/doc/guides/rel_notes/release_21_02.rst b/doc/guides/rel_notes/release_21_02.rst
> index 33bac4fff8..245d1a6473 100644
> --- a/doc/guides/rel_notes/release_21_02.rst
> +++ b/doc/guides/rel_notes/release_21_02.rst
> @@ -122,6 +122,14 @@ New Features
> * Added support for aes-cbc sha256-128-hmac cipher combination in OCTEON TX2
> crypto PMD lookaside protocol offload for IPsec.
>
> +* **Added support for build-time checking of header includes**
> +
> + A new build option ``check_includes`` has been added, which, when enabled,
> + will perform build-time checking on DPDK public header files, to ensure none
> + are missing dependent header includes. This feature, disabled by default, is
> + intended for use by developers contributing to the DPDK SDK itself, and is
> + integrated into the build scripts and automated CI for patch contributions.
> +
>
Should be earlier in the new features list.
> Removed Items
> -------------
> diff --git a/meson.build b/meson.build
> index e6e34d0a98..fcc4d4c900 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -68,6 +68,11 @@ if get_option('enable_kmods')
> subdir('kernel')
> endif
>
> +# check header includes if requested
> +if get_option('check_includes')
> + subdir('buildtools/chkincs')
> +endif
> +
> # write the build config
> build_cfg = 'rte_build_config.h'
> configure_file(output: build_cfg,
> --
> 2.27.0
>
On Thu, Jan 28, 2021 at 12:02:35PM +0100, David Marchand wrote:
> On Wed, Jan 27, 2021 at 6:37 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > To verify that all DPDK headers are ok for inclusion directly in a C file,
> > and are not missing any other pre-requisite headers, we can auto-generate
> > for each header an empty C file that includes that header. Compiling these
> > files will throw errors if any header has unmet dependencies.
> >
> > To ensure ongoing compliance, we enable this build test as part of the
> > default x86 build in "test-meson-builds.sh".
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > MAINTAINERS | 4 +++
> > buildtools/chkincs/gen_c_file_for_header.py | 12 +++++++
> > buildtools/chkincs/main.c | 4 +++
> > buildtools/chkincs/meson.build | 40 +++++++++++++++++++++
> > devtools/test-meson-builds.sh | 2 +-
> > doc/guides/rel_notes/release_21_02.rst | 8 +++++
> > meson.build | 5 +++
> > 7 files changed, 74 insertions(+), 1 deletion(-)
> > create mode 100755 buildtools/chkincs/gen_c_file_for_header.py
> > create mode 100644 buildtools/chkincs/main.c
> > create mode 100644 buildtools/chkincs/meson.build
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1a12916f56..d4f9ebe46d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1562,6 +1562,10 @@ F: app/test/test_resource.c
> > F: app/test/virtual_pmd.c
> > F: app/test/virtual_pmd.h
> >
> > +Header build sanity checking
> > +M: Bruce Richardson <bruce.richardson@intel.com>
> > +F: buildtools/chkincs/
> > +
>
> This can be squashed in the generic "Build System" block.
>
Ok.
>
> > Sample packet helper functions for unit test
> > M: Reshma Pattan <reshma.pattan@intel.com>
> > F: app/test/sample_packet_forward.c
> > diff --git a/buildtools/chkincs/gen_c_file_for_header.py b/buildtools/chkincs/gen_c_file_for_header.py
> > new file mode 100755
> > index 0000000000..ed46948aea
> > --- /dev/null
> > +++ b/buildtools/chkincs/gen_c_file_for_header.py
> > @@ -0,0 +1,12 @@
> > +#! /usr/bin/env python3
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2021 Intel Corporation
> > +
> > +from sys import argv
> > +from os.path import abspath
> > +
> > +(h_file, c_file) = argv[1:]
> > +
> > +contents = '#include "' + abspath(h_file) + '"'
> > +with open(c_file, 'w') as cf:
> > + cf.write(contents)
> > diff --git a/buildtools/chkincs/main.c b/buildtools/chkincs/main.c
> > new file mode 100644
> > index 0000000000..d25bb8852a
> > --- /dev/null
> > +++ b/buildtools/chkincs/main.c
> > @@ -0,0 +1,4 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2021 Intel Corporation
> > + */
> > +int main(void) { return 0; }
> > diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
> > new file mode 100644
> > index 0000000000..5dc43283e0
> > --- /dev/null
> > +++ b/buildtools/chkincs/meson.build
> > @@ -0,0 +1,40 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2021 Intel Corporation
> > +
> > +if not get_option('check_includes')
> > + build = false
> > + subdir_done()
> > +endif
> > +
> > +if is_windows
> > + # for windows, the shebang line in the script won't work.
> > + error('option "check_includes" is not supported on windows')
> > +endif
> > +
> > +gen_c_file_for_header = find_program('gen_c_file_for_header.py')
> > +gen_c_files = generator(gen_c_file_for_header,
> > + output: '@BASENAME@.c',
> > + arguments: ['@INPUT@', '@OUTPUT@'])
> > +
> > +cflags = machine_args
> > +cflags += '-Wno-unused-function' # needed if we include generic headers
> > +cflags += '-DALLOW_EXPERIMENTAL_API'
> > +
> > +# some ethdev headers depend on bus headers
> > +includes = include_directories('../../drivers/bus/pci',
> > + '../../drivers/bus/vdev')
>
> ethdev headers are fine now, afaics.
> So this comment can be changed to a more vague "some driver headers
> depend on bus headers".
>
Driver headers are not checked yet by the code, only lib ones, so I will
see if this block can be omitted until needed.
>
> > +
> > +sources = files('main.c')
> > +sources += gen_c_files.process(dpdk_chkinc_headers)
> > +
> > +deps = []
> > +foreach l:enabled_libs
> > + deps += get_variable('static_rte_' + l)
> > +endforeach
> > +
> > +executable('chkincs', sources,
> > + c_args: cflags,
> > + include_directories: includes,
> > + dependencies: deps,
> > + link_whole: dpdk_static_libraries + dpdk_drivers,
> > + install: false)
> > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
> > index efa91e0e40..07b5e6aeca 100755
> > --- a/devtools/test-meson-builds.sh
> > +++ b/devtools/test-meson-builds.sh
> > @@ -227,7 +227,7 @@ default_machine='nehalem'
> > if ! check_cc_flags "-march=$default_machine" ; then
> > default_machine='corei7'
> > fi
> > -build build-x86-default cc skipABI \
> > +build build-x86-default cc skipABI -Dcheck_includes=true\
>
> Space before \ to be consistent with the rest of the file.
>
Sure.
>
> > -Dlibdir=lib -Dmachine=$default_machine $use_shared
> >
> > # 32-bit with default compiler
> > diff --git a/doc/guides/rel_notes/release_21_02.rst b/doc/guides/rel_notes/release_21_02.rst
> > index 33bac4fff8..245d1a6473 100644
> > --- a/doc/guides/rel_notes/release_21_02.rst
> > +++ b/doc/guides/rel_notes/release_21_02.rst
> > @@ -122,6 +122,14 @@ New Features
> > * Added support for aes-cbc sha256-128-hmac cipher combination in OCTEON TX2
> > crypto PMD lookaside protocol offload for IPsec.
> >
> > +* **Added support for build-time checking of header includes**
> > +
> > + A new build option ``check_includes`` has been added, which, when enabled,
> > + will perform build-time checking on DPDK public header files, to ensure none
> > + are missing dependent header includes. This feature, disabled by default, is
> > + intended for use by developers contributing to the DPDK SDK itself, and is
> > + integrated into the build scripts and automated CI for patch contributions.
> > +
> >
>
> Should be earlier in the new features list.
>
This was actually a deliberate choice on my part, since I was in two minds
as to whether or how this should be called out in the release note. I felt
it was not really of interest to user, and only to DPDK developers, so I
put it at the end to de-emphasise it.
On Thu, Jan 28, 2021 at 12:27 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > +# some ethdev headers depend on bus headers
> > > +includes = include_directories('../../drivers/bus/pci',
> > > + '../../drivers/bus/vdev')
> >
> > ethdev headers are fine now, afaics.
> > So this comment can be changed to a more vague "some driver headers
> > depend on bus headers".
> >
> Driver headers are not checked yet by the code, only lib ones, so I will
> see if this block can be omitted until needed.
I tried yesterday, iirc, you'll hit issues with eventdev driver headers.
On Thu, Jan 28, 2021 at 12:31:20PM +0100, David Marchand wrote:
> On Thu, Jan 28, 2021 at 12:27 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > > +# some ethdev headers depend on bus headers
> > > > +includes = include_directories('../../drivers/bus/pci',
> > > > + '../../drivers/bus/vdev')
> > >
> > > ethdev headers are fine now, afaics.
> > > So this comment can be changed to a more vague "some driver headers
> > > depend on bus headers".
> > >
> > Driver headers are not checked yet by the code, only lib ones, so I will
> > see if this block can be omitted until needed.
>
> I tried yesterday, iirc, you'll hit issues with eventdev driver headers.
>
Ok, thanks for trying.
On Thu, Jan 28, 2021 at 12:31:20PM +0100, David Marchand wrote:
> On Thu, Jan 28, 2021 at 12:27 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > > +# some ethdev headers depend on bus headers
> > > > +includes = include_directories('../../drivers/bus/pci',
> > > > + '../../drivers/bus/vdev')
> > >
> > > ethdev headers are fine now, afaics.
> > > So this comment can be changed to a more vague "some driver headers
> > > depend on bus headers".
> > >
> > Driver headers are not checked yet by the code, only lib ones, so I will
> > see if this block can be omitted until needed.
>
> I tried yesterday, iirc, you'll hit issues with eventdev driver headers.
>
Ok, I've done up a patch for eventdev similar to that of ethdev to make
private the rte_eventdev_pmd*.h files, and these lines can now be removed.
The eventdev files already had doxygen comments in them that they were not
for application use.
/Bruce
@@ -1562,6 +1562,10 @@ F: app/test/test_resource.c
F: app/test/virtual_pmd.c
F: app/test/virtual_pmd.h
+Header build sanity checking
+M: Bruce Richardson <bruce.richardson@intel.com>
+F: buildtools/chkincs/
+
Sample packet helper functions for unit test
M: Reshma Pattan <reshma.pattan@intel.com>
F: app/test/sample_packet_forward.c
new file mode 100755
@@ -0,0 +1,12 @@
+#! /usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2021 Intel Corporation
+
+from sys import argv
+from os.path import abspath
+
+(h_file, c_file) = argv[1:]
+
+contents = '#include "' + abspath(h_file) + '"'
+with open(c_file, 'w') as cf:
+ cf.write(contents)
new file mode 100644
@@ -0,0 +1,4 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2021 Intel Corporation
+ */
+int main(void) { return 0; }
new file mode 100644
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2021 Intel Corporation
+
+if not get_option('check_includes')
+ build = false
+ subdir_done()
+endif
+
+if is_windows
+ # for windows, the shebang line in the script won't work.
+ error('option "check_includes" is not supported on windows')
+endif
+
+gen_c_file_for_header = find_program('gen_c_file_for_header.py')
+gen_c_files = generator(gen_c_file_for_header,
+ output: '@BASENAME@.c',
+ arguments: ['@INPUT@', '@OUTPUT@'])
+
+cflags = machine_args
+cflags += '-Wno-unused-function' # needed if we include generic headers
+cflags += '-DALLOW_EXPERIMENTAL_API'
+
+# some ethdev headers depend on bus headers
+includes = include_directories('../../drivers/bus/pci',
+ '../../drivers/bus/vdev')
+
+sources = files('main.c')
+sources += gen_c_files.process(dpdk_chkinc_headers)
+
+deps = []
+foreach l:enabled_libs
+ deps += get_variable('static_rte_' + l)
+endforeach
+
+executable('chkincs', sources,
+ c_args: cflags,
+ include_directories: includes,
+ dependencies: deps,
+ link_whole: dpdk_static_libraries + dpdk_drivers,
+ install: false)
@@ -227,7 +227,7 @@ default_machine='nehalem'
if ! check_cc_flags "-march=$default_machine" ; then
default_machine='corei7'
fi
-build build-x86-default cc skipABI \
+build build-x86-default cc skipABI -Dcheck_includes=true\
-Dlibdir=lib -Dmachine=$default_machine $use_shared
# 32-bit with default compiler
@@ -122,6 +122,14 @@ New Features
* Added support for aes-cbc sha256-128-hmac cipher combination in OCTEON TX2
crypto PMD lookaside protocol offload for IPsec.
+* **Added support for build-time checking of header includes**
+
+ A new build option ``check_includes`` has been added, which, when enabled,
+ will perform build-time checking on DPDK public header files, to ensure none
+ are missing dependent header includes. This feature, disabled by default, is
+ intended for use by developers contributing to the DPDK SDK itself, and is
+ integrated into the build scripts and automated CI for patch contributions.
+
Removed Items
-------------
@@ -68,6 +68,11 @@ if get_option('enable_kmods')
subdir('kernel')
endif
+# check header includes if requested
+if get_option('check_includes')
+ subdir('buildtools/chkincs')
+endif
+
# write the build config
build_cfg = 'rte_build_config.h'
configure_file(output: build_cfg,