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

[Barnabás Pőcze]

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.


Regards,
Barnabás Pőcze

> 
> 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
>