| Message ID | 20200924100201.353624-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran 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@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. > > > 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 ? > endif > > subdir('data') > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 24/09/2020 11:27, Jacopo Mondi wrote: > Hi Kieran > > 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@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. > >> >> >> 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 ? We can of course have the build fail if the RPi pipeline is enabled, and boost is not found, but that's not what this patch does. In fact, I think you're describing the current behaviour of libcamera ;-) This patch disables the RPi IPA *only* even if the RPi Pipeline is enabled, but boost is not found, (because it is enabled, by default). -- Kieran >> endif >> >> subdir('data') >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On 2020-09-24 12:27:35 +0200, Jacopo Mondi wrote: > Hi Kieran > > 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@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, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > endif > > > > subdir('data') > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@ragnatech.se>
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@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@ragnatech.se> > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@ragnatech.se> >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, On Thu, Sep 24, 2020 at 04:12:07PM +0100, Kieran Bingham wrote: > 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@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 ... Ah sorry, I completely missed that > > 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. > I see, I defer any decision to you build system educated people then.
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@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
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') endif subdir('data')
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@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... src/ipa/raspberrypi/meson.build | 40 +++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 17 deletions(-)