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

[Laurent Pinchart]

Hi Hiro,

On Thu, Feb 03, 2022 at 10:47:25AM +0900, Hirokazu Honda wrote:
> On Thu, Feb 3, 2022 at 10:09 AM Laurent Pinchart wrote:
> > 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.
> 
> Ah yeah, it is libc++ vs. libstdc++. I forgot that.
> 
> > 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.
> 
> That's right. Both libgtest and libcamera are built with libc++ on
> ChromeOS build environment.
> Therefore this is not problem. But I build libmcaera on Debian, I use
> clang for various convenient c++ checks e.g. thread-safety, so libc++
> is used. https://git.linuxtv.org/libcamera.git/tree/meson.build#n71
> I have to not use clang or uninstall either libc++ or libgtest.
> This change makes it impossible to build libcamera with clang++, if
> the installed libgest is built with different c++ standard library,
> unless meson.build is manually modified.
> Hmm, yeah maybe I should uninstall libgtest on my Debian.

No need to, you can use meson magic :-)

meson configure -Dforce_fallback_for=gtest

and meson will fallback to the wrap for gtest, even if it is detected in
the system.

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