Introduce options to disable Variorum and SystemTap #309
Conversation
| set(KOKKOSTOOLS_HAS_VARIORUM TRUE) | ||
| else() | ||
| message(WARNING "Variorum not found: ${MSG_NOTFOUND}") | ||
| message(FATAL_ERROR "Variorum not found: ${MSG_NOTFOUND}") |
There was a problem hiding this comment.
The message is made FATAL_ERROR because the user can now explicitly disable Variorum if it cannot be found.
I think once a new release of variorum is made, shipping llnl/variorum@c3b620a, we could replace this file by find_package(Variorum REQUIRED).
There was a problem hiding this comment.
Why don't we instead do find_package(Variorum REQUIRED) L19 and drop that if...else statement.
There was a problem hiding this comment.
My intent was to keep the message to help the user to set the expected variables. For example the user could expect that setting CMAKE_PREFIX_PATH works.
There was a problem hiding this comment.
Any strong preference ?
There was a problem hiding this comment.
My intent was to keep the message to help the user to set the expected variables. For example the user could expect that setting
CMAKE_PREFIX_PATHworks.
Why would it not work?
Any strong preference ?
No. That's fine we can rework the find modules later as necessary.
There was a problem hiding this comment.
Why would it not work?
Variorum used to install the CMake files in a non standard directory, CMake could not find them with CMAKE_PREFIX_PATH nor Variorum_ROOT. I suspect this is why we have all the logic about paths in the configure_variorum.cmake. The commit llnl/variorum@c3b620a should fix that but this will be available only in the next release of Variorum.
tpadioleau
force-pushed
the
introduce-variorum-and-systemtap-options
branch
3 times, most recently
from
d0fbb66 to
9a308a7
Compare
dalg24
left a comment
dalg24
left a comment
There was a problem hiding this comment.
Adding options is fine because it is consistent with what we currently do with other dependency.
Longer-term, I would like us to consider simply using the CMake native support for controlling finding packages, CMAKE_DISABLE_FIND_PACKAGE_<PackageName> and CMAKE_REQUIRE_FIND_PACKAGE_<PackageName>, instead of rolling our own.
(Not blocking your PR though as it is a legitimate improvement over the status quo)
| set(KOKKOSTOOLS_HAS_VARIORUM TRUE) | ||
| else() | ||
| message(WARNING "Variorum not found: ${MSG_NOTFOUND}") | ||
| message(FATAL_ERROR "Variorum not found: ${MSG_NOTFOUND}") |
There was a problem hiding this comment.
Why don't we instead do find_package(Variorum REQUIRED) L19 and drop that if...else statement.
A difficulty I see with CMake options is that they only relate to dependencies but not to features of the project. Some features have multiple dependencies, some dependencies belong to different features (like MPI). If we remove all our options and rely only on the predefined variables, I think it might end up be more difficult for the user to find the correct options to set. |
tpadioleau
force-pushed
the
introduce-variorum-and-systemtap-options
branch
from
9a308a7 to
6b11729
Compare
| find_program(KOKKOSTOOLS_DTRACE_EXECUTABLE dtrace) | ||
| if(KOKKOSTOOLS_DTRACE_EXECUTABLE) | ||
| if(KokkosTools_ENABLE_SYSTEMTAP) | ||
| if(NOT WIN32 AND NOT APPLE) |
There was a problem hiding this comment.
I would slightly prefer avoiding the negations
if(WIN32 OR APPLE)
message(FATAL_ERROR ...
else()
find_package(...| set(KOKKOSTOOLS_HAS_VARIORUM TRUE) | ||
| else() | ||
| message(WARNING "Variorum not found: ${MSG_NOTFOUND}") | ||
| message(FATAL_ERROR "Variorum not found: ${MSG_NOTFOUND}") |
There was a problem hiding this comment.
My intent was to keep the message to help the user to set the expected variables. For example the user could expect that setting
CMAKE_PREFIX_PATHworks.
Why would it not work?
Any strong preference ?
No. That's fine we can rework the find modules later as necessary.
3 participants
The PR adds two CMake options to disable Variorum (
KokkosTools_ENABLE_VARIORUM) and SystemTap (KokkosTools_ENABLE_SYSTEMTAP). This work is related to the issue spack/spack-packages#377 that needs an explicit way to disable the search of Variorum and dtrace.