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

[Jacopo Mondi]

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 ?

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

Thanks
   j

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