Skip to content

fix(hip-tests): Restore 12_cmake_hip_add_executable sample - #14879

Open
david-salinas wants to merge 2 commits into
developfrom
users/dsalinas/restore-hip-add-executable-sample
Open

david-salinas wants to merge 2 commits into
developfrom
users/dsalinas/restore-hip-add-executable-sample

Conversation

@david-salinas

@david-salinas david-salinas commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

The sample 12_cmake_hip_add_executable was removed in #5708 but the test harness still lists it, causing hip_samples to fail on every nightly across 17 of 21 Linux hosts (ROCM-33839). This restores the sample with an updated CMakeLists.txt that uses the FindHIP.cmake compatibility shim (find_package(HIP MODULE) +
HIP_ADD_EXECUTABLE) instead of the deleted hipconfig-based build, keeping coverage for the legacy FindHIP path that downstream projects still use.

Motivation

PR #5708 removed 12_cmake_hip_add_executable because it relied on hipconfig, which was being deprecated. However, the test harness was not updated to skip the sample, so every nightly build fails with exit code 127 (CMakeLists.txt not found). Additionally, no other sample exercises the FindHIP.cmake compatibility shim —
restoring this sample is the only coverage for the find_package(HIP MODULE) / HIP_ADD_EXECUTABLE legacy path.

Technical Details

  • Restored MatrixTranspose.cpp verbatim from pre-Remove uses of hipconfig: derive HIP version from find_package(hip) #5708 (same kernel used across other Cookbook samples)
  • Rewrote CMakeLists.txt to use the current FindHIP.cmake shim: find_package(HIP REQUIRED) + hip_add_executable(), which internally delegates to add_executable() + target_link_libraries(hip::device) — no hipconfig or hipcc dependency
  • Re-added add_subdirectory(12_cmake_hip_add_executable) to the parent 2_Cookbook/CMakeLists.txt

Issue Tracking

JIRA ID: ROCM-33839

Test Plan

  • CMake configure succeeds standalone with --rocm-path and amdclang++
  • Full build and runtime verification via CI hip_samples suite (the same suite that currently fails)

Test Result

  • CMake configure: PASS (standalone, ROCm 7.2 local)
  • Full compile requires ROCm 10.3+ where the new FindHIP.cmake shim is installed — CI will validate

Submission Checklist

The sample was removed in #5708 but the test harness still lists it,
causing hip_samples to fail on every nightly (ROCM-33839). Restore the
sample with an updated CMakeLists.txt that uses the FindHIP.cmake
compatibility shim (find_package(HIP MODULE) + HIP_ADD_EXECUTABLE)
instead of the deleted hipconfig-based build. This keeps coverage for
the legacy FindHIP path that downstream projects still use.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/hip-tests/samples/2_Cookbook/12_cmake_hip_add_executable/MatrixTranspose.cpp; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

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

🙋 Wish to Override Policy?

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

🤖 AI-assisted pre-review (for @mangupta)

- Use find_package(HIP MODULE REQUIRED) to explicitly request the
  FindHIP.cmake module path instead of relying on implicit resolution
- Add checkHipErrors(hipGetLastError()) after kernel launch to catch
  async launch errors
- Return EXIT_FAILURE/EXIT_SUCCESS instead of raw error count

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mangupta

mangupta commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (c5323f37be5a) — no new findings at or above the posting threshold.

This branch has not been deployed

No deployments
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.

2 participants