Use external GoogleTest optionally #4563

cz4rs · 2021-11-30T10:30:48Z

Use find_package(GTest) to check for external GoogleTest library.
When not available, fallback to our internal copy in tpls/gtest.
edit: Always use internal copy when building as a part of Trilinos.

Notes:

It is possible to force usage of the internal copy with CMAKE_DISABLE_FIND_PACKAGE

-DCMAKE_DISABLE_FIND_PACKAGE_GTest=TRUE

(for example in case when a GTest package is found, but it's not working correctly)

Google Test official recommendations: Incorporating Into An Existing CMake Project

.github/workflows/continuous-integration-workflow.yml

cz4rs · 2021-12-02T22:23:16Z

Should I add something like this:

When the tests are enabled, Kokkos will try to find and use available GoogleTest package by default. To force the use of internal copy, configure with
-DCMAKE_DISABLE_FIND_PACKAGE_GTest=TRUE

to README.md / BUILD.md? Or is that too much detail?

CMakeLists.txt

core/unit_test/CMakeLists.txt

cz4rs · 2021-12-03T20:14:46Z

core/unit_test/CMakeLists.txt

-)
-
-# avoid deprecation warnings from MSVC
-TARGET_COMPILE_DEFINITIONS(kokkos_gtest PUBLIC GTEST_HAS_TR1_TUPLE=0 GTEST_HAS_PTHREAD=0)


Actually GTEST_HAS_TR1_TUPLE option was removed completely from gtest.

To other reviewers: Make sure you see that this is dropping the compile definition -DGTEST_HAS_PTHREAD=0

dalg24 · 2021-12-08T17:27:06Z

Retest this please

dalg24 · 2021-12-08T17:29:09Z

core/unit_test/CMakeLists.txt

-)
-
-# avoid deprecation warnings from MSVC
-TARGET_COMPILE_DEFINITIONS(kokkos_gtest PUBLIC GTEST_HAS_TR1_TUPLE=0 GTEST_HAS_PTHREAD=0)


To other reviewers: Make sure you see that this is dropping the compile definition -DGTEST_HAS_PTHREAD=0

algorithms/unit_tests/CMakeLists.txt

core/unit_test/CMakeLists.txt

masterleinad

I think we should test this with Trilinos before merging. Otherwise, this looks good to me.

dalg24 · 2021-12-14T14:35:37Z

I think we should test this with Trilinos before merging. Otherwise, this looks good to me.

Not a bad idea. @ndellingwood would you assist with this?

cz4rs · 2021-12-14T19:03:20Z

I think we should test this with Trilinos before merging. Otherwise, this looks good to me.

Not a bad idea. @ndellingwood would you assist with this?

No experience with Trilinos here, so I would appreciate someone either testing it or walking me through 👍

ndellingwood · 2021-12-14T22:55:52Z

@cz4rs @dalg24 I'll test some builds of Trilinos with this PR

ndellingwood · 2021-12-15T03:24:38Z

@cz4rs @dalg24 so far I tested Trilinos with builds without an external Gtest package, so they fell back to the internal copy; for that case everything passed fine, no surprise. I still need to install and test with Gtest as an external package.

My understanding with these changes is that the use of Gtest as an external package here would be isolated to Kokkos' testing, so this should not impact users of Trilinos (and downstream applications) that rely on the Trilinos Gtest package

ndellingwood · 2021-12-15T05:08:08Z

@dalg24 @cz4rs My assumption was incorrect - If I enable gtest as TPL in Trilinos and add my external gtest install (googletest v.1.8.0) to the system PATH so my install is found by CMake, then Trilinos configuration fails:

CMake Error at cmake/tribits/core/package_arch/TribitsAddExecutable.cmake:477 (message):
  ERROR: 'GTest::gtest' in TESTONLYLIBS not a TESTONLY lib! If this a regular
  library in this SE package or in an dependent upstream SE package then
  TriBITS will link automatically to it.  If you remove this and it does not
  link, then you need to add a new SE package dependency to this SE package's
  dependencies file
  /ascldap/users/ndellin/trilinos/Trilinos/kokkos/core/cmake/Dependencies.cmake
