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

Tue Nov 9 06:31:45 CET 2021

Hi Kieran and Jacopo, thank you for commenting.

On Mon, Nov 8, 2021 at 7:50 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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 ?
> >

The packages are cached in the directory.
$ ls subprojects/packagecache/
gtest_1.11.0-1_patch.zip  gtest-1.11.0.zip

If I understand the meson doc correctly [1], meson checks if a package
is downloaded or not by seeing source_filename/patch_filename and
comparing the computed hash with source_hash/patch_hash.
Definitely, we would not like to put it in git history. So we should
add packagecache to .gitignore.

[1] https://mesonbuild.com/Wrap-dependency-system-manual.html#accepted-configuration-properties-for-wraps
> > > \ 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.
> >

I downloaded this wrap file by `meson wrap install gtest`.
It is interesting it has the redundant empty line.
Removed the line.

Thanks,
-Hiro
> > 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
> > >