Skip to content

[rocprofiler-compute] Fix RPATH for rocprofiler-compute test binary in non-default install location#5108

Merged
vedithal-amd merged 1 commit intomainfrom
users/vedithal-amd/rocprofiler-compute-fix-native-tool-tests
May 8, 2026
Merged

[rocprofiler-compute] Fix RPATH for rocprofiler-compute test binary in non-default install location#5108
vedithal-amd merged 1 commit intomainfrom
users/vedithal-amd/rocprofiler-compute-fix-native-tool-tests

Conversation

@vedithal-amd
Copy link
Copy Markdown
Contributor

Motivation

The C++ unit test binary test-rocprofiler-compute-tool (registered with CTest via TEST_FROM_INSTALL=ON) fails on TheRock builds with:

test-rocprofiler-compute-tool: error while loading shared libraries:
librocprofiler-compute-tool.so: cannot open shared object file

The library is installed to lib/rocprofiler-compute/ and the binary to libexec/rocprofiler-compute/tests/. The per-target INSTALL_RPATH set upstream resolves correctly in standalone CMake builds, but TheRock's global post-subproject hook overwrites it with paths computed relative to the default executable origin (bin/), producing an RPATH that does not reach lib/rocprofiler-compute/.

This unblocks the upstream rocprofiler-compute PR ROCm/rocm-systems#5721, which adds CTest registration of the native tool test and surfaces the failure on TheRock CI.

Technical Details

Two changes:

  1. profiler/CMakeLists.txt: add INSTALL_RPATH_DIRS "lib/rocprofiler-compute" to the rocprofiler-compute subproject declaration so the project's own lib subdir is part of the RPATH set TheRock plumbs into every binary in the subproject.

  2. profiler/post_hook_rocprofiler-compute.cmake (new): annotate test-rocprofiler-compute-tool with THEROCK_INSTALL_RPATH_ORIGIN libexec/rocprofiler-compute/tests so TheRock computes RPATH relative to the actual install location. Auto-discovered via the post_hook_<name>.cmake naming convention. Mirrors the pattern in the adjacent profiler/post_hook_rocprofiler-sdk.cmake. Guarded with if(TARGET ...) so it is a no-op when THEROCK_BUILD_TESTING=OFF.

No upstream changes required. The existing INSTALL_RPATH in the rocprofiler-compute project continues to handle standalone builds.

Test Plan

  • TheRock build with THEROCK_BUILD_TESTING=ON: confirm readelf -d on the installed test-rocprofiler-compute-tool shows RUNPATH containing a relative path that resolves to lib/rocprofiler-compute/ from libexec/rocprofiler-compute/tests/.
  • CTest runs test-rocprofiler-compute-tool end-to-end without setting LD_LIBRARY_PATH.
  • TheRock build with THEROCK_BUILD_TESTING=OFF: build still configures cleanly (post-hook guard prevents touching the missing target).

Submission Checklist

…location

test-rocprofiler-compute-tool installs to libexec/rocprofiler-compute/tests/
instead of the default bin/, so TheRock's RPATH machinery (which assumes
executables live in bin/) computes relative paths from the wrong origin and
the loader cannot find librocprofiler-compute-tool.so at runtime.

Add INSTALL_RPATH_DIRS for the project's lib subdir, plus a post hook that
sets THEROCK_INSTALL_RPATH_ORIGIN on the test binary so the relative path is
computed correctly. Mirrors the pattern in the adjacent
post_hook_rocprofiler-sdk.cmake.

Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 7, 2026 19:39
@vedithal-amd vedithal-amd changed the title Fix RPATH for rocprofiler-compute test binary in non-default install location [rocprofiler-compute] Fix RPATH for rocprofiler-compute test binary in non-default install location May 7, 2026
@vedithal-amd vedithal-amd requested a review from ScottTodd May 7, 2026 19:41
@vedithal-amd
Copy link
Copy Markdown
Contributor Author

FYI @svolkov-amd and @feizheng10 for future native collector changes

Copy link
Copy Markdown
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

This PR fixes runtime linking for the installed test-rocprofiler-compute-tool CTest binary when rocprofiler-compute is installed into TheRock’s non-default layout (binary under libexec/... and its dependent .so under lib/...). It does so by ensuring TheRock’s global post-subproject RPATH computation includes the rocprofiler-compute library subdirectory and computes relative RPATH entries from the test binary’s actual install location.

Changes:

  • Add INSTALL_RPATH_DIRS "lib/rocprofiler-compute" to the rocprofiler-compute subproject declaration so TheRock includes that directory when generating install RPATHs.
  • Add a new rocprofiler-compute post-hook that sets THEROCK_INSTALL_RPATH_ORIGIN for test-rocprofiler-compute-tool to libexec/rocprofiler-compute/tests, so relative RPATHs are computed from the correct origin.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
profiler/post_hook_rocprofiler-compute.cmake Sets THEROCK_INSTALL_RPATH_ORIGIN for the installed rocprofiler-compute test executable to ensure correct origin-relative RPATH computation.
profiler/CMakeLists.txt Adds rocprofiler-compute’s lib/rocprofiler-compute directory to TheRock-managed install RPATH dirs for that subproject.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Thanks, this looks correct based on existing patterns. Are there tests that would run on this PR itself that would confirm the fix, or just the separate PR in rocm-systems that will start using this?

Comment on lines +4 to +11
# test-rocprofiler-compute-tool installs to libexec/rocprofiler-compute/tests/
# instead of the default bin/. Tell TheRock the actual origin so it can compute
# correct relative RPATH entries.
if(TARGET test-rocprofiler-compute-tool)
set_target_properties(test-rocprofiler-compute-tool PROPERTIES
THEROCK_INSTALL_RPATH_ORIGIN libexec/rocprofiler-compute/tests
)
endif()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI I ran an audit across the project for other targets that might need similar treatment: https://github.com/ScottTodd/claude-rocm-workspace/blob/main/reviews/pr_TheRock_5108.md#broader-analysis-other-targets-that-may-be-affected

potentially missing:

  • roctracer codeobj_test and test executables
  • aqlprofile test executables
  • rocprofiler (v1), though we don't build it in TheRock and probably don't plan to in the future?

I also checked if we could validate RPATH correctness through unit tests to ensure we don't regress here. See https://github.com/ScottTodd/claude-rocm-workspace/blob/main/reviews/pr_TheRock_5108.md#unit-test-coverage-for-rpath-correctness for a ideas there

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Filed #5114 to follow up so we don't regress and other subprojects getting onboarded can have more confidence that the packaging will work

@vedithal-amd
Copy link
Copy Markdown
Contributor Author

Thanks, this looks correct based on existing patterns. Are there tests that would run on this PR itself that would confirm the fix, or just the separate PR in rocm-systems that will start using this?

The test-rocprofiler-compute-tool PR merge into rocm-systems is blocked by this.
So, once we merge this PR - TheRock CI on that PR will tell whether the fix worked.

@vedithal-amd
Copy link
Copy Markdown
Contributor Author

Its allowing me to merge even when things are red, can i go ahead ?

The rocprofiler-compute test error is due to flaky test_L1_cache_counter metric validation test which i just disabled in develop to fix it later, so that is not related to this PR

image

@ScottTodd
Copy link
Copy Markdown
Member

If the builds and tests you expect to pass do pass then yes, safe to merge.

@vedithal-amd vedithal-amd merged commit fac1317 into main May 8, 2026
122 of 138 checks passed
@vedithal-amd vedithal-amd deleted the users/vedithal-amd/rocprofiler-compute-fix-native-tool-tests branch May 8, 2026 03:50
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants