-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Testing] Externalize (find_package() or fetch) googletest/gtest #4471
Conversation
[ci-build][with-all-tests] |
[ci-build][with-all-tests][force-full-build] |
[ci-depends-on] detected during build #4. To unlock the merge button, you must
|
[ci-depends-on] detected during build #5. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the following adjustments, this compiles both using fetched gtest or using external gtest package from conda (tested on linux-x64 with gtest 1.14.0 from conda-forge).
@olivier-roussel thanks for the conda test and the feedbacks 👍 |
[ci-depends-on] detected during build #6. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
[ci-depends-on] detected during build #7. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
[ci-depends-on] detected during build #8. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
[ci-build][with-all-tests][force-full-build] |
[ci-depends-on] detected during build #9. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
[ci-depends-on] detected during build #10. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
@olivier-roussel |
cbfc102
to
eacd343
Compare
Indeed, sorry about that |
Indeed again 😢 |
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 👍 |
[ci-depends-on] detected during build #14. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
…ge of the old one
0895143
to
c406196
Compare
[ci-depends-on] detected during build #15. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
* find_package gtest or fetch gtest ; and use it for testing.Remove usage of the old one * fix compilation for newer gtest * remove source of the embedded gtest * update cmake message for SOFA_ALLOW_FETCH_DEPENDENCIES * fix define * apply suggestion by using GTest ; and remove CONFIG keyword * use cmake 3.20 version for gtest modules
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 ❗ ) 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