You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/10/06 13:57:41 UTC
[GitHub] [tvm] kparzysz-quic opened a new pull request #9208: [cpptest] Use find_package to locate GTest files
kparzysz-quic opened a new pull request #9208:
URL: https://github.com/apache/tvm/pull/9208
There is a standard CMake utility for finding GTest, use that instead of doing a manual search.
Thanks @tkonolige for the suggestion!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #9208:
URL: https://github.com/apache/tvm/pull/9208
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#discussion_r723559973
##########
File path: CMakeLists.txt
##########
@@ -52,6 +52,7 @@ tvm_option(INDEX_DEFAULT_I64 "Defaults the index datatype to int64" ON)
tvm_option(USE_LIBBACKTRACE "Build libbacktrace to supply linenumbers on stack traces" AUTO)
tvm_option(BUILD_STATIC_RUNTIME "Build static version of libtvm_runtime" OFF)
tvm_option(USE_PAPI "Use Performance Application Programming Interface (PAPI) to read performance counters" OFF)
+tvm_option(USE_GTEST "Use GoogleTest for C++ sanity tests" AUTO)
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#issuecomment-939137540
seems CI is green, let's land this and we can always do the GMock in a follow-on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#discussion_r723525084
##########
File path: CMakeLists.txt
##########
@@ -52,6 +52,7 @@ tvm_option(INDEX_DEFAULT_I64 "Defaults the index datatype to int64" ON)
tvm_option(USE_LIBBACKTRACE "Build libbacktrace to supply linenumbers on stack traces" AUTO)
tvm_option(BUILD_STATIC_RUNTIME "Build static version of libtvm_runtime" OFF)
tvm_option(USE_PAPI "Use Performance Application Programming Interface (PAPI) to read performance counters" OFF)
+tvm_option(USE_GTEST "Use GoogleTest for C++ sanity tests" AUTO)
Review comment:
Can you also add a line to `cmake/config.cmake`.
##########
File path: CMakeLists.txt
##########
@@ -585,22 +596,14 @@ endif()
# Create the `cpptest` target if we can find GTest. If not, we create dummy
# targets that give the user an informative error message.
-if(GTEST_INCLUDE_DIR AND GTEST_LIB)
+if(GTEST_FOUND)
file(GLOB_RECURSE TEST_SRCS tests/cpp/*.cc)
add_executable(cpptest ${TEST_SRCS})
- target_include_directories(cpptest SYSTEM PUBLIC ${GTEST_INCLUDE_DIR})
- target_link_libraries(cpptest PRIVATE ${TVM_TEST_LIBRARY_NAME} ${GTEST_LIB} gtest_main pthread dl)
+ target_include_directories(cpptest SYSTEM PUBLIC ${GTEST_INCLUDE_DIRS})
+ target_link_libraries(cpptest PRIVATE ${TVM_TEST_LIBRARY_NAME} ${GTEST_BOTH_LIBRARIES} pthread dl)
Review comment:
```suggestion
target_link_libraries(cpptest PRIVATE GTest::GTest GTest::Main)
```
In cmake it is preferred to use targets over manually specifying include directories and libraries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#issuecomment-936300428
Hi @kparzysz-quic, is it possible to do this for `GMock` as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#issuecomment-936778921
> @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 https://github.com/apache/tvm/pull/9106 and have updated the CI images to include it with https://github.com/apache/tvm/pull/9107 and https://github.com/apache/tvm/pull/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 :smile_cat:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#discussion_r723589474
##########
File path: CMakeLists.txt
##########
@@ -585,22 +596,14 @@ endif()
# Create the `cpptest` target if we can find GTest. If not, we create dummy
# targets that give the user an informative error message.
-if(GTEST_INCLUDE_DIR AND GTEST_LIB)
+if(GTEST_FOUND)
file(GLOB_RECURSE TEST_SRCS tests/cpp/*.cc)
add_executable(cpptest ${TEST_SRCS})
- target_include_directories(cpptest SYSTEM PUBLIC ${GTEST_INCLUDE_DIR})
- target_link_libraries(cpptest PRIVATE ${TVM_TEST_LIBRARY_NAME} ${GTEST_LIB} gtest_main pthread dl)
+ target_include_directories(cpptest SYSTEM PUBLIC ${GTEST_INCLUDE_DIRS})
+ target_link_libraries(cpptest PRIVATE ${TVM_TEST_LIBRARY_NAME} ${GTEST_BOTH_LIBRARIES} pthread dl)
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#issuecomment-937012973
@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](https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_search_module) which uses PKGConfig to find the libraries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#issuecomment-936326262
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #9208: [cpptest] Use find_package to locate GTest files
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #9208:
URL: https://github.com/apache/tvm/pull/9208#issuecomment-936732595
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org