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

[paul.elder at ideasonboard.com]

On Thu, Sep 24, 2020 at 05:12:07PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Thu, Sep 24, 2020 at 04:12:30PM +0300, Laurent Pinchart wrote:
> > 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)
> 
> That would be my preference as
> 
> > 2. Skip the IPA module but keep the pipeline handler enabled (that's
> >    what this patch implements)
> 
> Building a pipeline without the associated IPA, if any, doesn't make
> much sense if not for testing and development

I think this option makes the most sense. We support closed-source
IPAs, which by extension means we support out-of-tree IPAs. This means
that we need to support building pipelines without also building their
IPAs.

When libcamera is run, it prints an error anyway if an IPA isn't found.
I think that's sufficient for notifying the user why their
pipeline-without-IPA isn't working.

> > 3. Disable the pipeline handler automatically (that would be similar to
> >    the "feature" option type of meson)
> 
> and this would possibly hide problems and puzzle users.
> 
> Fail early and loudly seems more reasonable to me :)
> 
> >
> > 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.

That would work too :)


Paul