Re: [dpdk-dev] [PATCH] ethdev: introduce enable_driver_sdk to install driver headers

[Thomas Monjalon <thomas@monjalon.net>]

12/03/2021 16:34, Bruce Richardson:
> On Fri, Mar 12, 2021 at 04:25:09PM +0100, David Marchand wrote:
> > On Fri, Mar 12, 2021 at 4:20 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > >
> > > On 3/11/2021 7:27 PM, Tyler Retzlaff wrote:
> > > > Introduce a meson option enable_driver_sdk when true installs internal
> > > > driver headers for ethdev. this allows drivers that do not depend on
> > > > stable api/abi to be built external to the dpdk source tree.
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > > >
> > > > it's still unclear to me if we should be renaming the headers:
> > > >
> > > >    ethdev_driver.h -> rte_ethdev_driver.h
> > > >    ethdev_pci.h -> rte_ethdev_pci.h
> > > >    ethdev_vdev.h -> rte_ethdev_vdev.h
> > > >
> > > >   lib/librte_ethdev/meson.build | 5 +++++
> > > >   meson_options.txt             | 2 ++
> > > >   2 files changed, 7 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
> > > > index c37b2e377..7ecdec6f0 100644
> > > > --- a/lib/librte_ethdev/meson.build
> > > > +++ b/lib/librte_ethdev/meson.build
> > > > @@ -20,6 +20,11 @@ headers = files('rte_ethdev.h',
> > > >       'rte_mtr_driver.h',
> > > >       'rte_tm.h',
> > > >       'rte_tm_driver.h')
> > > > +if get_option('enable_driver_sdk')
> > > > +headers += files('ethdev_driver.h',
> > > > +     'ethdev_pci.h',
> > > > +     'ethdev_vdev.h')
> > > > +endif
> > >
> > > Instead of adding the "get_option('enable_driver_sdk')" checks to the modules,
> > > what about a more generic solution, like:
> > >
> > > modules assign relevant headers into a new variable, let's say 'pmd_headers',
> > > and in a high level meson file, all 'pmd_headers' are installed if
> > > 'enable_driver_sdk' enabled?
> > >
> > 
> > +1.
> > Just, I don't like "pmd_headers" as the list name.
> > It can be misunderstood as the list of pmd-specific headers (thinking
> > of rte_pmd_i40e.h), that are exposed to applications.
> >
> Since the option is called "enable_driver_sdk" the variable name of
> "driver_sdk_headers" would be a good match. We should try and keep variable
> names and option names in sync as much as possible.

+1