Skip to content

Introduce options to disable Variorum and SystemTap #309

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.

Sign up for GitHub

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

Merged
dalg24 merged 2 commits into from
Oct 15, 2025
Merged

Introduce options to disable Variorum and SystemTap #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd973de
Add Variorum and SystemTap options
tpadioleau Oct 13, 2025
6b11729
Prefer to fail for SystemTap on Windows and macOS platforms
tpadioleau Oct 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ option(KokkosTools_ENABLE_PAPI "Enable PAPI support" OFF)
option(KokkosTools_ENABLE_MPI "Enable MPI support" OFF)
option(KokkosTools_ENABLE_CALIPER "Enable building Caliper library" OFF)
option(KokkosTools_ENABLE_APEX "Enable building Apex library" OFF)
option(KokkosTools_ENABLE_VARIORUM "Enable Variorum support" OFF)
option(KokkosTools_ENABLE_SYSTEMTAP "Enable SystemTap support" OFF)
option(KokkosTools_ENABLE_EXAMPLES "Build examples" OFF)
option(KokkosTools_ENABLE_TESTS "Build tests" OFF)

Expand Down Expand Up @@ -85,7 +87,9 @@ else()
set(KOKKOSTOOLS_HAS_MPI 0)
endif()

include(cmake/configure_variorum.cmake)
if(KokkosTools_ENABLE_VARIORUM)
include(cmake/configure_variorum.cmake)
endif()

set(KOKKOSTOOLS_HAS_CALIPER ${KokkosTools_ENABLE_CALIPER})
set(KOKKOSTOOLS_HAS_NVTX ${Kokkos_ENABLE_CUDA}) # we assume that enabling CUDA for Kokkos program means nvtx should be available
Expand Down Expand Up @@ -157,14 +161,13 @@ if(KokkosTools_ENABLE_PAPI)
add_subdirectory(profiling/papi-connector)
endif()

if(NOT WIN32 AND NOT APPLE)
find_program(KOKKOSTOOLS_DTRACE_EXECUTABLE dtrace)
if(KOKKOSTOOLS_DTRACE_EXECUTABLE)
if(KokkosTools_ENABLE_SYSTEMTAP)
if(NOT WIN32 AND NOT APPLE)
Copy link
Member

@dalg24 dalg24 Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would slightly prefer avoiding the negations

if(WIN32 OR APPLE)
  message(FATAL_ERROR ...
else()
  find_package(...

Copy link
Member Author

@tpadioleau tpadioleau Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

find_program(KOKKOSTOOLS_DTRACE_EXECUTABLE dtrace REQUIRED)
add_subdirectory(profiling/systemtap-connector)
set(KOKKOSTOOLS_HAS_SYSTEMTAP ON)
else()
message(STATUS "Skipping systemtap-connector (dtrace executable wasn't found)")
set(KOKKOSTOOLS_HAS_SYSTEMTAP OFF)
message(FATAL_ERROR "KokkosTools_ENABLE_SYSTEMTAP=ON is not supported on Windows and macOS platforms")
endif()
else()
set(KOKKOSTOOLS_HAS_SYSTEMTAP OFF)
Expand Down
2 changes: 1 addition & 1 deletion cmake/configure_variorum.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ find_package(Variorum QUIET)
if(Variorum_FOUND)
set(KOKKOSTOOLS_HAS_VARIORUM TRUE)
else()
message(WARNING "Variorum not found: ${MSG_NOTFOUND}")
message(FATAL_ERROR "Variorum not found: ${MSG_NOTFOUND}")
Copy link
Member Author

@tpadioleau tpadioleau Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@dalg24 dalg24 Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we instead do find_package(Variorum REQUIRED) L19 and drop that if...else statement.

Copy link
Member Author

@tpadioleau tpadioleau Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tpadioleau tpadioleau Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any strong preference ?

Copy link
Member

@dalg24 dalg24 Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Why would it not work?

Any strong preference ?

No. That's fine we can rework the find modules later as necessary.

Copy link
Member Author

@tpadioleau tpadioleau Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif()