refactor: duplicated path helpers into common/path.hpp#1249
Merged
habajpai-amd merged 9 commits intodevelopfrom Nov 20, 2025
Merged
refactor: duplicated path helpers into common/path.hpp#1249habajpai-amd merged 9 commits intodevelopfrom
habajpai-amd merged 9 commits intodevelopfrom
Conversation
habajpai-amd
force-pushed
the
users/habajpai-amd/refactor/common-path-utils
branch
2 times, most recently
from
72a120a to
e890973
Compare
adjordje-amd
reviewed
Nov 6, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors duplicated path helper functions and the remove_env function from multiple binaries into centralized utilities in common/path.hpp and common/environment.hpp. The changes eliminate code duplication across rocprof-sys-sample, rocprof-sys-run, rocprof-sys-instrument, rocprof-sys-causal, and core/argparse.cpp.
Key Changes:
- Moved
get_rocprofsys_root(),get_internal_libpath(),get_internal_script_path(), and newget_internal_libdir()functions tocommon/path.hpp - Moved
remove_env()function tocommon/environment.hppwith a globaloriginal_envsvariable - Updated all call sites to use namespace-qualified functions (e.g.,
path::realpath(),path::get_internal_libpath())
Reviewed Changes
Copilot reviewed 3 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| source/lib/common/path.hpp | Added centralized path helper functions with improved get_internal_libpath() that checks both lib and lib64 directories |
| source/lib/common/environment.hpp | Added centralized remove_env() function with global original_envs tracking |
| source/lib/core/argparse.cpp | Updated to use path:: namespace functions and remove_env() from common headers, removed local implementations |
| source/bin/rocprof-sys-sample/rocprof-sys-sample.hpp | Removed function declarations that are now in common headers |
| source/bin/rocprof-sys-sample/impl.cpp | Updated to use centralized path functions, removed local implementations |
| source/bin/rocprof-sys-run/impl.cpp | Updated to use centralized path functions, removed local implementations |
| source/bin/rocprof-sys-instrument/rocprof-sys-instrument.cpp | Updated to use path::get_internal_libdir() and path::realpath() |
| source/bin/rocprof-sys-causal/rocprof-sys-causal.hpp | Removed function declarations that are now in common headers |
| source/bin/rocprof-sys-causal/impl.cpp | Updated to use centralized path functions, removed local implementations |
| source/bin/rocprof-sys-causal/rocprof-sys-causal.cpp | Minor cleanup of unused includes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mradosav-amd
approved these changes
Nov 12, 2025
projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/rocprof-sys-instrument.cpp
Outdated
Show resolved
Hide resolved
habajpai-amd
force-pushed
the
users/habajpai-amd/refactor/common-path-utils
branch
2 times, most recently
from
05dc270 to
f17635d
Compare
dgaliffiAMD
approved these changes
Nov 19, 2025
…mmon/path.hpp for binaries
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
dgaliffiAMD
force-pushed
the
users/habajpai-amd/refactor/common-path-utils
branch
from
f17635d to
2776336
Compare
Contributor
|
Thank you. Rebasing to allow PSDB-test to rerun. |
habajpai-amd
deleted the
users/habajpai-amd/refactor/common-path-utils
branch
systems-assistant bot
pushed a commit
to ROCm/rocprofiler-systems
that referenced
this pull request
Nov 20, 2025
(#1249) * refactor: duplicated path helpers into common/path.hpp * update rocprof-sys-instrument to use shared path utility * Add path::realpath(std::string[, std::string*]) helper function in common/path.hpp for binaries * common: centralize remove_env implementation in environment.hpp * remove unused includes from rocprof-sys binaries and argparse * changing set to unordered_set wherever sorting is not required and additional cleanup * review comment incorporated * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * copilot review for remove_env incorporated [rocm-systems] ROCm/rocm-systems#1249 (commit b09834e)
1 task
habajpai-amd
added a commit
that referenced
this pull request
Dec 17, 2025
habajpai-amd
added a commit
that referenced
this pull request
Dec 18, 2025
* test: add unit tests for common utilities from PR #1249 * incorporate review comments specific to tests formatting * use filesystem API instead of std::system for safer cleanup * Add ghc/filesystem submodule v1.5.14 for portable C++17 filesystem support * fix: add cmake/GhcFilesystem.cmake for CI submodule auto-checkout * incorporate review comment * incorporate review comment
litvaOo
pushed a commit
that referenced
this pull request
Dec 18, 2025
* test: add unit tests for common utilities from PR #1249 * incorporate review comments specific to tests formatting * use filesystem API instead of std::system for safer cleanup * Add ghc/filesystem submodule v1.5.14 for portable C++17 filesystem support * fix: add cmake/GhcFilesystem.cmake for CI submodule auto-checkout * incorporate review comment * incorporate review comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Remove duplicated path helper functions across multiple binaries and centralize them in a common utility.
Technical Details
Moved
get_rocprofsys_root,get_internal_libpath,get_internal_script_path,get_rocprofsys_root,get_internal_libpath,get_internal_script_path,get_internal_libdirintosource/lib/common/path.hppand updated all usages.Moved
remove_envintocommon/environment.hppand updated all usages.The remaining utilities will be included in Refactor Part 2.
Test Plan
Verified with HPCTraining daxpy example
Verified remove_env and all realpath-related APIs using the DAXPY example across four binaries on all ASICs.
Test Result
Verified build and above HPC example with 4 binaries in navi, mi300, 308, 325, 350 and 355
quick manual test for remove_env
Submission Checklist