GTest as a TPL

[jhux2]

Question

@trilinos/framework @bartlettroscoe

The version of gtest in Trilinos is old, and projects such as Exawind would benefit from having a newer version.

Why is gtest part of Trilinos instead of being a TPL?
Which packages rely on gtest?

As an FYI, I believe @alanw0 is looking at upgrading gtest.

[bartlettroscoe]

@jhux2, I did not have anything to do with adding GTest as a Trilinos package. Given that STK is having version problems with GTest in Trilinos, a GTest TPL would also be problematic, right? Therefore, I think each package should just snapshot the version of GTest that they want into their source tree if the version of GTest provided by the Trilinos GTest package becomes a problem. That is what @etphipp did with Sacado. That is my two cents so I will not unsubscribe.

[alanw0]

Thanks @jhux2, I want to tag Timothy @tasmith4 as he is the one specifically looking into gtest.
I believe he is considering the relative merits of upgrading the trilinos gtest, vs having stk pull in its own copy of gtest. To me it feels less ideal to have each trilinos package pull in their own copy of gtest... But we understand there are pros and cons.

[etphipp]

As to why gtest is a package and not a TPL, my guess is so that Trilinos packages can use it without requiring a TPL dependency, which ensures the tests will actually be run. That was my motivation for using it in Sacado (previously Sacado used CppUnit, which almost no one has, so those tests were rarely run). I spent a lot of time trying to update the gtest package in Trilinos because I wanted to use functionality that was not in the Trilinos version. Ultimately that failed because I could never solve the cmake installation issues, so I ended up putting my own snapshot in Sacado (which does not get installed). The issues are described in issue #7698.

[etphipp]

I should also mention that if someone wants to pick this up and can solve the installation issues, I would be happy to remove the snapshot in Sacado.

[jhux2]

@bartlettroscoe @etphipp Thanks for the information. #7698 might be helpful for @tasmith4 as he decides on a path forward.

[ibaned]

I just spent about an hour unsuccessfully trying to build a code that actually uses GTest properly as a TPL from the actual repository here on GitHub where Google provides it, and also tries to use Trilinos. This practice of copy-pasting packages that are not part of the Trilinos project into Trilinos and installing them as if we are the sole providers of that package must stop. GTest, CppUnit, and all others must be turned into TPLs. If your users "don't have them", they are as easy to get from GitHub as Trilinos is.

[jhux2]

@jwillenbring @ccober6 Should this be added to the Trilinos planning board?

[ccober6]

I added it to the agenda for the next Trilinos Product Leads Meeting to discuss, and from there make the needed Jira issues.

[bartlettroscoe]

Just for the record, I did not have anything to do with adding gtest as a Trilinos package and most certainly did not have anything to do with installing it. (I think that was added while I was at ORNL.) This situation with the Gtest package was a major pain with Trilinos integration with SPARC and EMPIRE. (In fact, it was the last unresolved issue that stopped SPARC and EMPIRE from being able to use the exact same binary installs of Trilinos because SPARC needed the install of Gtest and EMPIRE broke if it was installed.) Now, if gtest was just built and used internally to test things in Trilinos, then that is fine. (But in that case, each package should just use their own version of gtest like Sacado and Kokkos do.) But given that it is being used by an installed Trilinos library, this is a huge mess.

Why force every Trilinos package to use the exact same version of GTest? Yes that saves in source space and binary space but given the experience that @etphipp had in #7698, is that really worth it?

[etphipp]

@bartlettroscoe brings up a good point. @ibaned have you or anyone else tested linking the installed Trilinos with the GTest TPL in #9514 against a code that uses it (e.g., Albany I think). This was the major stumbling block in #7698 and is why I ended up putting my own gtest library in Sacado instead of updating the GTest package in Trilinos. It required a custom CMake targets file for that library, which I didn't know how to create.

Generally I support the idea of removing the GTest package in Trilinos and replacing it with a TPL, however it does seem to me to be an awful lot of work to install the module everywhere Trilinos is tested. I also question why some Trilinos packages are allowed to keep their local gtest copy and others aren't. To me it seems like a simpler solution to understand why an installed Trilinos library is depending on gtest, and whether that is really required.