Call Stack (most recent call first):
  cmake/tribits/core/package_arch/TribitsAddExecutableAndTest.cmake:68 (tribits_add_executable)
  cmake/tribits/core/package_arch/TribitsAddExecutableAndTest.cmake:214 (tribits_add_executable_wrapper)
  kokkos/cmake/kokkos_tribits.cmake:161 (TRIBITS_ADD_EXECUTABLE_AND_TEST)
  kokkos/core/unit_test/CMakeLists.txt:412 (KOKKOS_ADD_EXECUTABLE_AND_TEST)

The relevant CMake configure options I used were:

-D TPL_ENABLE_GTEST=ON  -D GTEST_INCLUDE_DIR=<path-to-gtest-include>

I confirmed the above works with Kokkos' develop branch, so the changes in this PR are impacting the Trilinos configuration. Some applications use gtest through Trilinos for their testing, so this will need to be resolved for acceptance of the next release with Trilinos.

When testing I modified my existing configuration to enable Gtest as TPL as above, but a simplified Trilinos setup and configuration to reproduce using just Kokkos' develop branch should look something like this:

Clone the Trilinos repo
git clone https://github.com/trilinos/Trilinos.git
Add a symbolic link to local kokkos repo with this PR checked out to the Trilinos source directory, see
https://github.com/kokkos/kokkos/wiki/Compiling#building-trilinos-with-kokkos-develop-branch
Configure with CMake >= 3.17 - below will configure with Kokkos' serial backend

cmake -DCMAKE_CXX_COMPLER=<g++,clang++, or icpc> -DTPL_ENABLE_GTEST=ON  -DGTEST_INCLUDE_DIR=<path-to-ext-gtest-include> -DTrilinos_ENABLE_Kokkos=ON -DKokkos_ENABLE_TESTS=ON -DKokkos_SOURCE_DIR_OVERRIDE:STRING=kokkos $TRILINOS_DIR

ndellingwood · 2021-12-15T05:46:49Z

I tried another configuration option, enabling Gtest as a package in Trilinos (i.e. -DTrilinos_ENABLE_Gtest=ON) and that case configured without issue with the changes in this PR.
Looking through some relevant Trilinos github issues, there is explicit mention of applications using Gtest as a package through Trilinos (I previously commented that Gtest was enabled as TPL). Better (or proper?) support for enabling Gtest as a TPL in Trilinos is a desired feature as discussed here trilinos/Trilinos#8001 along with a PR with work towards Gtest TPL support in Trilinos here trilinos/Trilinos#9514

cz4rs · 2021-12-15T14:36:29Z

@dalg24 @cz4rs My assumption was incorrect - If I enable gtest as TPL in Trilinos and add my external gtest install (googletest v.1.8.0) to the system PATH so my install is found by CMake, then Trilinos configuration fails (...)

Thanks for the feedback! I will see what can be done about this (and take a look at the Trilinos issues linked).

cz4rs · 2021-12-15T15:22:07Z

Rebased on top of develop and added c206c49 - this forces the use of internal copy of GTest when building as a part of Trilinos.

Rombur

It looks good. Do we still need the changes in cmake/kokkos_tribits.cmake since we use the internal gtest with Trilinos?

Rombur · 2021-12-16T16:31:24Z

cmake/kokkos_tribits.cmake

+  SET(KOKKOS_GTEST_LIB GTest::gtest)
+ELSE()  # fallback to internal gtest
+  SET(KOKKOS_GTEST_LIB kokkos_gtest)
+ENDIF()


Do we still need these changes since we now always use us the internal gtest with Trilinos?

Yes, because it's still possible to use either the internal copy or external GTest (without Trilinos).

cz4rs marked this pull request as ready for review December 1, 2021 14:29

cz4rs linked an issue Dec 1, 2021 that may be closed by this pull request

use external GoogleTest optionally #4384

Closed

3 tasks

