[libcamera-devel] [RFC PATCH] ipa: rpi: Make boost optional
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Sep 24 17:12:07 CEST 2020
Hi Jacopo,
On 24/09/2020 16:12, 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
You're in luck.
That's what you currently have ...
My issue with this is that if I have a system which does not have boost,
and is not a Raspberry Pi - *by default* libcamera will fail to build -
because RPi is enabled by default.
>> 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.
I sort of like this, that enables people to explicitly state "I need
RPi", and anything else comes along for the ride as an auto value.
--
Kieran
>>
>>> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list