Re: [dpdk-dev] [PATCH v6 6/8] buildtools/chkincs: add app to verify header includes

[Bruce Richardson <bruce.richardson@intel.com>]

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.