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

14468 diff mbox series

libgtest-dev is provided as a static library. The compiler and linker
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@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

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@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 ?

>      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@jmondi.org>

Thanks
   j

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

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@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@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
> >

Hi Kieran and Jacopo, thank you for commenting.

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