search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.18k stars 559 forks source link

Address providing compilers in TrilinosConfig.cmake and <Package>Config.cmake files #12306

Open bartlettroscoe opened 10 months ago

bartlettroscoe commented 10 months ago

CC: @sebrowne, @ccober6, @jwillenbring, @rppawlo, @jhux2

Description

The TriBITS-generated TrilinosConfig.cmake and <Package>Config.cmake files have long included variables that gave the full paths to the different compilers used to build and install Trilinos as well as the complier options as the variables:

set(<prefix>_CMAKE_C_COMPILER ...)
set(<prefix>_CMAKE_CXX_COMPILER ...)
set(<prefix>_CMAKE_Fortaran_COMPILER ...)

set(<prefix>_CMAKE_C_FLAGS ...)
set(<prefix>_CMAKE_CXX_FLAGS ...)
set(<prefix>_CMAKE_Fortaran_FLAGS ...)

This allowed downstream projects like Albany to call find_package(Trilinos) and then use the compilers defined by that call. However, this does not work when using GNUInstallDirs.cmake (see https://github.com/sandialabs/Albany/issues/982) or when using a CUDA build with Kokkos (see https://github.com/trilinos/Trilinos/pull/11863#issuecomment-1540403871 and https://github.com/TriBITSPub/TriBITS/issues/545).

Therefore, to avoid these problems when moving to modern CMake (see https://github.com/TriBITSPub/TriBITS/issues/411), these compiler-related variables should be removed from these files.

However, if it is desired to have a way to provide these compilers and compiler options to downstream, another solution is suggested in Proposed Solution 2 in:

bartlettroscoe commented 10 months ago

@sebrowne, @ccober6, @jwillenbring, with the merge of:

it has just been discovered that this change that will result in libs and <Pacakg>Config.cmake files being installed under <prefix>/lib64/ on most Linux systems will break the usage of Trilinos for downstream CMake projects that are getting the compilers from Trilinos when calling find_package(Trilinos). See:

I think that we should ASAP update the Trilinos RELEASE_NOTES to state that Trilinos 15.0+ will no longer support providing the compilers in the installed <Package>Config.cmake files. Then we can rip out support for that from Trilinos and TriBITS (see https://github.com/TriBITSPub/TriBITS/issues/545).

Note that when doing a CUDA build, you have to define the compilers first before calling find_package(Kokkos) or find_package(Trilinos) (see https://github.com/trilinos/Trilinos/issues/11955#issuecomment-1584933666).

With Trilinos 15.0 this is a chance to officially remove this functionality with explanation!

Should I put in a PR that makes this change and drop support for putting the compilers in the <Package>Config.cmake files from TriBITS? Should we pull the trigger on:

and either remove support for providing compilers altogether or provide them by a separate call to find_package(TrilinosEnv)?

This seems like the best time to address this (or back out the change to use GNUInstallDirs until Trilinos 16.0).

bartlettroscoe commented 10 months ago

@sebrowne, @ccober6, @jwillenbring, @rppawlo, @jhux2,

So given that at least Albany and Nalu still want to be able to get compilers from the Trilinos installation, we may need to determine how many other customers want to keep this functionality. And if they want to keep that functionality, we should consider pulling the trigger on Proposed Solution 2 in:

Someone should bring this up at the next SART meeting.

bartlettroscoe commented 9 months ago

FYI: At the Trilinos Developers meeting yesterday, it was mentioned that this issue was discussed with the SART meeting and that @glhenni specifically asked about this.

@glhenni, if you have a strong opinion on this, please let us know.

glhenni commented 9 months ago

FYI: At the Trilinos Developers meeting yesterday, it was mentioned that this issue was discussed with the SART meeting and that @glhenni specifically asked about this.

@glhenni, if you have a strong opinion on this, please let us know.

I guess I don't see the problem, admittedly probably because I don't have the depth of knowledge required. If a project chooses to use, for example, Trilinos_CXX_COMPILER, but that doesn't always work, like in the case of cuda, then why isn't that burden on the project to figure out? The compiler and associated flags used to build Trilinos seems like pretty useful information to have.

jhux2 commented 9 months ago

@bartlettroscoe This has also affected Sierra. @vbrunini can give more details.

bartlettroscoe commented 9 months ago

@bartlettroscoe This has also affected Sierra. @vbrunini can give more details.

@vbrunini, if I had to guess, this would be related to the <prefix>/lib to <prefix>/lib64 issue with the change to GNUInstlalDirs.cmake. See:

https://github.com/trilinos/Trilinos/blob/55108f75f26e686089cca543fb54afa5fd91665e/RELEASE_NOTES#L18-L47

vbrunini commented 9 months ago

@bartlettroscoe This has also affected Sierra. @vbrunini can give more details.

@vbrunini, if I had to guess, this would be related to the <prefix>/lib to <prefix>/lib64 issue with the change to GNUInstlalDirs.cmake. See:

https://github.com/trilinos/Trilinos/blob/55108f75f26e686089cca543fb54afa5fd91665e/RELEASE_NOTES#L18-L47

Yes it was from the move to <prefix>/lib64, I've got a fix working it's way through our CI now so I don't think there are any significant concerns on our end. Thanks.

bartlettroscoe commented 9 months ago

Yes it was from the move to <prefix>/lib64, I've got a fix working it's way through our CI now so I don't think there are any significant concerns on our end. Thanks.

@vbrunini, just curious, but what fix the Sierra go with?

vbrunini commented 9 months ago

Yes it was from the move to <prefix>/lib64, I've got a fix working it's way through our CI now so I don't think there are any significant concerns on our end. Thanks.

@vbrunini, just curious, but what fix the Sierra go with?

Previously we used a dummy CMakeLists.txt like:

cmake_minimum_required(VERSION 3.20)
project(dummyfinder LANGUAGES)

find_package(Trilinos PATHS {prefix} NO_MODULE NO_DEFAULT_PATH NO_SYSTEM_ENVIRONMENT_PATH)

# Code to extract include paths & library names from cmake targets so that we can pass it on to sierra's non-cmake build system here

and I just added CXX to the list of languages so cmake would pick up that it should look in lib64.

bartlettroscoe commented 9 months ago

@glhenni, if you have a strong opinion on this, please let us know.

I guess I don't see the problem, admittedly probably because I don't have the depth of knowledge required. If a project chooses to use, for example, Trilinos_CXX_COMPILER, but that doesn't always work, like in the case of cuda, then why isn't that burden on the project to figure out?

Right, but what good is a process that does not work for all situations? If you need to do something completely different for CUDA, then what is the value of trying to rely on Trilinos complilers?

The compiler and associated flags used to build Trilinos seems like pretty useful information to have.

Okay, so if that is useful, then the current system breaks when Trilinos is using the standard CMake module GNUInstallDirs.cmake (and installs under <prefix>/lib64) and find_package(Trilinos ...) can't find the installed Trilinos because find_package() will not search under <prefix>/lib64 until the compilers are defined. So the whole darn thing is broken.

The idea with the Proposed Solution 2 in:

provides as small tweak to this process that will work for CUDA and when using GNUInstallDirs.cmake with the TriBITS project (i.e. Trilinos).

Is that worth implementing and supporting if it means that Trilinos customer projects can use the compilers and tools that were used in the installed Trilinos in all cases, even CUDA? That is the question because even before switching to GNUInstallDirs.cmake that was not longer possible due to find_package(CUDAToolkit)! This would fix this and the issue with GNUInstallDirs.cmake.

Make sense? If not, I should explain this at the next SART meeting.

glhenni commented 9 months ago

@rabartl Make sense? If not, I should explain this at the next SART meeting.

I'm okay with Proposed Solution 2.

bartlettroscoe commented 9 months ago

@rabartl Make sense? If not, I should explain this at the next SART meeting.

I'm okay with Proposed Solution 2.

@glhenni, if you want that, then as an important user of Trilinos, you should request that via the Trilinos HelpDesk (an internal SNL site). If the request comes from an important user, then it will carry more weight with the Trilinos prioritization process. The Trilinos framework team does not directly manage issues on GitHub, only on the Trilinos HelpDesk (or at least that was their policy).