[libcamera-devel] [PATCH] libcamera: Enable vimc pipeline handler when tests are enabled

[Laurent Pinchart]

Hi Sebastian,

On Tue, Feb 23, 2021 at 07:08:57AM +0100, Sebastian Fricke wrote:
> On 23.02.2021 03:15, Laurent Pinchart wrote:
> >
> > The
> 
> Hmm something must have gone wrong here right?

Well... OK, maybe this is a tiny bit too terse ;-)

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/ipa/meson.build  |  2 +-
> >  meson.build                        | 13 ++++++++++++-
> >  src/ipa/meson.build                |  2 +-
> >  src/libcamera/pipeline/meson.build |  2 +-
> >  4 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > index a4d3f868dd41..2d72c1fcc8d6 100644
> > --- a/include/libcamera/ipa/meson.build
> > +++ b/include/libcamera/ipa/meson.build
> > @@ -72,7 +72,7 @@ ipa_mojoms = []
> >  foreach file : ipa_mojom_files
> >      name = file.split('.')[0]
> > 
> > -    if not get_option('pipelines').contains(name)
> > +    if name not in pipelines
> 
> This is probably nitpicking but it seems like you perform two changes in
> this patch, I see that you added the pipelines variable and adjusted
> code sequences that use it accordingly and that you enable vimc for
> tests. Shouldn't those two things be separated?

In general I like splitting separate changes in individual patches, but
in this case the introduction of the pipelines variable is a small
change, and is meant to support enabling the vimc pipeline handler
automatically, so I think it's best to keep them together.

> >          continue
> >      endif
> > 
> > diff --git a/meson.build b/meson.build
> > index be77191d22c4..1768f6eaf98e 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -111,6 +111,17 @@ py_modules = []
> >  # Libraries used by multiple components
> >  liblttng = cc.find_library('lttng-ust', required : get_option('tracing'))
> > 
> > +# Pipeline handlers
> > +#
> > +# Tests require the vimc pipeline handler, include it automatically when tests
> > +# are enabled.
> > +pipelines = get_option('pipelines')
> > +
> > +if get_option('test') and 'vimc' not in pipelines
> > +    message('Enabling vimc pipeline handler to support tests')
> > +    pipelines += ['vimc']
> > +endif
> > +
> >  # Utilities are parsed first to provide support for other components.
> >  subdir('utils')
> > 
> > @@ -156,5 +167,5 @@ py_mod.find_installation('python3', modules: py_modules)
> > 
> >  ## Summarise Configurations
> >  summary({
> > -            'Enabled pipelines': get_option('pipelines'),
> > +            'Enabled pipelines': pipelines,
> >          }, section : 'Configuration')
> > diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> > index 9d623f227a1f..df385eae84d3 100644
> > --- a/src/ipa/meson.build
> > +++ b/src/ipa/meson.build
> > @@ -22,7 +22,7 @@ ipa_sign = files('ipa-sign.sh')
> >  ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc']
> >  ipa_names = []
> > 
> > -foreach pipeline : get_option('pipelines')
> > +foreach pipeline : pipelines
> >      if ipas.contains(pipeline)
> >          subdir(pipeline)
> >          ipa_names += join_paths(ipa_install_dir, ipa_name + '.so')
> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> > index 46424493aa37..30dc5b97f1dc 100644
> > --- a/src/libcamera/pipeline/meson.build
> > +++ b/src/libcamera/pipeline/meson.build
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: CC0-1.0
> > 
> > -foreach pipeline : get_option('pipelines')
> > +foreach pipeline : pipelines
> >      subdir(pipeline)
> >  endforeach
> 
> I've compiled with this patch and run tests.
> 
> ```
> meson --reconfigure build -Dpipelines=rkisp1
> ninja -C build test
> # output
> ...
>    Configuration
>      Enabled pipelines: rkisp1
> 
> Found ninja-1.10.1 at /usr/bin/ninja
> [20/67] Linking target test/ipa/ipa_interface_test
> FAILED: test/ipa/ipa_interface_test 
> c++  -o test/ipa/ipa_interface_test test/ipa/ipa_interface_test.p/ipa_interface_test.cpp.o -Wl,--as-needed -Wl,--no-undefined -Wshadow -include config.h -Wl,--start-group src/ipa/libipa/libipa.a src/libcamera/libcamera.so test/libtest/liblibtest.a -Wl,--end-group '-Wl,-rpath,$ORIGIN/../../src/libcamera' -Wl,-rpath-link,/home/basti/libcamera/build/src/libcamera
> /usr/bin/ld: test/ipa/ipa_interface_test.p/ipa_interface_test.cpp.o: in function `std::_MakeUniq<libcamera::ipa::vimc::IPAProxyVimc>::__single_object std::make_unique<libcamera::ipa::vimc::IPAProxyVimc, libcamera::IPAModule*&, bool>(libcamera::IPAModule*&, bool&&)':
> /usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `libcamera::ipa::vimc::IPAProxyVimc::IPAProxyVimc(libcamera::IPAModule*, bool)'
> collect2: error: ld returned 1 exit status
> [27/67] Linking target test/media_device/media_device_print_test
> ninja: build stopped: subcommand failed.
> 
> 
> # After applying the patch
> ninja -C build test
> #output
> ...
>    Configuration
>      Enabled pipelines: rkisp1
>                         vimc
> 
> Found ninja-1.10.1 at /usr/bin/ninja
> [69/70] Running all tests.
> ```
> 
> If you like you can add:
> Tested-by: Sebastian Fricke<sebastian.fricke at posteo.net>

Thank you!

-- 
Regards,

Laurent Pinchart