[PATCH v1] meson: Don't override pipeline list when `auto` is selected

[Laurent Pinchart]

On Tue, Dec 17, 2024 at 06:18:28PM +0000, Barnabás Pőcze wrote:
> 2024. december 17., kedd 17:22 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:
> 
> > Hi Barnabás,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Dec 17, 2024 at 03:15:16PM +0000, Barnabás Pőcze wrote:
> > > Consider `pipelines=auto,virtual`. Previously that would select
> > > everything that `auto` would, but not actually enable the `virtual`
> > > pipeline handler because the `pipelines` list was reset.
> > 
> > Use the imperative mood in the commit message, and the present tense to
> > describe the current behaviour.:
> > 
> > Consider the `pipelines=auto,virtual` meson option. One could expect it
> > to auto-select pipeline handlers and to enable the virtual pipeline
> > handler in addition. libcamera however ignores the `virtual` pipeline
> > handler because the `pipelines` list is reset by `auto`. As enabling
> > additional pipeline handlers beside auto-selection, fix this by
> > considering all pipeline handlers in the list.
> > 
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=247
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > ---
> > >  meson.build | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/meson.build b/meson.build
> > > index 33afbb741..5e5e4756d 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -204,7 +204,7 @@ liblttng = dependency('lttng-ust', required : get_option('tracing'))
> > >
> > >  # Pipeline handlers
> > >  #
> > > -pipelines = get_option('pipelines')
> > > +wanted_pipelines = get_option('pipelines')
> > >
> > >  arch_arm = ['arm', 'aarch64']
> > >  arch_x86 = ['x86', 'x86_64']
> > > @@ -220,16 +220,18 @@ pipelines_support = {
> > >      'virtual':      ['test'],
> > >  }
> > >
> > > -if pipelines.contains('all')
> > > +if wanted_pipelines.contains('all')
> > >      pipelines = pipelines_support.keys()
> > > -elif pipelines.contains('auto')
> > > +elif wanted_pipelines.contains('auto')
> > >      host_cpu = host_machine.cpu_family()
> > >      pipelines = []
> > >      foreach pipeline, archs : pipelines_support
> > > -        if host_cpu in archs or 'any' in archs
> > > +        if pipeline in wanted_pipelines or host_cpu in archs or 'any' in archs
> > >              pipelines += pipeline
> > >          endif
> > >      endforeach
> > > +else
> > > +    pipelines = wanted_pipelines
> > >  endif
> > 
> > Would the following be clearer ?
> 
> Well, after some thinking, unfortunately I have to report that
> it does not seem clearer to me personally. But if there is a
> consensus, I will change it.

I have a slight preference for the version below but both are OK with
me.

> > pipelines = {}
> > 
> > foreach pipeline : get_option('pipelines')
> >     if pipeline == 'all'
> >         pipelines += pipelines_support
> >     elif pipeline == 'auto'
> >         host_cpu = host_machine.cpu_family()
> >         foreach pipeline, archs : pipelines_support
> >             if host_cpu in archs or 'any' in archs
> >                 pipelines += {pipeline: true}
> >             endif
> >         endforeach
> >     else
> >         pipelines += {pipeline: true}
> >     endif
> > endforeach
> > 
> > pipelines = pipelines.keys()
> > 
> > I don't have a strong preference. Either way,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > >  # Tests require the vimc pipeline handler, include it automatically when tests

-- 
Regards,

Laurent Pinchart