[libcamera-devel] [RFC PATCH] ipa: rpi: Make boost optional
Jacopo Mondi
jacopo at jmondi.org
Thu Sep 24 17:12:07 CEST 2020
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
> 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 :)
Thanks
j
>
> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list