| Message ID | 20220202143141.160660-1-javierm@redhat.com |
|---|---|
| State | Accepted |
| Commit | e526a57d09a3cb9fe226e70f896b4beaa8f9180a |
| Headers | show |
| Series |
|
| Related | show |
Hi Javier, Thank you for the patch. 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@redhat.com> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> 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@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([
Hi Javier, thank you for the patch. On Thu, Feb 3, 2022 at 2:32 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Javier, > > Thank you for the patch. > > 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@redhat.com> > > Signed-off-by: Javier Martinez Canillas <javierm@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. 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. Best Regards, -Hiro > > 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@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
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@redhat.com> > > > Signed-off-by: Javier Martinez Canillas <javierm@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@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([
Hi Laurent, On Thu, Feb 3, 2022 at 10:09 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@redhat.com> > > > > Signed-off-by: Javier Martinez Canillas <javierm@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. -Hiro > > > 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@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
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@redhat.com> > > > > > Signed-off-by: Javier Martinez Canillas <javierm@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@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([
Hi Laurent, On Thu, Feb 3, 2022 at 10:57 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@redhat.com> > > > > > > Signed-off-by: Javier Martinez Canillas <javierm@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. Aha, I didn't know the way. Thanks. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> -Hiro > > > > > > 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@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
On 2/3/22 03:50, Hirokazu Honda wrote: > Hi Laurent, [snip] >>> 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 >> I should probably had mentioned this in the commit message and also that you can make meson to fallback for all wrap subprojects with: meson build --wrap-mode=forcefallback I'll post a v2 mentioning these two commands to prevent others to make this more clear. >> and meson will fallback to the wrap for gtest, even if it is detected in >> the system. > > Aha, I didn't know the way. Thanks. > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > Thanks! Best regards,
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([
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@redhat.com> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- src/lc-compliance/meson.build | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)