[libcamera-devel] [PATCH] lc-compliance: Only download a gtest subproject as a fallback

[Laurent Pinchart]

Hi Hiro,

On Thu, Feb 03, 2022 at 09:57:38AM +0900, Hirokazu Honda wrote:
> On Thu, Feb 3, 2022 at 2:32 AM Laurent Pinchart wrote:
> > On Wed, Feb 02, 2022 at 03:31:40PM +0100, Javier Martinez Canillas wrote:
> > > Currently, a subproject is used to fetch gtest dependency unconditionally
> > > for any Linux distribution besides ChromeOS. But it leads to a regression
> > > in distros whose builders are not allowed to download files during build.
> > >
> > > This change was introduced by commit 0d50a04cc918 ("lc-compliance: Build
> > > with gtest in subprojects") and the rationale is that some distros, such
> > > as Debian ship libgtest-dev as a static library. And this could be built
> > > with a different toolchain than the one used to build libcamera itself.
> > >
> > > But this seems to be a corner case, usually users will either build both
> > > libcamera and all its dependencies using the same toolchain or build it
> > > using both the libgtest library and toolchain as provided by the distro.
> > >
> > > If someone doesn't want for meson to pick up the non-compatible static
> > > library provided by the distro, then instead should make sure that their
> > > build root does not have the package providing this installed.
> > >
> > > Let's simplify the logic to find the dependency and just use the built-in
> > > support in dependency() function to fallback to a subproject if not found.
> > >
> > > This covers to common case of attempting to use the gtest provided by the
> > > system or pulling from source if not found or is not preferred.
> > >
> > > Fixes: commit 0d50a04cc918 ("lc-compliance: Build with gtest in subprojects")
> > > Reported-by: Eric Curtin <ecurtin at redhat.com>
> > > Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> 
> The fallback option happens if there is no libgtest in a system.
> https://mesonbuild.com/Dependencies.html#building-dependencies-as-subprojects
> It is at linking time that we find the libgtest doesn't work.
> So I think the problem happens in an environment where libgtest is
> installed and a different compiler from one building the libgtest is
> used for libcamera.

That's correct.

> If I understand correctly, the built-in libgest is compiled always with gcc?
>  I am not sure how edge case it is to build libcamera with a different compiler.

In most cases libcamera will be built with the same compiler as the rest
of the system, including the gtest version provided by the distribution.
Things should thus work fine. However, in our CI environment, we
typically compile-test libcamera with different versions of gcc and
clang, without using a separate build root that is compiled with the
same compiler. It failed linking to gtest with building with libcamera
with clang while the system is built with gcc, which is the usual case
(actually I believe it's not related to clang vs. gcc but to libc++ vs.
libstdc++). I used to disable lc-compliance when building with clang for
this reason, and your patch that added the gtest wrap helped increasing
the CI coverage.

I'm not entirely sure what other issues, if any, your patch fixes
though. On Chrome OS we kept building with the system-provided gtest, so
I suppose it wasn't about Chrome OS.

> > As discussed on IRC, this breaks the build when compiling libcamera with
> > clang + libc++ on a system providing a gtest package compiled with g++ +
> > libstdc++. Compilation also breaks with gcc 7 and gcc 8, due to the
> > corresponding libstdc++ having different versions of some symbols:
> >
> > /usr/lib/gcc/x86_64-pc-linux-gnu/7.4.0/../../../../x86_64-pc-linux-gnu/bin/ld: /usr/lib64/libgtest.so: undefined reference to `std::__cxx11::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> >::basic_ostringstream()@GLIBCXX_3.4.26'
> >
> > Such "cross" compilation isn't expected in most cases, but can be useful
> > for CI to test multiple compilers on a host system without a fully
> > separate build root. Setting the meson force_fallback_for option to
> > gtest fixes the CI compilation tests, so
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > ---
> > >
> > >  src/lc-compliance/meson.build | 17 +++--------------
> > >  1 file changed, 3 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > > index 130ddbb55916..8b57474be2b2 100644
> > > --- a/src/lc-compliance/meson.build
> > > +++ b/src/lc-compliance/meson.build
> > > @@ -1,25 +1,14 @@
> > >  # SPDX-License-Identifier: CC0-1.0
> > >
> > >  libevent = dependency('libevent_pthreads', required : get_option('lc-compliance'))
> > > +libgtest = dependency('gtest', required : get_option('lc-compliance'),
> > > +                      fallback : ['gtest', 'gtest_dep'])
> > >
> > > -if not libevent.found()
> > > +if not (libevent.found() and libgtest.found())
> > >      lc_compliance_enabled = false
> > >      subdir_done()
> > >  endif
> > >
> > > -if get_option('android_platform') == 'cros'
> > > -    libgtest = dependency('gtest', required : get_option('lc-compliance'))
> > > -
> > > -    if not libgtest.found()
> > > -        lc_compliance_enabled = false
> > > -        subdir_done()
> > > -    endif
> > > -
> > > -else
> > > -    libgtest_sp = subproject('gtest')
> > > -    libgtest = libgtest_sp.get_variable('gtest_dep')
> > > -endif
> > > -
> > >  lc_compliance_enabled = true
> > >
> > >  lc_compliance_sources = files([

-- 
Regards,

Laurent Pinchart