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

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

On Thu, Sep 25, 2025 at 12:29:08PM +0200, David Marchand wrote:
> On Thu, 25 Sept 2025 at 11:22, Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Thu, Sep 25, 2025 at 10:42:47AM +0200, David Marchand wrote:
> > > Hello Bruce,
> > >
> > > On Thu, 25 Sept 2025 at 10:00, Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Wed, Sep 24, 2025 at 07:25:34PM +0200, David Marchand wrote:
> > > > > A problem with the current headers check is that it relies on meson
> > > > > dependencies objects that come with their include_directories
> > > > > directives, and all of those point at the library / driver sources.
> > > > >
> > > > > This means that we won't detect a public header including a private
> > > > > (as in, not exported) header, or a driver only header.
> > > > >
> > > > > To address this issue, a staging directory is added and every header
> > > > > is copied to it.
> > > > >
> > > > > Drivers and library headers are staged to two different directories
> > > > > and the check is updated accordingly.
> > > > >
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > >
> > > > In general looks ok to me.  One small comment though - can we not have
> > > > "staging" as a top-level directory, but instead hide it inside the
> > > > buildtools directory, or even the chkincs directory? I dislike having
> > > > too many subdirectories directly off the root of the project,
> > > > especially ones purely for internal tooling.
> > >
> > > Well, at first I was trying to change the whole build process iow rely
> > > only on the staging directory and remove all the include_directories:
> > > directives from the declare_dependency() objects.  Libraries and apps
> > > were ok, but there were a *lot* of complications with drivers (what a
> > > *huge mess*, especially for NXP drivers with "compat.h" includes, and
> > > Marvell drivers to a smaller extent).  I may retry in the future with
> > > some AI tool that will brute force this :-).
> > >
> > One note of caution here: if doing this, you may want to consider using
> > run_command rather than a custom_target to copy the headers in order to
> > avoid potentially slowing down the build.
> >
> > We used to do this copying of header files in the old make build system,
> > and one downside of it is that it means that the build of ".c" files in one
> > directory cannot be started until the copying of headers from the dependent
> > components are completed. The decision to use the include paths in the
> > dependency objects rather than copying the headers to a central location
> > was a deliberate decision when moving build system because it means that
> > when you run "ninja", every single .c file can be compiled to a .o file
> > in parallel, because all dependent headers are available at their original
> > locations. For most components, it's only the final link stage that has any
> > dependencies, the compile commands are all independent.
> >
> > On the other hand, using run_command is not necessarily a good solution
> > either, because it means that all changes to the headers require a re-run
> > of meson, which will also be slower because of all the copying. :-(
> >
> > Therefore, I'd tend towards keeping things as they are here, in order to
> > minimise reconfiguration and build times.
> 
> I am sticking to the headers check update atm.
> The slow down should only concern developers using check_includes,
> once I properly gate those deps to the meson option.
> 
> Is it still a concern for you?
> 
No, no concerns about copying the headers over for check_includes checks,
that makes sense. I'm just flagging a potential issue if you want to switch
back to the old model of copying the headers as part of a normal build.

/Bruce