cz4rs commented Dec 2, 2021

View reviewed changes

.github/workflows/continuous-integration-workflow.yml Show resolved Hide resolved

dalg24 reviewed Dec 3, 2021

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

cz4rs commented Dec 3, 2021

View reviewed changes

core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved

cz4rs commented Dec 3, 2021

View reviewed changes

dalg24 approved these changes Dec 8, 2021

View reviewed changes

masterleinad reviewed Dec 13, 2021

View reviewed changes

algorithms/unit_tests/CMakeLists.txt Show resolved Hide resolved

masterleinad reviewed Dec 13, 2021

View reviewed changes

core/unit_test/CMakeLists.txt Show resolved Hide resolved

masterleinad reviewed Dec 13, 2021

View reviewed changes

cz4rs added 11 commits December 15, 2021 16:09

Find external google test for algorithms unit tests

39bba2d

Find and use GTest package consistently

a163dc2

Remove redundant enable_testing() call

ff755f0

Remove kokkosalgorithms_gtest target

775ee75

Only search for GTest once

b2eece4

Fallback to internal GTest when package not found

5b721e9

Test builiding with external GTest in CI

f472fa6

Only search for GTest when tests are enabled

a0b8c05

Let gtest auto-detect pthreads

4bc0779

Remove obsolete build option

760f57d

Link Threads library to tests when found

83edd10

Disable external GTest when building as a part of Trilinos

c206c49

Rombur reviewed Dec 16, 2021

View reviewed changes

Rombur approved these changes Dec 16, 2021

View reviewed changes

crtrott merged commit bdd8ffa into kokkos:develop Dec 16, 2021

cz4rs mentioned this pull request Dec 16, 2021

use external GoogleTest optionally #4384

Closed

3 tasks

masterleinad mentioned this pull request Feb 3, 2022

Allow using external gtest #4263

Closed

ndellingwood mentioned this pull request Feb 9, 2022

Add custom implementation for strcasecmp kokkos/kokkos-kernels#1227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use external GoogleTest optionally #4563

Use external GoogleTest optionally #4563

cz4rs commented Nov 30, 2021 •

edited

cz4rs commented Dec 2, 2021

cz4rs Dec 3, 2021

dalg24 Dec 8, 2021

dalg24 commented Dec 8, 2021

dalg24 Dec 8, 2021

masterleinad left a comment

dalg24 commented Dec 14, 2021

cz4rs commented Dec 14, 2021

ndellingwood commented Dec 14, 2021

ndellingwood commented Dec 15, 2021

ndellingwood commented Dec 15, 2021 •

edited

ndellingwood commented Dec 15, 2021

cz4rs commented Dec 15, 2021

cz4rs commented Dec 15, 2021

Rombur left a comment

Rombur Dec 16, 2021

dalg24 Dec 16, 2021

cz4rs Dec 16, 2021

Use external GoogleTest optionally #4563

Use external GoogleTest optionally #4563

Conversation

cz4rs commented Nov 30, 2021 • edited

cz4rs commented Dec 2, 2021

cz4rs Dec 3, 2021

Choose a reason for hiding this comment

dalg24 Dec 8, 2021

Choose a reason for hiding this comment

dalg24 commented Dec 8, 2021

dalg24 Dec 8, 2021

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

dalg24 commented Dec 14, 2021

cz4rs commented Dec 14, 2021

ndellingwood commented Dec 14, 2021

ndellingwood commented Dec 15, 2021

ndellingwood commented Dec 15, 2021 • edited

ndellingwood commented Dec 15, 2021

cz4rs commented Dec 15, 2021

cz4rs commented Dec 15, 2021

Rombur left a comment

Choose a reason for hiding this comment

Rombur Dec 16, 2021

Choose a reason for hiding this comment

dalg24 Dec 16, 2021

Choose a reason for hiding this comment

cz4rs Dec 16, 2021

Choose a reason for hiding this comment

cz4rs commented Nov 30, 2021 •

edited

ndellingwood commented Dec 15, 2021 •

edited