[bartlettroscoe]

. @ibaned have you or anyone else tested linking the installed Trilinos with the GTest TPL in #9514 against a code that uses it (e.g., Albany I think).

And you likely need to add SPARC to that list as well. (I would be curious to know how many other APPs used easy access to Gtest through Trilinos in this way. This could turn into a real mess.)

To me it seems like a simpler solution to understand why an installed Trilinos library is depending on gtest, and whether that is really required.

That is a great question. But if there is a Trilinos library that gets installed that depends on GTest, then I think that GTest has to be a TPL so that the customer can choose the GTest version (which is the whole problem here).

[ibaned]

Downstream applications can find GTest using the same (proper modern CMake) find_package command that Trilinos uses to find it as a TPL. This is actually much better because more convenient targets like GTest::gtest_main become available to them, as well as target properties beyond just the library and include lists that TriBITS provides. In short, everything the actual GTest developers want applications to be able to use becomes available.

That was actually the whole use case for this pull request, since I'm in charge of an application that uses Trilinos but also uses other things besides Trilinos and all of them get GTest from an installed location like well-behaved C++ projects. This works because instead of trying to launder GTest's CMake packages through TriBITS, I used them directly as intended.

Again, I built every relevant package in Trilinos with the latest GTest version and there were no issues, so this fear of issues with sharing one version is also completely unfounded.

[bartlettroscoe]

Again, I built every relevant package in Trilinos with the latest GTest version and there were no issues, so this fear of issues with sharing one version is also completely unfounded.

