[dpdk-dev] [PATCH v2 01/22] event/dlb2: add documentation and meson build infrastructure

[Bruce Richardson]

On Tue, Oct 20, 2020 at 04:33:42PM +0100, McDaniel, Timothy wrote:
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas at monjalon.net>
> > Sent: Tuesday, October 20, 2020 10:21 AM
> > To: McDaniel, Timothy <timothy.mcdaniel at intel.com>; Eads, Gage
> > <gage.eads at intel.com>
> > Cc: Richardson, Bruce <bruce.richardson at intel.com>; Jerin Jacob
> > <jerinjacobk at gmail.com>; Mcnamara, John <john.mcnamara at intel.com>;
> > Kovacevic, Marko <marko.kovacevic at intel.com>; Ray Kinsella
> > <mdr at ashroe.eu>; Neil Horman <nhorman at tuxdriver.com>; dpdk-dev
> > <dev at dpdk.org>; Carrillo, Erik G <erik.g.carrillo at intel.com>; Van Haaren, Harry
> > <harry.van.haaren at intel.com>; Jerin Jacob <jerinj at marvell.com>;
> > david.marchand at redhat.com
> > Subject: Re: [dpdk-dev] [PATCH v2 01/22] event/dlb2: add documentation and
> > meson build infrastructure
> >
> > 20/10/2020 17:17, McDaniel, Timothy:
> > > From: Bruce Richardson <bruce.richardson at intel.com>
> > > > On Sun, Oct 18, 2020 at 02:18:32PM +0530, Jerin Jacob wrote:
> > > > > On Sat, Oct 17, 2020 at 11:50 PM Timothy McDaniel
> > > > > <timothy.mcdaniel at intel.com> wrote:
> > > > > >
> > > > > > Adds the meson build infrastructure, which includes
> > > > > > compile-time constants in rte_config.h. DLB2 is
> > > > > > only supported on Linux X86 platforms at this time.
> > > > > >
> > > > > > Signed-off-by: Timothy McDaniel <timothy.mcdaniel at intel.com>
> > > > > > Reviewed-by: Gage Eads <gage.eads at intel.com>
> > > > > > ---
> > > > > > --- a/drivers/event/meson.build
> > > > > > +++ b/drivers/event/meson.build
> > > > > > @@ -10,6 +10,9 @@ if not (toolchain == 'gcc' and
> > > > cc.version().version_compare('<4.8.6') and
> > > > > >         dpdk_conf.has('RTE_ARCH_ARM64'))
> > > > > >         drivers += 'octeontx'
> > > > > >  endif
> > > > > > +if (dpdk_conf.has('RTE_ARCH_X86_64') and is_linux)
> > > > > > +        drivers += 'dlb2'
> > > > > > +endif
> > > > >
> > > > > Please add the message in "Content Skipped" section,
> > > > > Reference: grep "reason" in drivers/vdpa/mlx5/meson.build
> > > > >
> > > >
> > > > The octeontx case is also wrong in this file, IMHO. Rather than checking
> > > > things in the event level and adding things to the list, the list should
> > > > just be static. If something should be optionally compiled, then check the
> > > > conditions in the driver meson.build file itself and add "build=false" to
> > > > disable, setting "reason" to the cause of it being disabled. This keeps all
> > > > the logic about a driver in the other file, rather than someone having to
> > > > look in multiple places for why something is or isn't getting built
> > > > properly.
> > > >
> > > > Regards,
> > > > /Bruce
> > >
> > > Due to time constraints, I would prefer to take these issues up in a future
> > release.
> >
> > If you have strong time constraints to complete everything,
> > then it is better to postpone the feature to the next release.
> >
> 
> You read too much into my comment.
> My point is that there does not seem to be community consensus on this issue,

There is consensus here, and the way the octeon event driver is doing
things is wrong, and was just missed in previous reviews.

> so why are we debating it at this point in the release cycle.  There are plenty of examples
> doing exactly what I am doing.
> 
Looking through the driver/*/meson.build files this appears to be the only
incidence where a driver is optionally adding itself to the list. All other
drivers are always processed so that when enabled/disabled they can be
properly listed in the configuration summary and a reason given.

Regards,
/Bruce