[libcamera-devel] [RFC PATCH] ipa: rpi: Make boost optional

[Laurent Pinchart]

On Thu, Sep 24, 2020 at 02:35:45PM +0200, Niklas Söderlund wrote:
> On 2020-09-24 12:27:35 +0200, Jacopo Mondi wrote:
> > On Thu, Sep 24, 2020 at 11:02:01AM +0100, Kieran Bingham wrote:
> > > By default the Raspberry Pi pipeline handler is enabled when
> > > configuring the build.
> > >
> > > The Raspberry Pi IPA currently depends upon 'boost' which is a large
> > > dependency to bring in.
> > >
> > > Make the IPA compilation dependant upon the availabilty of the boost
> > > library, but display a warning when the pipeline is enabled and the
> > > dependency is not found.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >
> > > This is mostly posted to promote discussion on this topic.
> > >
> > > The requirement for boost is really heavy (+100MB package for a few
> > > headers), and is only used for the RPi IPA.
> > >
> > > This patch automatically disables the IPA while printing a message if it
> > > was selected if the boost library is not available.
> > >
> > > Doing all of this in a clean way seems quite difficult because of the
> > > way the meson options works ... so ...
> > >
> > > 3 ... 2 ... 1 ....
> > >
> > >    Discuss...
> > 
> > I really like the idea of depending on boost only if RPi is enabled.
> 
> +1
>  
> > >  src/ipa/raspberrypi/meson.build | 40 +++++++++++++++++++--------------
> > >  1 file changed, 23 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> > > index 9445cd097df5..b284378c9621 100644
> > > --- a/src/ipa/raspberrypi/meson.build
> > > +++ b/src/ipa/raspberrypi/meson.build
> > > @@ -2,9 +2,11 @@
> > >
> > >  ipa_name = 'ipa_rpi'
> > >
> > > +boost = dependency('boost', required : false)
> > > +
> > >  rpi_ipa_deps = [
> > >      libcamera_dep,
> > > -    dependency('boost'),
> > > +    boost,
> > >      libatomic,
> > >  ]
> > >
> > > @@ -41,22 +43,26 @@ rpi_ipa_sources = files([
> > >      'controller/pwl.cpp',
> > >  ])
> > >
> > > -mod = shared_module(ipa_name,
> > > -                    rpi_ipa_sources,
> > > -                    name_prefix : '',
> > > -                    include_directories : rpi_ipa_includes,
> > > -                    dependencies : rpi_ipa_deps,
> > > -                    link_with : libipa,
> > > -                    install : true,
> > > -                    install_dir : ipa_install_dir)
> > > -
> > > -if ipa_sign_module
> > > -    custom_target(ipa_name + '.so.sign',
> > > -                  input : mod,
> > > -                  output : ipa_name + '.so.sign',
> > > -                  command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> > > -                  install : false,
> > > -                  build_by_default : true)
> > > +if boost.found()
> > > +    mod = shared_module(ipa_name,
> > > +                        rpi_ipa_sources,
> > > +                        name_prefix : '',
> > > +                        include_directories : rpi_ipa_includes,
> > > +                        dependencies : rpi_ipa_deps,
> > > +                        link_with : libipa,
> > > +                        install : true,
> > > +                        install_dir : ipa_install_dir)
> > > +
> > > +    if ipa_sign_module
> > > +        custom_target(ipa_name + '.so.sign',
> > > +                      input : mod,
> > > +                      output : ipa_name + '.so.sign',
> > > +                      command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> > > +                      install : false,
> > > +                      build_by_default : true)
> > > +    endif
> > > +else
> > > +    warning('The Raspberry Pi pipeline is enabled, but dependency "boost" was not found')
> > 
> > 
> > I know nothing about meson, but can we have a contruct that
> >         if !boost.found
> >                 error "..."
> > 
> > and the build fails ?
> 
> I agree with Jacopo here. I think the build should fail if the RPi 
> pipeline is enabled and the dependencies for it can not be found. I 
> think fail early is good, otherwise someone may enable the RPi pipeline 
> have the compilation and installation succeed only to later find out 
> their $APP won't work as it can't find the IPA module.
> 
> Having it fail with an error(Friendly message) is also nicer then having 
> the compilation fail. So with s/warning(/error(/ above,

Before looking at the implementation, let's agree on the desired
behaviour. When a dependency needed by an IPA module isn't found, and
the corresponding pipeline handler is enabled, should we

1. Make configuration fail (that's the current behaviour)
2. Skip the IPA module but keep the pipeline handler enabled (that's
   what this patch implements)
3. Disable the pipeline handler automatically (that would be similar to
   the "feature" option type of meson)

For any of these options, we can add explicit warning or error messages.

The second option is the one I like the least, as the pipeline handler
will be unusable. The first option has the upside of notifying the user
very explicitly that something went wrong, but the downside of not
offering a nice way to only enable pipeline handlers that have their
dependencies met. The third option is the opposite, it makes
configuration more automatic, but at the expense of possibly confusing
users who will wonder why libcamera doesn't work on their platform.

There's a fourth option too, turning the pipelines option into
individual pipeline-* feature options (name subject to bikeshedding),
defaulting to auto. A user who wants to ensure support for their
platform is enabled can set the corresponding optio to enabled, and the
build will then fail if dependencies are not found.

> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

-- 
Regards,

Laurent Pinchart