@ibaned, but what about APPs like SPARC and Albany (and perhaps others) that may be using the older GTest version through the Trilinos Gtest package that will break when the existing Gtest package comes out? That will break them and cause a fire-drill and force them to get their own GTest TPL and upgrade their APP software and Trilinos configuration. (We don't want to cause fire-drills to our customers when we can avoid it.)

Below is just a suggestion; feel free to ignore. (I don't have a horse in this race.)

A short-term (and even medium-term) solution that will allow those existing APPs to keep working and not break all of the other existing use cases is to set up STK to allow the usage of either this new GTest TPL or the old Gtest package. Some simple if() statements will take care of that. I think it is as simple as updating the file stk/stk_ngp_test/cmake/Dependencies.cmake as:

IF (TPL_ENABLE_GTest)
  SET(Gtest_NAME)
  SET(GTest_NAME GTest)
ELSE()
  SET(Gtest_NAME Gtest)
  SET(GTest_NAME)
ENDIF()

SET(LIB_REQUIRED_DEP_PACKAGES ${Gtest_NAME} Kokkos KokkosCore KokkosContainers)
SET(LIB_OPTIONAL_DEP_PACKAGES)
SET(TEST_REQUIRED_DEP_PACKAGES)
SET(TEST_OPTIONAL_DEP_PACKAGES)
SET(LIB_REQUIRED_DEP_TPLS ${GTest_NAME})
SET(LIB_OPTIONAL_DEP_TPLS)
SET(TEST_REQUIRED_DEP_TPLS)
SET(TEST_OPTIONAL_DEP_TPLS)

and the other STK Dependencies.cmake files as:

IF (TPL_ENABLE_GTest)
  SET(Gtest_NAME)
  SET(GTest_NAME GTest)
ELSE()
  SET(Gtest_NAME Gtest)
  SET(GTest_NAME)
ENDIF()

...
SET(TEST_REQUIRED_DEP_PACKAGES ${Gtest_NAME})
...
SET(TEST_REQUIRED_DEP_TPLS ${GTest_NAME})
...

I think the rest of the Trilinos Dependencies.cmake files can stay as they are for now and just depend uncontinionally on the internal Gtest package since there should be no interaction between the stk_ngp_test library and the other Trilinos package test executables.

Then customers that want to use STK with the new GTest TPL can configure Trilinos with -DTPL_ENABLE_GTest=ON and get STK with that GTest. But existing customers can keep configuring Trilinos with the old Gtest package for now and keep working. Then, we can work with them calmly to remove their dependence on the old Gtest package and get their TPL installers updated to install the new GTest TPL. That way, we will not be creating a fire-drill for these customers.

[ibaned]

@bartlettroscoe that seems like it would work for me and for backward compatibility as well. I'm fine going in the if() direction but I don't know when I'll have time to modify this PR. Would you have any time to implement that solution?

[bartlettroscoe]

@bartlettroscoe that seems like it would work for me and for backward compatibility as well. I'm fine going in the if() direction but I don't know when I'll have time to modify this PR. Would you have any time to implement that solution?

@ibaned, it is basically the same solution you already have in PR #9514 except you keep the Gtest package and skip the changes to all of the other packages other than STK. Given that this impacts several different Trilinos customers, this seems like something the @trilinos/framework would need to coordinate.

[tasmith4]

We had a discussion on the STK team about the best path forward and we think it's best for STK to shift from using GTest as a package to using it as a TPL.

@ibaned can you do another, smaller PR that allows GTest as a TPL through TriBITS (or fixes the name or whatever is required?)

Then we will do something like what @bartlettroscoe shows to enable use of GTest as a TPL and deprecate usage of GTest as a package, eventually deleting the STK dependence on the GTest package.

[bartlettroscoe]

And installing gtest along with Trilinos and having customers use it from Trilinos is causing problems with the move to modern CMake (and note that yaml-cpp is doing the same thing as per jbeder/yaml-cpp#539). See TriBITSPub/TriBITS#443 (comment) for details.

No package should be installing gtest as part its install. This just sets you for problems.

[bartlettroscoe]

In fact, it was the last unresolved issue that stopped SPARC and EMPIRE from being able to use the exact same binary installs of Trilinos because SPARC needed the install of Gtest and EMPIRE broke if it was installed.

This is coming back to haunt even SPARC now as it tries to pull in its own version of gtest. But in the case of EMPIRE, if they would have put the include path for their GTest in front of the include path for Trilinos, they it would have been selected over the one installed in Trilinos. This what SPARC is going to have to do. I think that is the lesion for packages like GTest that everyone seems to want to install as part of their package; put the -I include for your own copy of GTest first on the compile line for your unit tests. Don't leave it to chance what version of Gtest gets found. (Same for any other such library you may suck in and then use internally.)

It turns out that modern CMake with imported targets prior to CMake 3.23 (which is not even released yet) did not provide enough fine-grained control to do this properly but after CMake 3.23 and with a little property hacking, a CMake project should have full nearly control over the include directories it uses, even from IMPORTED targets. (But those tweaks are not well known.)

[bartlettroscoe]

FYI: Looks like SPARC is very close to sucking in its own GTest. I helped them fix some include dirs issues that required a small patch to gtest to build it internally which was:

commit 3bd8673c4919eefb6eb4cc1fec53243d1d253e90
Author: Roscoe A. Bartlett <rabartl@sandia.gov>
Date:   Mon Feb 7 14:03:28 2022 -0700

    Remove 'SYSTEM' from gtest include dirs (SPAR-614)
    
    This is needed to allow this version of gtest to come before all other gtests
    that might be out there.
    
    I don't understand why a project would set SYSTEM for its own include
    directories?  CMake needs to let client CMake projects control include
    directories without having to patch the CMakeLists.txt files of these imported
    packages.

diff --git a/tests/googletest/googlemock/CMakeLists.txt b/tests/googletest/googlemock/CMakeLists.txt
index e7df8ec53..6f2809f33 100644
--- a/tests/googletest/googlemock/CMakeLists.txt
+++ b/tests/googletest/googlemock/CMakeLists.txt
@@ -109,10 +109,10 @@ endif()
 # to the targets for when we are part of a parent build (ie being pulled
 # in via add_subdirectory() rather than being a standalone build).
 if (DEFINED CMAKE_VERSION AND NOT "${CMAKE_VERSION}" VERSION_LESS "2.8.11")
-  target_include_directories(gmock SYSTEM INTERFACE
+  target_include_directories(gmock INTERFACE
     "$<BUILD_INTERFACE:${gmock_build_include_dirs}>"
     "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>")
-  target_include_directories(gmock_main SYSTEM INTERFACE
+  target_include_directories(gmock_main INTERFACE
     "$<BUILD_INTERFACE:${gmock_build_include_dirs}>"
     "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>")
 endif()
diff --git a/tests/googletest/googletest/CMakeLists.txt b/tests/googletest/googletest/CMakeLists.txt
index abdd98b79..ec8318095 100644
--- a/tests/googletest/googletest/CMakeLists.txt
+++ b/tests/googletest/googletest/CMakeLists.txt
@@ -136,10 +136,10 @@ set_target_properties(gtest_main PROPERTIES VERSION ${GOOGLETEST_VERSION})
 # to the targets for when we are part of a parent build (ie being pulled
 # in via add_subdirectory() rather than being a standalone build).
 if (DEFINED CMAKE_VERSION AND NOT "${CMAKE_VERSION}" VERSION_LESS "2.8.11")
-  target_include_directories(gtest SYSTEM INTERFACE
+  target_include_directories(gtest INTERFACE
     "$<BUILD_INTERFACE:${gtest_build_include_dirs}>"
     "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>")
-  target_include_directories(gtest_main SYSTEM INTERFACE
+  target_include_directories(gtest_main INTERFACE
     "$<BUILD_INTERFACE:${gtest_build_include_dirs}>"
     "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>")
 endif()

And to make sure that their gtest headers are selected (i.e. by moving their include dirs forward on the compile line and list them with -I instead of -isystem), the customer CMake proejct can call include_directories(BEFORE )`).

And with that, a customer CMake project can suck in GTest with add_subdirectory(googletest) and have the include dirs for this GTest to come before any other version of GTest installed that may be found in the rest of the searched include directories by the compiler (again, see TriBITSPub/TriBITS#443).

[bartlettroscoe]

FYI: I verified that the internal build of googletest (gtest) with SPARC does not appear to have broken any of their nighty builds. See internal issue tribitsrefactoring#3.

This should be an example for all packages that may have similar problems and need to be building with their own gtest and need to make sure that their gtest headers are selected (i.e. by moving their include dirs forward on the compile line by calling include_directories(BEFORE <list-of-gtest-include-dirs>)).

[bartlettroscoe]

Just got the below email from @e10harvey. I will respond in the next comment.

---

Hi Ross,

Both Kokkos and KokkosKernels currently have a snapshot of GTEST under version control in their repositories.

After running it by Kokkos and KokkosKernels, I would like to do the following:

1. Remove the snapshots of GTEST from both Kokkos.git and KokkosKernels.git
2. Request that Kokkos Core CMake use find_package for gtest
   a. KokkosKernels can add a find_package too but it depends on Core and could use Core’s GTEST instead.
3. Request gtest modules on machines that Kokkos cares about

What are your thoughts on this approach? Is this approach “better” than continuing to snapshot GTEST? Would this conflict with how Trilinos uses GTEST?

[bartlettroscoe]

After running it by Kokkos and KokkosKernels, I would like to do the following:

@e10harvey, I think that individual packages should be able to do what they want with test-only dependencies like GTest (googletest). If they want to rely on an external installation (that is found through find_package()) then that is fine. If they want to snapshot GTest to only be used by their test suite, that is fine too.

As long as only the tests for Kokkos and KokkosKernels need GTest, then I don't know why anyone should care.

Other opinions?

[jhux2]

There is some discussion in #9514, as well.

[ibaned]

I've brought this up before as well. I think the Kokkos team was not receptive to this at the time, but I support asking them again. I agree with Ross that if it is truly a test-only dependency then having a copy-pasted version doesn't hurt downstream packages (although it is still bad software design). However, there is a GTest version in Trilinos that isn't always test-only (an STK package can cause it to be installed). This does hurt our downstream packages and needs to be fixed.

[bartlettroscoe]

Thinking more about this, we can't have a top-level TriBITS TPL for GTest and still allow individual packages to snapshot and use their own version of GTest. The problem is that, with the new TriBITS, if you define a top-level TriBITS TPL called GTest that calls find_package(GTest), then that will define base project-level imported targets that may clash with targets that are generated by a package's local build of GTest.

Therefore, I think we need to eliminate the TriBITS GTest TPL and eliminate the Trilinos GTest package and just make individual Trilinos packages either snapshot and build their own version of GTest (like Sacado does) or they should call find_package(GTest) scoped within their package's CMakeListxs.txt file (or below). That way, each package can make their own decision on how to handle GTest and what versions they are compatible with. And indeed, two Trilinos packages could require two different GTest versions that are installed out on the system and that should work (because the targets generated by the find_package(GTest) are scoped within the Trilinos package that calls them).

And yes, we can't have any regular package libraries for any Trilinos package depend on GTest. That is a big no no. (So STK would need to be refactored to remove such a dependency or one needs to be very careful how STK libraries that depend on GTest are handled as not to clash with downstream packages. Such STK libraries could be marked as TESTONLY and INSTALLABLE for example.)

Thoughts?

[e10harvey]

I like the idea of having individual packages handle their own dependencies in one way. I think Kokkos has to maintain some extra cmake right now to build in Trilinos. How would this decision affect how TriBITS handles other TPLs such as BLAS and CUDA? Would TPL symbols across packages conflict at Trilinos link time?

[alanw0]

We would be fine with cleaning up STK's dependence on Gtest. It looks like most of STK's gtest dependencies use 'TEST_REQUIRED_DEP_PACKAGES Gtest'. Is that ok? Just one says 'LIB_REQURED_...'. In any case we're fine with doing whatever refactoring/restructuring is needed for this change to work.
In principle, stk libraries should not depend on Gtest, except for a couple that are specifically intended to be unit-test-utilities, and marking those as TESTONLY would probably be fine.
(Although I have a faint memory that TESTONLY prevents a target from being exported, and we have a couple cases where the exawind project has unit-tests that use stk unit-test-utilities... What a tangled web we weave.)

[ibaned]

I'm strongly in favor of the approach outlined by Ross to remove the TriBITS GTest TPL and package and allow per-package solutions. The STK package that causes problems is stk_unit_test_utils because I think it builds functionality on top of GTest that then gets used by Sierra. I recommend that STK package use find_package(GTest) internally and Sierra set up their builds to install GTest before Trilinos, use the installed GTest in stk_unit_test_utils and also find the same installed GTest inside Sierra.

[alanw0]

@tasmith4 what do you think of Dan's last comment? I think it sounds fine for Sierra but not so sure about exawind. (Sierra doesn't build stk with cmake so it's a non-issue.) Exawind depends on stk_unit_test_utils, so that needs to be an exported target. If we can accomplish that, then this approach sounds fine.
The other question for @bartlettroscoe or @ibaned, is: if these changes are made to trilinos' management of gtest, presumably gtest would still be available for trilinos PR testing?

[tasmith4]

@alanw0 off the top of my head, I think if we and nalu-wind both use find_package(gtest), and if the exawind stack includes gtest built as a TPL available to both, then we should be able to export stk_unit_test_utils without issue.

And yeah, Sierra builds are handled through the bake/jamfile system so the gtest dependency is captured in a completely different way and won't affect this discussion.

[ibaned]

@alanw0 we've definitely made sure that SEMS has a GTest package and if installs are needed anywhere else to support Trilinos testing I'm happy to request it through SEMS and ASC DevOps.

[bartlettroscoe]

In principle, stk libraries should not depend on Gtest, except for a couple that are specifically intended to be unit-test-utilities, and marking those as TESTONLY would probably be fine.
(Although I have a faint memory that TESTONLY prevents a target from being exported, and we have a couple cases where the exawind project has unit-tests that use stk unit-test-utilities... What a tangled web we weave.)

@alanw0, I can help with dealing with these test libraries.

[bartlettroscoe]

How would this decision affect how TriBITS handles other TPLs such as BLAS and CUDA? Would TPL symbols across packages conflict at Trilinos link time?

@e10harvey, no, for external packages/TPLs that are used by the libraries of a TriBITS package (and therefore propagate downstream), they need to be handled as TriBITS TPLs to ensure there is just one definition across all packages that may have that same TPL dependency. GTest and other external dependencies that only impact the tests and examples for a package are a different situation. They don't propagate anywhere (unless a downstream package specifically uses a test-only library from an upstream package but that is specific type of dependency for which the downstream package takes on with its eyes wide open).

[bartlettroscoe]

(Sierra doesn't build stk with cmake so it's a non-issue.) Exawind depends on stk_unit_test_utils, so that needs to be an exported target. If we can accomplish that, then this approach sounds fine.

@alanw0, it may require a TriBITS extension but I think that we could support this as a TESTONLY and INSTALLABLE library. This would cause the target to get exported to the <Package>Config.cmake file but it would not be included in the <Package>::all_libs target and therefore downstream packages or external CMake projects would not pull in these libraries by default.

One issue is that I think having a external CMake package dependency not handled as a TriBITS TPL would require adding some custom code to the generated <Package>Config.cmake file to call find_dependency(GTest). (And this will require more TriBITS examples, documentation, and tests to support this use case.) Then Exawind would then get this GTest along with stk_unit_test_utils automatically and not have to find it (if Sierra was using CMake).

The other question for @bartlettroscoe or @ibaned, is: if these changes are made to trilinos' management of gtest, presumably gtest would still be available for trilinos PR testing?

I think in this case, we would need to have an acceptable version of GTest installed on the system before configuring Trilinos for those Trilinos packages that are using find_package(GTest). But as @ibaned said above, GTest is already installed in the SEMS env. (And we can likely get the ASC DevOps team to install GTest as part of future CDE versions.)

[bartlettroscoe]

FYI: After the merge of TriBITSPub/TriBITS#560 and the sync to Trilinos, it should be pretty easy to build Trilinos with internal GTest package or external GTest TPL (and we should be able to call the TPL and the internal package the same name "GTest").

[tasmith4]

@bartlettroscoe this sounds great, I was just about to circle back to this for STK. Can you keep me in the loop on that sync to Trilinos?

[romintomasetti]

• @bartlettroscoe If I may add on this issue, I think it is indeed better to have a single GTest version as @ibaned said. I don't think GTest should be copy/pasted in Trillinos for the good reasons mentioned above. I think having GTest easily "pickable" by any package would be very nice, because GTest brings tons of features Teuchos unit testing hasn't (thinking e.g. at GMock). And indeed, allowing the user to say "please use my version of GTest" is also nice.

• Still, it seems reasonable to keep that "one version" of GTest in Trilinos such that when any package relying on it for the testing is compiled (and its tests enabled), the life is still easy for the user/developer. However, I would strongly suggest that you use git submodule to "store" GTest. It would make clear to everyone what GTest SHA is used at any time.

• We have developed a very small wrapper around GTest to allow it working with MPI (use Teuchos MPI communicators to make reduction of test results etc). I think having GTest + this wrapper would allow you "deprecating" Teuchos unit test framework in the long term, since GTest+wrapper works exactly as Teuchos unit test w.r.t. MPI, but brings all the GTest/GMock features.

[romintomasetti]

@ccober6 I would like to bring your attention on this as well for the Trilinos User-Developer Group Meeting 😉

[ccober6]

Yeah, this one has been sitting on the back burner for a while, and has not had a lot of discussion recently.

I will add it to the list. :)

[jhux2]

#11184 may be related.

[bartlettroscoe]

This topic did not make it to the top of the list at the TUG 2023 meeting yesterday. Perhaps this could be discussed at a future Trilinos Developers meeting?

With the updated TriBITS, I think it would be fairly easy to set up Trilinos to be able to build GTest internally (by default), or build against an externally installed GTest. Since GTest is not a quite a TriBITS-Compliant external package when installed, we will need to create a FindTplGTest.cmake module for this and will need to extend TriBITS to support using the FindTplGTest.cmake file when TPL_ENABLE_GTest=ON is set. (NOTE: Currently, the Trilinos TPL is called gtest while the Trilinos package is called Gtest. We will need to resolve that as part of this.)

Any interested customers should contact me by my SNL email and we can discuss how to get this done.