Skip to content

[rpp] link half publicly - #13951

Merged
afzpatel merged 3 commits into
developfrom
users/afzpatel/rpp-half
Oct 26, 2026
Merged

afzpatel merged 3 commits into
developfrom
users/afzpatel/rpp-half

Conversation

@afzpatel

@afzpatel afzpatel commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

JIRA ID : ROCM-33202

Motivation

rppdefs.h includes half.hpp, but HALF::half is linked privately, so dependents cannot find the header:
fatal error: 'half.hpp' file not found

@afzpatel
afzpatel requested a review from a team as a code owner October 19, 2026 19:24
@therock-pr-bot

therock-pr-bot Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@LakshmiKumar23 LakshmiKumar23 added the test:rpp Run full tests for rpp label Oct 21, 2026
@LakshmiKumar23
LakshmiKumar23 requested a balanced review from Copilot October 21, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The public dependency and installed-package discovery are consistent, with no unresolved issues identified.

No open issues 🎉

What changed in this PR

Fixes RPP consumers’ missing half.hpp error by exposing HALF as a transitive dependency.

Changes:

  • Links HALF::half publicly while keeping OpenMP private.
  • Installs FindHALF.cmake and resolves HALF before loading exported targets.
File Description
projects/​rpp/​CMakeLists.txt Exposes HALF publicly and installs its discovery module.
projects/​rpp/​cmake_modules/​rpp-config.cmake.in Adds the module search path and discovers HALF for consumers.

Review effort: Balanced

@zacharyvincze zacharyvincze left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Installing FindHALF.cmake alongside RPP makes RPP responsible for finding half in every consumer's build and appends to their CMAKE_MODULE_PATH. This is beyond the scope of RPP's responsibilities, and will likely interfere with other libraries.

Could you describe the environment that ran into this problem? For same-prefix installs, half should already be on the same include path as the rpp install (typically ROCM_PATH/include), and half is a header-only library as far as I'm aware.

@afzpatel

Copy link
Copy Markdown
Contributor Author

Installing FindHALF.cmake alongside RPP makes RPP responsible for finding half in every consumer's build and appends to their CMAKE_MODULE_PATH. This is beyond the scope of RPP's responsibilities, and will likely interfere with other libraries.

Could you describe the environment that ran into this problem? For same-prefix installs, half should already be on the same include path as the rpp install (typically ROCM_PATH/include), and half is a header-only library as far as I'm aware.

This issue was found on Spack where rpp and half are installed to different prefixes.

@zacharyvincze

Copy link
Copy Markdown
Contributor

Installing FindHALF.cmake alongside RPP makes RPP responsible for finding half in every consumer's build and appends to their CMAKE_MODULE_PATH. This is beyond the scope of RPP's responsibilities, and will likely interfere with other libraries.
Could you describe the environment that ran into this problem? For same-prefix installs, half should already be on the same include path as the rpp install (typically ROCM_PATH/include), and half is a header-only library as far as I'm aware.

This issue was found on Spack where rpp and half are installed to different prefixes.

@afzpatel Got it. In that case, the solution for this would have to pass the include path that contains HALF as a public dependency without installing FindHalf.cmake.

Potentially something like this in rpp-config:

 # half is header-only with no CMake package; rppdefs.h includes it.
find_path(@PROJECT_NAME@_HALF_INCLUDE_DIR
  NAMES half/half.hpp half.hpp
  HINTS "${PACKAGE_PREFIX_DIR}/include" "$ENV{ROCM_PATH}/include")
mark_as_advanced(@PROJECT_NAME@_HALF_INCLUDE_DIR)
if(@PROJECT_NAME@_HALF_INCLUDE_DIR)
  set_property(TARGET @PROJECT_NAME@::@PROJECT_NAME@ APPEND PROPERTY
    INTERFACE_INCLUDE_DIRECTORIES "${@PROJECT_NAME@_HALF_INCLUDE_DIR}")
endif()

Optimally, the preferred solution would be to have a FindHALF.cmake already included in ROCm installations. For now though, we'll have to find another way to specify the dependency.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused changes cover build-tree and installed-package include propagation, with no unresolved findings.

No open issues 🎉

Review effort: Balanced

@afzpatel
afzpatel merged commit c1bd80a into develop Oct 26, 2026
88 checks passed
@afzpatel
afzpatel deleted the users/afzpatel/rpp-half branch October 26, 2026 15:41
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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.

4 participants