sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
871 stars 297 forks source link

[Testing] Externalize (find_package() or fetch) googletest/gtest #4471

Closed fredroy closed 3 months ago

fredroy commented 3 months ago

Originally, the goal was not really to go for

But more to upgrade the gtest version, as the one in SOFA is really old (1.7.0 , Sep 19, 2013 :exclamation: ) and was throwing warnings for newer cmake. So instead of simply update the source in extlibs, I took the liberty to go into the find/fetch mechanism.

A bit difficult as the CMakefile in the embedded version was heavily customized, so I am not 100% sure the install/package process is working well. But it should šŸ˜…

Last note: it is adding a target gtest_main which is always activated in the gtest cmake file. It is not used by SOFA as we have our own main for gtest.

[ci-depends-on https://github.com/sofa-framework/PluginExample/pull/5]


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

fredroy commented 3 months ago

[ci-build][with-all-tests]

fredroy commented 3 months ago

[ci-build][with-all-tests][force-full-build]

sofabot commented 3 months ago

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

sofabot commented 3 months ago

[ci-depends-on] detected during build #5.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

fredroy commented 3 months ago

@olivier-roussel thanks for the conda test and the feedbacks šŸ‘ You should have made suggestions and/or make a PR on my branch, as I cannot credit you your patches šŸ˜µ

sofabot commented 3 months ago

[ci-depends-on] detected during build #6.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 3 months ago

[ci-depends-on] detected during build #7.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 3 months ago

[ci-depends-on] detected during build #8.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

fredroy commented 3 months ago

[ci-build][with-all-tests][force-full-build]

sofabot commented 3 months ago

[ci-depends-on] detected during build #9.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 3 months ago

[ci-depends-on] detected during build #10.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

fredroy commented 3 months ago

@olivier-roussel The findGTest https://cmake.org/cmake/help/v3.19/module/FindGTest.html of cmake only works for CMake >= 3.20 šŸ˜­ (tested with cmake 3.16 and 3.19 āŒ whereas cmake 3.20 and 3.28 āœ…)

olivier-roussel commented 3 months ago

@olivier-roussel thanks for the conda test and the feedbacks šŸ‘ You should have made suggestions and/or make a PR on my branch, as I cannot credit you your patches šŸ˜µ

Indeed, sorry about that

olivier-roussel commented 3 months ago

@olivier-roussel The findGTest https://cmake.org/cmake/help/v3.19/module/FindGTest.html of cmake only works for CMake >= 3.20 šŸ˜­ (tested with cmake 3.16 and 3.19 āŒ whereas cmake 3.20 and 3.28 āœ…)

Indeed again :cry: Another option but maybe a bit conda-specific would be to switch to config mode find_package(GTest CONFIG QUIET), as the conda package comes with the GTest cmake config files. Tested with conda + cmake 3.19.

olivier-roussel commented 3 months ago

Ok so maybe I misunderstood your last comment, but the GTest find module you added in your last commit works fine on my side with cmake >= 3.12 (which is our min required version). So I guess you meant that the embedded GTest find module in recent cmake version (>=3.20) was required to have the correct targets defined, which is not the case in earlier <3.20 version of cmake (tested in 3.12), but this recent GTest find module was not building if used with cmake <3.20. In the end, your last commit seems to fix everything, so looks like a more generic option that relying on cmake config files as suggested in my last comment.

fredroy commented 3 months ago

Ok so maybe I misunderstood your last comment, but the GTest find module you added in your last commit works fine on my side with cmake >= 3.12 (which is our min required version). So I guess you meant that the embedded GTest find module in recent cmake version (>=3.20) was required to have the correct targets defined, which is not the case in earlier <3.20 version of cmake (tested in 3.12), but this recent GTest find module was not building if used with cmake <3.20. In the end, your last commit seems to fix everything, so looks like a more generic option that relying on cmake config files as suggested in my last comment.

I just added the cmake module from v3.20 for GTest directly in our cmake folder modules; not very clean but it does the job apparently šŸ‘

sofabot commented 3 months ago

[ci-depends-on] detected during build #14.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

sofabot commented 3 months ago

[ci-depends-on] detected during build #15.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1: