Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bartlettroscoe opened this issue Sep 21, 2023 · 13 comments
Labels
MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. PA: Framework Issues that fall under the Trilinos Framework Product Area

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Sep 21, 2023

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 sandialabs/Albany#982) or when using a CUDA build with Kokkos (see #11863 (comment) and TriBITSPub/TriBITS#545).

Therefore, to avoid these problems when moving to modern CMake (see TriBITSPub/TriBITS#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 bartlettroscoe added the PA: Framework Issues that fall under the Trilinos Framework Product Area label Sep 21, 2023
@bartlettroscoe
Copy link
Member Author

@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 TriBITSPub/TriBITS#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 #11955 (comment)).

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
Copy link
Member Author

@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 bartlettroscoe changed the title Consider removing compilers from TrilinosConfig.cmake and <Package>Config.cmake files Address providing compilers in TrilinosConfig.cmake and <Package>Config.cmake files Sep 25, 2023
@bartlettroscoe
Copy link
Member Author

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
Copy link
Contributor

glhenni commented Sep 27, 2023

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
Copy link
Member

jhux2 commented Sep 27, 2023

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

@bartlettroscoe
Copy link
Member Author

@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:

Trilinos/RELEASE_NOTES

Lines 18 to 47 in 55108f7

- Change the default for `Trilinos_USE_GNUINSTALLDIRS` from `OFF` to `ON`,
in the goal to move Trilinos and TriBITS to modern CMake.
TriBITS has had the ability to use that paths selected by the standard
CMake module `GNUInstallDirs.cmake` for a long time. But it is turned off
in TriBITS by default and was never turned on in Trilinos, both for the
sake of backward compatibility.
This may break people's existing configurations because it will install
libs in `<prefix>/libs64/` instead of in `<prefix>/libs/` on many systems
(e.g. Linux systems). For example, this will break downstream CMake
projects that call `find_package(Trilinos ...)` before defining the
compilers (e.g. so they can get the compilers from Trilinos). If the
compilers are not defined, `find_package()` will not search
`<prefix>/lib64`. To revert back to using `<prefix>/lib` but still use
`GNUInstallDirs.cmake` for Trilinos, set `-D
CMAKE_INSTALL_LIBDIR:STRING=lib` when configuring Trilinos. To avoid the
`find_package(Trilinos ...)` problem not searching `<prefix>/lib64`,
consider explicitly specifying the compiler to and having the downstream
CMake project define the compilers first with `project(<ProjectName>
COMPILERS C CXX ...)` before calling `find_package(Trilinos ...)`. (That
is, don't try to get the compilers from the installed Trilinos, see
https://github.com/trilinos/Trilinos/issues/12306.)
NOTE: The setting `-D Trilinos_USE_GNUINSTALLDIRS=OFF` is deprecated and
may be removed in the future. (I.e. the usage of `GNUInstallDirs.cmake`
may be hard-coded in the future so please try adjusting to the usage of
`GNUInstallDirs.cmake` by Trilinos.) See
https://github.com/trilinos/Trilinos/issues/12104#issuecomment-1691945033
for additional details and instructions.

@vbrunini
Copy link
Contributor

@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:

Trilinos/RELEASE_NOTES

Lines 18 to 47 in 55108f7

- Change the default for `Trilinos_USE_GNUINSTALLDIRS` from `OFF` to `ON`,
in the goal to move Trilinos and TriBITS to modern CMake.
TriBITS has had the ability to use that paths selected by the standard
CMake module `GNUInstallDirs.cmake` for a long time. But it is turned off
in TriBITS by default and was never turned on in Trilinos, both for the
sake of backward compatibility.
This may break people's existing configurations because it will install
libs in `<prefix>/libs64/` instead of in `<prefix>/libs/` on many systems
(e.g. Linux systems). For example, this will break downstream CMake
projects that call `find_package(Trilinos ...)` before defining the
compilers (e.g. so they can get the compilers from Trilinos). If the
compilers are not defined, `find_package()` will not search
`<prefix>/lib64`. To revert back to using `<prefix>/lib` but still use
`GNUInstallDirs.cmake` for Trilinos, set `-D
CMAKE_INSTALL_LIBDIR:STRING=lib` when configuring Trilinos. To avoid the
`find_package(Trilinos ...)` problem not searching `<prefix>/lib64`,
consider explicitly specifying the compiler to and having the downstream
CMake project define the compilers first with `project(<ProjectName>
COMPILERS C CXX ...)` before calling `find_package(Trilinos ...)`. (That
is, don't try to get the compilers from the installed Trilinos, see
https://github.com/trilinos/Trilinos/issues/12306.)
NOTE: The setting `-D Trilinos_USE_GNUINSTALLDIRS=OFF` is deprecated and
may be removed in the future. (I.e. the usage of `GNUInstallDirs.cmake`
may be hard-coded in the future so please try adjusting to the usage of
`GNUInstallDirs.cmake` by Trilinos.) See
https://github.com/trilinos/Trilinos/issues/12104#issuecomment-1691945033
for additional details and instructions.

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

bartlettroscoe commented Sep 29, 2023

@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
Copy link
Contributor

glhenni commented Oct 3, 2023

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

I'm okay with Proposed Solution 2.

@bartlettroscoe
Copy link
Member Author

@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).

Copy link

github-actions bot commented Oct 5, 2024

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

@github-actions github-actions bot added the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. PA: Framework Issues that fall under the Trilinos Framework Product Area
Projects
Development

No branches or pull requests

4 participants