Skip to content

Conversation

@tpadioleau
Copy link
Contributor

@tpadioleau tpadioleau commented Oct 3, 2025

The PR disables the optional find_package(Variorum QUIET) call, The PR disables the Variorum and SystemTap cmake options, this should help fix #377. These options can be later defined as Spack variants with associated dependencies.

@jennfshr

@tldahlgren
Copy link
Contributor

Manually added missing maintainers to Reviewers list and, when couldn't, to the description.

Copy link
Contributor

@vlkale vlkale left a comment

Choose a reason for hiding this comment

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

This is fine from my end. Though Variorium provides many rich capabilities w.r.t. to energy profiling, I don't think Variorium is critical to the core functionality of Kokkos Tools. I agree this should help fix the problem in #377.

@rbberger
Copy link
Member

rbberger commented Oct 6, 2025

@vlkale Just for understanding, why isn't there a KokkosTools_ENABLE_VARIORIUM?

@vlkale
Copy link
Contributor

vlkale commented Oct 8, 2025

@vlkale Just for understanding, why isn't there a KokkosTools_ENABLE_VARIORIUM?

This should be added and I think is low hanging fruit. @tpatki @ethan-puyaubreau
@slabasan

Do you see any obstacles putting it into the Kokkos Tools CMakeLists.txt? Is this embedded in one of Ethan's recent Kokkos Tools draft PRs for Variorium?

@rbberger
Copy link
Member

rbberger commented Oct 8, 2025

@vlkale if it can get into develop I would prefer that over adding a workaround here and then later removing it again. Specifically since the only kokkos-tools version right now is develop.

@tpadioleau
Copy link
Contributor Author

tpadioleau commented Oct 13, 2025

@vlkale if it can get into develop I would prefer that over adding a workaround here and then later removing it again. Specifically since the only kokkos-tools version right now is develop.

I drafted a PR kokkos/kokkos-tools#309, actually even if we merge #1844, we will still have the issue of dtrace.

@vlkale
Copy link
Contributor

vlkale commented Oct 15, 2025

@vlkale if it can get into develop I would prefer that over adding a workaround here and then later removing it again. Specifically since the only kokkos-tools version right now is develop.

I drafted a PR kokkos/kokkos-tools#309, actually even if we merge #1844, we will still have the issue of dtrace.

Thank you! I think you have gotten good comments there and it looks like that PR should fix this. I'm also noting PR #310 for making BUILD_SHARED_LIBS a CACHE variable.

@tpadioleau tpadioleau changed the title kokkos-tools: Disable optional Variorum find_package kokkos-tools: Disable Variorum and SystemTap cmake options Oct 16, 2025
@tpadioleau tpadioleau changed the title kokkos-tools: Disable Variorum and SystemTap cmake options kokkos-tools: Disable Variorum and SystemTap CMake options Oct 16, 2025
@tpadioleau
Copy link
Contributor Author

tpadioleau commented Oct 16, 2025

Thank you! I think you have gotten good comments there and it looks like that PR should fix this. I'm also noting PR #310 for making BUILD_SHARED_LIBS a CACHE variable.

The PR kokkos/kokkos-tools#309 has been merged, I have updated this PR accordingly.

rbberger
rbberger previously approved these changes Oct 16, 2025
@rbberger rbberger enabled auto-merge (squash) October 16, 2025 06:38
@rbberger rbberger disabled auto-merge October 16, 2025 06:38
@rbberger rbberger enabled auto-merge (squash) October 16, 2025 06:38
@vlkale vlkale requested a review from Copilot October 16, 2025 12:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Disable selected optional KokkosTools CMake options by default in the Spack package to avoid unintended dependencies (e.g., Variorum, SystemTap), aligning with the plan to introduce them later as variants.

  • Add explicit CMake defines to OFF for APEX, CALIPER, SYSTEMTAP, VARIORUM, EXAMPLES, SINGLE, and TESTS
  • Keep MPI and PAPI toggled via existing variants

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

auto-merge was automatically disabled October 16, 2025 12:57

Head branch was pushed to by a user without write access

@github-actions github-actions bot requested a review from rbberger October 16, 2025 12:58
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Based on comment threads it looks like this PR is now ready to be merged. If not, let me know.

@tldahlgren tldahlgren enabled auto-merge (squash) October 16, 2025 18:30
@tldahlgren tldahlgren merged commit 668504e into spack:develop Oct 16, 2025
17 checks passed
@tpadioleau tpadioleau deleted the kokkos-tools-update branch October 17, 2025 04:02
mkrack pushed a commit to mkrack/spack-packages that referenced this pull request Dec 23, 2025
* Disable optional Variorum find_package, explicitly disable all other options

* Use latest cmake options of Kokkos Tools

* Comment on disabled plugins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installation issue: kokkos-tools

4 participants