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.richard...@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.richard...@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.richard...@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.pat...@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.
