Skip to content
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

[cpptest] Use find_package to locate GTest files #9208

Merged
merged 5 commits into from Oct 8, 2021
Merged

[cpptest] Use find_package to locate GTest files #9208

merged 5 commits into from Oct 8, 2021

Conversation

kparzysz-quic
Copy link
Contributor

There is a standard CMake utility for finding GTest, use that instead of doing a manual search.

Thanks @tkonolige for the suggestion!

There is a standard CMake utility for finding GTest, use that instead
of doing a manual search.

Thanks @tkonolige for the suggestion!
@Mousius
Copy link
Member

Mousius commented Oct 6, 2021

Hi @kparzysz-quic, is it possible to do this for GMock as well?

@kparzysz-quic
Copy link
Contributor Author

I don't know. From what I just googled it seems like GMock is somehow handled by GoogleTest, which is included by the FindGTest routine.

@tkonolige
Copy link
Contributor

@Mousius If I understand this thread https://gitlab.kitware.com/cmake/cmake/-/issues/17365 correctly, and the cmake documentation (which says "New in version 3.20: Upstream GTestConfig.cmake is used if possible."), if the version of gtest is new enough (>=1.8) and cmake is new enough (>=3.20) then the find_package(GTest) will include a GMock target. Unfortunately, our required cmake version is much lower than 3.20 so we cannot rely on this feature. But right now we aren't using GMock are we?

Copy link
Contributor

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @kparzysz-quic! It should make finding gtest a lot more reliable. I've left a couple of small changes to make.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@Mousius
Copy link
Member

Mousius commented Oct 6, 2021

@Mousius If I understand this thread https://gitlab.kitware.com/cmake/cmake/-/issues/17365 correctly, and the cmake documentation (which says "New in version 3.20: Upstream GTestConfig.cmake is used if possible."), if the version of gtest is new enough (>=1.8) and cmake is new enough (>=3.20) then the find_package(GTest) will include a GMock target. Unfortunately, our required cmake version is much lower than 3.20 so we cannot rely on this feature. But right now we aren't using GMock are we?

Hi @tkonolige,

We don't currently use GMock, I have used it as part of #9106 and have updated the CI images to include it with #9107 and #9141. It'd greatly help our ability to write good C++ tests to have GMock setup properly - although I'm only using the headers iirc so it works without referencing gmock or gmock_main.

I'm also not going to suggest we don't do this until we figure all that out, I had just hoped it'd be easy enough to do 😸

Krzysztof Parzyszek added 3 commits October 6, 2021 13:11
Cmake will automatically propagate the interface include directories
to the dependends of (in this case) GTest.
@tkonolige
Copy link
Contributor

@Mousius If we required a recent version of gmock, then we could just use the cmake config it provides. But all versions of Ubuntu seem to not include it, so that approach is out. So unfortunately I think you'll have to write your own FindGMock cmake file. I find it easiest to use pkg_search_module which uses PKGConfig to find the libraries.

@areusch
Copy link
Contributor

areusch commented Oct 8, 2021

seems CI is green, let's land this and we can always do the GMock in a follow-on.

@areusch areusch merged commit a931a2d into apache:main Oct 8, 2021
@kparzysz-quic kparzysz-quic deleted the gtest-find-package branch October 8, 2021 22:06
masahi pushed a commit to Laurawly/tvm-1 that referenced this pull request Oct 14, 2021
* [cpptest] Use find_package to locate GTest files

There is a standard CMake utility for finding GTest, use that instead
of doing a manual search.

Thanks @tkonolige for the suggestion!

* Use GTest:: targets instead of variable

* Add USE_GTEST to cmake/config.cmake

* Remove GTEST_INCLUDE_DIRS from target_include_directories

Cmake will automatically propagate the interface include directories
to the dependends of (in this case) GTest.

* Restart CI
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
* [cpptest] Use find_package to locate GTest files

There is a standard CMake utility for finding GTest, use that instead
of doing a manual search.

Thanks @tkonolige for the suggestion!

* Use GTest:: targets instead of variable

* Add USE_GTEST to cmake/config.cmake

* Remove GTEST_INCLUDE_DIRS from target_include_directories

Cmake will automatically propagate the interface include directories
to the dependends of (in this case) GTest.

* Restart CI
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* [cpptest] Use find_package to locate GTest files

There is a standard CMake utility for finding GTest, use that instead
of doing a manual search.

Thanks @tkonolige for the suggestion!

* Use GTest:: targets instead of variable

* Add USE_GTEST to cmake/config.cmake

* Remove GTEST_INCLUDE_DIRS from target_include_directories

Cmake will automatically propagate the interface include directories
to the dependends of (in this case) GTest.

* Restart CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants