[libcamera-devel] [PATCH] lc-compliance: Build with googltest in subproject

[Kieran Bingham]

Quoting Jacopo Mondi (2021-11-08 08:43:58)
> Hi Hiro,
> 
> On Mon, Nov 08, 2021 at 03:01:35PM +0900, Hirokazu Honda wrote:
> > libgtest-dev is provided as a static library. The compiler and linker
> 
> Isn't this actually a distribution-specific decision (to ship gtest as
> a static library or not) ?
> 
> It however causes issues, in example building on Debian 10 with gcc8
> http://buildbot.uovobw.net:8080/#/builders/6/builds/1
> 
> > to create the static library might be different from ones used for
> > libcamera. This causes a problem upon linking.
> >
> > This puts googltest code to subprojects, builds the code and link it
> > for lc-compliance. However, libgtest is locally built as a library on
> > ChromeOS and thus the used compiler and linker are the same as one
> > used for libcamera. We don't do these on ChromeOS build environment.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  README.rst                    |  2 +-
> >  src/lc-compliance/meson.build | 14 ++++++++++----
> >  subprojects/.gitignore        |  4 +++-
> >  subprojects/gtest.wrap        | 15 +++++++++++++++
> >  4 files changed, 29 insertions(+), 6 deletions(-)
> >  create mode 100644 subprojects/gtest.wrap
> >
> > diff --git a/README.rst b/README.rst
> > index 8af5f118..c48b4dba 100644
> > --- a/README.rst
> > +++ b/README.rst
> > @@ -99,7 +99,7 @@ for android: [optional]
> >          libexif-dev libjpeg-dev libyaml-dev
> >
> >  for lc-compliance: [optional]
> > -        libevent-dev libgtest-dev
> > +        libevent-dev
> >
> >  Using GStreamer plugin
> >  ~~~~~~~~~~~~~~~~~~~~~~
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index aa5852f6..4d13ad00 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -1,15 +1,21 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >
> > -libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> > -libgtest = dependency('gtest', required : get_option('lc-compliance'))
> > -
> > -if not (libevent.found() and libgtest.found())
> > +if not get_option('lc-compliance').enabled()
> 
> Why change this ? Shouldn't this just become
> 
> libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> 
> if not (libevent.found()
>       lc_compliance_enabled = false
>       subdir_done()
> endif
> 
> Afaict meson 'auto' features should be enabled/disabled depending on
> the availability of its dependencies at runtime ?
> 
> I'm not against changing this to be a boolean like in example the
> 'v4l2' build-option, but a default should then be provided in
> meson_options.txt ?

I don't think we should change the behaviour here. If libevent_pthreads
isn't found, then lc-compliance should be disabled.

If it is found, then it can be automatically enabled, since we will
either use the (CrOS) provided gtest, or we will build it ourselves
through the wrap.


> 
> >      lc_compliance_enabled = false
> >      subdir_done()
> >  endif
> >
> >  lc_compliance_enabled = true
> >
> > +libevent = dependency('libevent_pthreads', required : true)
> > +
> > +if get_option('android_platform') == 'cros'
> > +   libgtest = dependency('gtest', required : true)
> > +else
> > +   libgtest_sp = subproject('gtest')
> > +   libgtest = libgtest_sp.get_variable('gtest_dep')
> > +endif
> > +
> >  lc_compliance_sources = files([
> >      '../cam/event_loop.cpp',
> >      '../cam/options.cpp',
> > diff --git a/subprojects/.gitignore b/subprojects/.gitignore
> > index 410b8bd6..82ef2c83 100644
> > --- a/subprojects/.gitignore
> > +++ b/subprojects/.gitignore
> > @@ -1 +1,3 @@
> > -/libyuv
> > \ No newline at end of file
> > +/libyuv
> > +/googletest-release*
> > +/packagecache
> 
> What's packagecache and why is added here now ?
> 
> > \ No newline at end of file
> > diff --git a/subprojects/gtest.wrap b/subprojects/gtest.wrap
> > new file mode 100644
> > index 00000000..8513793f
> > --- /dev/null
> > +++ b/subprojects/gtest.wrap
> > @@ -0,0 +1,15 @@
> > +[wrap-file]
> > +directory = googletest-release-1.11.0
> > +source_url = https://github.com/google/googletest/archive/release-1.11.0.zip
> > +source_filename = gtest-1.11.0.zip
> > +source_hash = 353571c2440176ded91c2de6d6cd88ddd41401d14692ec1f99e35d013feda55a
> > +patch_filename = gtest_1.11.0-1_patch.zip
> > +patch_url = https://wrapdb.mesonbuild.com/v2/gtest_1.11.0-1/get_patch
> > +patch_hash = d38c39184384608b08419be52aed1d0f9d9d1b5ed71c0c35e51cccbdddab7084
> > +
> > +[provide]
> > +gtest = gtest_dep
> > +gtest_main = gtest_main_dep
> > +gmock = gmock_dep
> > +gmock_main = gmock_main_dep
> > +
> 
> I get a complaint when applying the patch
> 
> .git/rebase-apply/patch:82: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
> 
> The change fixes building on older platforms where indeed the static
> library and libcamera were built with two different compilers version
> and we had errors at linking time. Very nice!
> 
> Tested-by: Jacopo Mondi <jacopo at jmondi.org>

I think this will fix things for me too, so I'm looking forward to
seeing it merged.

Thanks.

Kieran


> 
> Thanks
>    j
> 
> > --
> > 2.34.0.rc0.344.g81b53c2807-goog
> >