Re: [PATCH v4 6/7] buildtools/chkincs: use a staging directory for headers

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

On Fri, Sep 26, 2025 at 10:14:31AM +0200, David Marchand wrote:
> On Thu, 25 Sept 2025 at 16:47, Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
> > > index 49dbc55254..376556f5f4 100644
> > > --- a/buildtools/chkincs/meson.build
> > > +++ b/buildtools/chkincs/meson.build
> > > @@ -12,38 +12,47 @@ gen_c_files = generator(gen_c_file_for_header,
> > >          arguments: ['@INPUT@', '@OUTPUT@'])
> > >
> > >  cflags = machine_args
> > > +cflags += '-Wno-missing-field-initializers'
> > >  cflags += no_wvla_cflag
> > > +cflags += '-I' + dpdk_source_root + '/config'
> > > +cflags += '-I' + dpdk_build_root
> > > +cflags += '-I' + dpdk_chkinc_staging_dir
> > >
> >
> > Two minor suggestions, 1) should we not use "include_directories()" here to
> > pass these properly? and 2) would using the existing "global_inc" value as
> > include path not work ok? Is the reason for not using the latter the fact
> > it includes too many source dirs?
> 
> 1) include_directories() requires a source directory to exist, so I
> would need to keep almost empty directories with minimal meson.build
> in them.
> 
Ack.

> 2) Indeed, global_inc is not usable as is.
> 
> meson.build:global_inc = [include_directories('.', 'config',
> meson.build-    'lib/eal/include',
> meson.build-    'lib/eal/@0@/include'.format(host_machine.system()),
> meson.build-    'lib/eal/@0@/include'.format(arch_subdir),
> meson.build-)]
> 
> global_inc refers to the EAL sources, so anything in those directories
> is reachable.
> There are a number of non exported headers in there, like
> lib/eal/include/rte_eal_paging.h or Windows specific/internal headers
> in lib/eal/windows/include.
> 
> I can split it as two variables.
>

Ok, thanks. Was just making suggestions to try and improve things.
 
> >
> > >  sources = files('main.c')
> > >  sources += gen_c_files.process(dpdk_chkinc_headers)
> > >
> > > -# some driver SDK headers depend on these two buses, which are mandatory in build
> > > -# so we always include them in deps list
> > > -deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
> > > -if dpdk_conf.has('RTE_BUS_VMBUS')
> > > -    deps += get_variable('shared_rte_bus_vmbus')
> > > -endif
> 
> [snip]
> 
> > > diff --git a/drivers/meson.build b/drivers/meson.build
> > > index f25f425565..dae1e83ca4 100644
> > > --- a/drivers/meson.build
> > > +++ b/drivers/meson.build
> > > @@ -245,7 +245,31 @@ foreach subpath:subdirs
> > >          if get_option('enable_driver_sdk')
> > >              install_headers(driver_sdk_headers)
> > >          endif
> > > -        dpdk_chkinc_headers += driver_sdk_headers
> > > +        dpdk_chkinc_headers += headers
> > > +        dpdk_chkinc_drivers_headers += driver_sdk_headers
> > > +
> > > +        if get_option('check_includes')
> > > +            if headers.length() > 0
> > > +                dpdk_chkinc_staging_deps += declare_dependency(sources:
> > > +                        custom_target(lib_name + '_header_staging',
> > > +                                output: lib_name + '_header_staging.stamp',
> > > +                                command: [stage_headers_cmd, dpdk_chkinc_staging_dir, '@OUTPUT@',
> > > +                                    headers],
> > > +                                install: false,
> > > +                        )
> > > +                )
> >
> > Rather than using an external script and custom target for this, would the
> > configure_file() meson function, with "copy" set to true also work for us?
> > https://mesonbuild.com/Reference-manual_functions.html#configure_file
> 
> I don't like configure_file().
> Touching any of those headers will trigger a meson reconfigure, right?
> I think I am not the only one who dislike meson reconfigurations that
> pollutes your terminal a lot when you do many compilations (Thomas?).
> 
> But let's say this is acceptable.
> 
> I had tried but configure_file() won't accept outputting to a specific
> directory, or I am doing it wrong?
> Trying again as an example in lib/meson.build:
>         foreach header: headers + indirect_headers
>             configure_file(input: header,
>                 output: 'staging/@BASENAME@.h',
>                 copy: true,
>                 install: false,
>             )
>         endforeach
> 
> Gives:
> ../lib/meson.build:221:12: ERROR: configure_file keyword argument
> "output" Output 'staging/@BASENAME@.h' must not contain a path
> segment.
> 
> 
> Relooking *again* at all this (la nuit porte conseil), if we tracked
> headers in global variables, for "normal" headers, arch headers,
> generic headers and drivers headers, all of the changes could be
> isolated in the buildtools/chkincs directory.
> This would also make it possible to factorize install_headers() calls,
> and avoid drivers doing it behind drivers/meson.build back (I spotted
> one).
> 
> I just pushed an experiment:
> - single headers install:
> https://github.com/david-marchand/dpdk/commit/headers_rework~2
> - chkincs rework (still not using configure_file):
> https://github.com/david-marchand/dpdk/commit/headers_rework~1
> 
> WDYT?
> 

Those changes seem good to me.

One final idea/suggestion (apologies for the many suggestions here). Rather
than having a new script and dependency tracking for the header copies,
would it be easier to do the header copy as part of the existing C file
generation script? It's already writing a C file to an output directory,
would it make sense to have that script just copy the header file to the
same location too?

/Bruce