[libcamera-devel] [PATCH v1 4/6] apps: Move libevent dependency to src/apps/meson.build

[Kieran Bingham]

Quoting Laurent Pinchart (2022-10-20 11:09:45)
> Hi Kieran,
> 
> On Thu, Oct 20, 2022 at 10:06:32AM +0100, Kieran Bingham wrote:
> > Quoting Paul Elder via libcamera-devel (2022-10-20 07:54:45)
> > > On Thu, Oct 20, 2022 at 02:15:35AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > libtiff is a shared dependency between cam and lc-compliance, move it to
> > > 
> > > s/libtiff/libevent/
> > > 
> > > > src/apps/. The shared dependency will be used to condition compilation
> > > > of source files in an upcoming application static library.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > >  src/apps/cam/meson.build           | 4 +---
> > > >  src/apps/lc-compliance/meson.build | 3 +--
> > > >  src/apps/meson.build               | 8 ++++++++
> > > >  3 files changed, 10 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/src/apps/cam/meson.build b/src/apps/cam/meson.build
> > > > index 06dbea0645b7..4b6099ddab63 100644
> > > > --- a/src/apps/cam/meson.build
> > > > +++ b/src/apps/cam/meson.build
> > > > @@ -1,8 +1,6 @@
> > > >  # SPDX-License-Identifier: CC0-1.0
> > > >  
> > > > -libevent = dependency('libevent_pthreads', required : get_option('cam'))
> > > > -
> > > > -if not libevent.found()
> > > > +if opt_cam.disabled() or not libevent.found()
> > > >      cam_enabled = false
> > > >      subdir_done()
> > > >  endif
> > > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build
> > > > index 8b57474be2b2..05d622be0a40 100644
> > > > --- a/src/apps/lc-compliance/meson.build
> > > > +++ b/src/apps/lc-compliance/meson.build
> > > > @@ -1,10 +1,9 @@
> > > >  # SPDX-License-Identifier: CC0-1.0
> > > >  
> > > > -libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> > > >  libgtest = dependency('gtest', required : get_option('lc-compliance'),
> > > >                        fallback : ['gtest', 'gtest_dep'])
> > > >  
> > > > -if not (libevent.found() and libgtest.found())
> > > > +if opt_lc_compliance.disabled() or not libevent.found() or not libgtest.found()
> > > >      lc_compliance_enabled = false
> > > >      subdir_done()
> > > >  endif
> > > > diff --git a/src/apps/meson.build b/src/apps/meson.build
> > > > index 9e4388bd7881..159deb0b7fc2 100644
> > > > --- a/src/apps/meson.build
> > > > +++ b/src/apps/meson.build
> > > > @@ -1,5 +1,13 @@
> > > >  # SPDX-License-Identifier: CC0-1.0
> > > >  
> > > > +opt_cam = get_option('cam')
> > > > +opt_lc_compliance = get_option('lc-compliance')
> > > > +
> > > > +libevent = dependency('libevent_pthreads', required : opt_cam)
> > > > +if not libevent.found()
> > > > +    libevent = dependency('libevent_pthreads', required : opt_lc_compliance)
> > > > +endif
> > 
> > This ... is an odd way to express this. I expect we can't do sufficient
> > boolean operations with opt_cam and opt_lc_compliance?
> 
> Unfortunately not, they're feature options, so tri-state. One
> alternative is
> 
> if opt_cam.disabled() and opt_lc_compliance.disabled()
>     libevent = disabler()
> else
>     libevent = dependency('libevent_pthreads', required : opt_cam.enabled() or opt_lc_compliance.enabled())
> endif
> 
> but I don't think that's better.
> 
> > > > +libevent = dependency('libevent_pthreads', required : opt_cam)
> > > > +if not libevent.found()
> > > > +    libevent = dependency('libevent_pthreads', required : opt_lc_compliance)
> > > > +endif
> 
> > It deserves a comment above to say why we are searching for the
> > dependency twice. Though I expect anyone trying to optimise it to a
> > single line would quickly hit whatever has caused you to break it out to
> > two.
> 
> Would this work for you ?
> 
> # libevent is needed by cam and lc-compliance. As they are both feature options,
> # they can't be combined with simple boolean logic.

That' solves my concern thanks. It's just an odd code pattern, that
doesn't read well, so the comment helps. Thanks.



> 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > > > +
> > > >  subdir('lc-compliance')
> > > >  
> > > >  subdir('cam')
> 
> -- 
> Regards,
> 
> Laurent Pinchart