Framework: Make GTest only a TPL #9514
Conversation
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Using Repos:
Pull Request Author: ibaned |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 5203 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 2730 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 3210 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 10471 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105 # 1883 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105_uvm_off # 880 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 3247 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_python_3 # 5748 (click to expand) |
|
@ibaned I agree in principle that GTest is more appropriate as a TPL than as a package. One concern from the STK side, is that we develop all STK code against Sierra's GTest TPL, which is locked in at the older version currently snapshotted into Trilinos. It's possible that with updates to GTest, a code that attempts to build STK against the latest GTest will fail to build if features have been deprecated and/or removed (especially given GTest's current "live at head" philosophy). Worse, it's unlikely to be obvious to the user that the solution is in fact to roll their GTest back to v1.10.0 (or whatever the Sierra GTest happens to be at the time), and it's not clear to me how to effectively communicate that workaround (assuming that this rollback would even work for them in the first place -- possibly they're not in control of their TPL installs, or possibly they have code that depends on the same new feature that breaks STK). I'm open to hearing your thoughts and suggested solutions for this problem, as I do agree it makes much more sense for GTest to be a TPL. Also, can you share which STK package fails to build when GTest is not available? Our intent with our build system was that no production code packages should rely on GTest, so that if GTest is missing, the tests are the only thing that won't build. |
|
Thanks for looking at this @tasmith4 ! The package I was referring to is As for the version mismatch concern, I built the STK packages that use GTest with the latest version from Google today and didn't run into any issues. I wouldn't make decisions based on a fear of issues happening in the future since it is such a popular package and seems to be very committed to backward compatibility. |
|
Just for my own records, here is a list of packages whose dependency files were changed:
|
|
@ibaned Do we also need to remove Kokkos internal gtest ? I see that the current PR does not include this but the motivation sounds like to do this as well. https://github.com/trilinos/Trilinos/tree/master/packages/kokkos/tpls/gtest/gtest
|
|
@lxmota for MiniTensor |
|
@kyungjoo-kim in principle I think this would be good to do, however it increases the political difficulty of this change (a lot), and my personal issue will be resolved without removing them. |
|
@ibaned - thanks for doing this! We've needed gtest sorted out for a while. I have a question. If gtest becomes a pure TPL, could we be at risk of losing testing without knowing? Right now if I configure with Trilinos_ENABLE_TESTS=ON, those packages will automatically enable testing. If gtest is a TPL only and the TPL is not installed/enabled on a platform, will all testing for these packages be silently disabled due to missing TPL dependencies? If the configure fails saying that it can't enable testing, that's fine. But if we expect all testing enabled yet silently lose testing for packages relying on gtest, that could be very bad, giving us false positives. Can someone verify the behavior? Also, @bartlettroscoe is currently doing a refactor of TriBITS to treat packages and TPLs using the same infrastructure. This means in the future we can redirect any trilinos package in the dependency chain to an installed TPL version. In this case, we could leave a copy of gtest internally as long as we can always redirect to a TPL. Not sure how far out this capability is. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Using Repos:
Pull Request Author: ibaned |
|
The Kokkos gtest never gets installed, so you shouldn't be able to see it externally. |
ibaned
added
the
AT: WIP
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 5209 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 2736 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 3216 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 10477 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105 # 1889 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105_uvm_off # 886 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 3255 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_python_3 # 5754 (click to expand) |
|
@rppawlo I just confirmed that if you don't have GTest then CMake configuration will fail. This is because GTest is a required test dependency for these packages, so TriBITS will fail to configure if tests are enabled for the package and GTest is not present. As for the future TriBITS TPL/Package equivalence, I don't think it's as applicable once you realize that GTest should never appear in Trilinos in the form of a package, so since it only ever needs to be a TPL it won't benefit from being able to be a TPL or a package. |
The benefit is one less package to download and install to build trilinos, along with avoiding having to figure out a correct version. Not a big deal. I'm fine with removing. Just need to make sure sems/atdm have a gtest module available on all our testing platforms (PR, nightly and ATDM machines). NOTE: ROL, STK, Percept and SEACAS are all snapshotted into trilinos, so these changes could be wiped unless they are backported to the external repos as well. |
|
@ibaned thanks, since you're pretty confident GTest is committed to backwards compatibility I think it makes sense to go ahead with this change and not worry about compatibility issues unless they arise. Since |
|
@tasmith4 sounds good! the reason we're hitting the STK utils issue is that we're building another piece of Sierra (NGS (Morph)) against a build of Trilinos and it wants to use those STK utilities for its own unit tests. |
|
@ibaned SEACAS changes look good. I've made those changes in my SEACAS/github repository also, so they will not get overwritten on the next seacas snapshot. Thanks for doing this. |
|
Thank you @gsjaardema ! |
|
@wadeburgess confirmed that SEMS will ship GTest as a TPL module with their |
|
@glhenni - removing gtest from trilinos means the CDE environment will need to add support for gtest. |
We don't use gtest in charon but gemma might. I'll have to look into it. Is it required by STK and that's the issue? |
|
@rppawlo @nmhamster the SEMS request process has room to request testbed installs, so if you can provide a list of relevant testbeds I can comment on them in the request. @glhenni GTest is not required to just install and use Trilinos, but it will be required to build Trilinos tests and run its tests to confirm it is working correctly. This is typically not something that applications like Gemma need to do, but if you do then this change is relevant. |
|
@ibaned - please provide the list of testbeds and what versions you want installed. |
|
It might be faster to list the machines we don't need it on :) Start with all the testbeds that the trilinos atdm builds cover. That would be all of the builds in the Clean, Nightly, ATDM, PrimaryATDM, SecondaryATDM, and SparcATDM tracks listed on the trilinos cdash site: https://testing.sandia.gov/cdash/index.php?project=Trilinos&display=project Also, probably need the new machines supporting HIP (and SYCL?). This PR will need to be updated by framework team to switch the builds scripts to load the new modules and add the TPL paths. @trilinos/framework @jwillenbring - what other machines do we have testing on? |
|
Some testbeds to include:
|
|
I think this will break SPARC. Better check with @sebrowne. |
|
FYI: I don't have access to most of the ATDM systems anymore. The maintenance of all of this has been handed over to the @trilinos/framework team. |
|
You can stop the install of gtest in Trilinos by setting: -DGtest_SKIP_INSTALL:BOOL=TRUE If you look in the CMakeCache.txt file, you will see that var defined there. Perhaps we should just make that default to TRUE instead of FALSE? (NOTE, the default is FALSE). Note that you will break SPARC if you pull GTest out of Trilinos. (If someone can help them to use their own copy of gtest, then this problem goes away.) As far as I know, there is no reason to remove gtest from Trilinos just because you don’t want to install it. It is harmless to have a copy of gtest in Trilinos since it is only used to test Trilinos packages. It never gets used by any libraries in Trilinos that get installed (as far as I know). That, for example, is how STK packages are enabled, tested, and installed in Trilinos but EMPIRE can still use them even though EMPIRE has its own version of GTest. |
|
Okay, going over all of this PR I see the problem is with the subpackage
I am getting closer to this (you can track this in TriBITSPub/TriBITS#367). My idea for TPLs like this that you always need with Trilinos is to provide a separate optional git repo (called something like TrilinosCoreTPLs) that you can clone under Trilinos and then choose to build these (or any subset of these) as TriBITS packages along with Trilinos. Otherwise, you need to point to existing installs of all of these as already installed external packages (i.e. TPLs). That gives you the best of both worlds without having to resort to the full complexity of something like Spack. |
This reverts commit a2a2aaf.
it is too old. this change means Sacado tests don't run unless we use the GTest TPL
Conflicts: packages/stk/stk_balance/cmake/Dependencies.cmake packages/stk/stk_io/cmake/Dependencies.cmake packages/stk/stk_search/cmake/Dependencies.cmake packages/stk/stk_tools/cmake/Dependencies.cmake packages/stk/stk_unit_test_utils/cmake/Dependencies.cmake packages/stk/stk_util/cmake/Dependencies.cmake
|
This PR now has the fallback-to-the-package approach implemented. One important difference from before is that the Sacado copy of gtest is still gone, so Sacado needs to enable the TPL in order to build its tests. I did confirm that Sacado's tests build and run when the TPL is enabled. |
ibaned
removed
the
AT: WIP
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Using Repos:
Pull Request Author: ibaned |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 5312 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 2842 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 3322 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 10574 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105 # 1986 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105_uvm_off # 983 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 3359 (click to expand)Console Output (last 100 lines) : python-3 # 32 (click to expand) |
ibaned
added
the
AT: WIP
|
This Pull Request has been marked for closure due to inactivity. |
github-actions
bot
added
the
MARKED_FOR_CLOSURE
jhux2
added
DO_NOT_AUTOCLOSE
@trilinos/framework
Motivation
This pull request solves the problem that Trilinos includes copy-pasted outdated copies of GTest, which it installs (I know you can disable this but it also disables the install of an STK package we need), which then causes conflicts in a larger application build that obtains GTest as a proper package from its actual repository here on GitHub. It also solves a secondary problem that there are multiple copies of GTest in Trilinos because the first copy-pasted version is too out-of-date.
The problem is solved by removing the Package
Gtestand the TPLgtestand replacing them both with a TPLGTest(the capitalization matches the exported CMake package that the latest version of GTest installs). All packages that depended on either the old package or old TPL now depend on the new TPL. This was tested with the latest version of GTest and packages in Trilinos seemed to build and execute their tests fine. Note that the optional/required nature of the dependency was not changed, so this new TPL is now a required test TPL for several packages.Closes #8001
Closes #4421
Related to #7698
Related to #8235
Stakeholder Feedback
Being solicited here!
Testing
A build of Trilinos was done with most packages that depend on
Gtestorgtestenabled, and tests executed successfully.I've also put in a request for SEMS to maintain a GTest package.