diff mbox series

ethdev: introduce enable_driver_sdk to install driver headers

Message ID 1615490833-23052-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: introduce enable_driver_sdk to install driver headers |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS

Commit Message

Tyler Retzlaff March 11, 2021, 7:27 p.m. UTC 
  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(+)

Comments

Ferruh Yigit March 12, 2021, 3:19 p.m. UTC | #1 
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?
David Marchand March 12, 2021, 3:25 p.m. UTC | #2 
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.
Bruce Richardson March 12, 2021, 3:34 p.m. UTC | #3 
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.
Thomas Monjalon March 12, 2021, 3:52 p.m. UTC | #4 
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
Tyler Retzlaff March 12, 2021, 6:43 p.m. UTC | #5 
On Fri, Mar 12, 2021 at 04:52:05PM +0100, Thomas Monjalon wrote:
> 12/03/2021 16:34, Bruce Richardson:
> > > >
> > > > 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
> 

seems like there is consensus. i'll figure out how introduce a
driver_sdk_headers variable generically and remove the per-meson.build
conditional evaluation.

thanks
diff mbox series

Patch

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
 indirect_headers += files(
 	'rte_ethdev_core.h',
 	'rte_eth_ctrl.h')
diff --git a/meson_options.txt b/meson_options.txt
index 6eff62e47..857874a19 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -8,6 +8,8 @@  option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>',
 	description: 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
 option('enable_docs', type: 'boolean', value: false,
 	description: 'build documentation')
+option('enable_driver_sdk', type: 'boolean', value: false,
+	description: 'install internal driver plugin headers')
 option('enable_kmods', type: 'boolean', value: false,
 	description: 'build kernel modules')
 option('examples', type: 'string', value: '',