Repository navigation
fix(rocprofiler-sdk): miscellanous build fixes for downstream packaging - #4721
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates rocprofiler-sdk’s build system to be more robust for downstream packaging (notably NixOS) by fixing missing headers/dependencies, correcting CMake find_package() visibility, and adding opt-in support for system-provided third-party libraries.
Changes:
- Add missing standard library headers and ensure several targets explicitly link against the AMD COMGR interface target.
- Fix CMake package visibility/scoping by using
find_package(... GLOBAL)and bumpcmake_minimum_requiredaccordingly. - Add build options to use bundled vs system dependencies for ELFIO, OTF2, and nlohmann/json; reduce/standardize custom warning flags on test binaries.
Reviewed changes
Copilot reviewed 17 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-sdk/CMakeLists.txt | Bump CMake minimum version to support find_package(... GLOBAL). |
| projects/rocprofiler-sdk/cmake/rocprofiler_options.cmake | Add options to toggle bundled vs system ELFIO/OTF2/JSON. |
| projects/rocprofiler-sdk/external/CMakeLists.txt | Apply GLOBAL find_package scoping fixes; add system-dependency paths for ELFIO/OTF2/JSON. |
| projects/rocprofiler-sdk/source/lib/att-tool/att_lib_wrapper.hpp | Add missing standard headers for successful compilation in stricter environments. |
| projects/rocprofiler-sdk/source/lib/att-tool/waitcnt/tests/CMakeLists.txt | Ensure unit test links AMD COMGR interface target. |
| projects/rocprofiler-sdk/source/lib/python/utilities.cmake | Add AMD COMGR link dependency for rocpd python bindings. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-rocpd/sql.cpp | Add missing <fstream> include. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-rocattach/auxv.cpp | Add missing <fstream> include. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-rocattach/symbol_lookup.cpp | Add missing standard headers (<array>, <string>, <fstream>). |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/metrics.cpp | Add missing <fstream> include. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/CMakeLists.txt | Add AMD COMGR dependency to counter-related tests/targets. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/parser/tests/CMakeLists.txt | Add AMD COMGR dependency to pc-sampling parser tests. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/CMakeLists.txt | Add AMD COMGR dependency to pc-sampling tests. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/tests/CMakeLists.txt | Add AMD COMGR dependency to rocprofiler-sdk lib tests (static/shared). |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/tests/status.cpp | Add missing <iomanip> include. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/thread_trace/tests/CMakeLists.txt | Add AMD COMGR dependency to thread-trace tests. |
| projects/rocprofiler-sdk/tests/bin/attachment-test/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/hsa-code-object/CMakeLists.txt | Remove per-target warning flags (incl. -Werror). |
| projects/rocprofiler-sdk/tests/bin/hsa-memory-allocation/CMakeLists.txt | Replace per-target warning flags with minimal -Wpedantic. |
| projects/rocprofiler-sdk/tests/bin/hsa-queue-dependency/CMakeLists.txt | Remove per-target warning flags (incl. -Werror). |
| projects/rocprofiler-sdk/tests/bin/hip-graph/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/hip-in-libraries/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/multistream/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/pc-sampling/exec-mask-manipulation/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/reproducible-dispatch-count/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/reproducible-runtime/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/roctx-pause-resume/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/scratch-memory/CMakeLists.txt | Replace per-target warning flags with minimal -Wpedantic. |
| projects/rocprofiler-sdk/tests/bin/simple-transpose/CMakeLists.txt | Replace per-target warning flags; link standardized test build flags. |
| projects/rocprofiler-sdk/tests/bin/transpose/CMakeLists.txt | Replace per-target warning flags within helper function. |
| projects/rocprofiler-sdk/tests/bin/vector-operations/CMakeLists.txt | Replace per-target warning flags with minimal -Wpedantic. |
| projects/rocprofiler-sdk/tests/pc_sampling/CMakeLists.txt | Link against AMD COMGR interface target instead of raw amd_comgr. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d4e3fd7 to
5aa7375
Compare
5aa7375 to
6a40713
Compare
geomin12
left a comment
There was a problem hiding this comment.
workflow updates look fine to me, but going to let rocprofiler team review this
6a40713 to
9e990cb
Compare
|
Rebased due to a change in the base branch. I've rebuilt it after fixing the changes, seems to still be fine. |
5292240 to
a1ed20d
Compare
|
I think the failure is unrelated to my changes here. |
a1ed20d to
a0cf322
Compare
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
This pull request has been automatically closed due to inactivity (30 days with no updates). If you'd like to continue working on this, feel free to reopen the PR or create a new one. |
6b5b52d to
d4a823e
Compare
jbonnell-amd
left a comment
There was a problem hiding this comment.
Approved for .github/workflows/* changes
geomin12
left a comment
There was a problem hiding this comment.
a few yml file comments
rest of the cmake changes can be reviewed via rocprofiler team
|
@Snektron should be ready to merge once conversations are resolved. We could resolve the nonblocking comments in a follow-up issue + PR. |
|
Thanks yall! |
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
TheRock Submodule Bump ActivityNewest first
|
…ng (#4721) ## Motivation These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820. ## Technical Details There should be nothing shocking, but its a mixed bag of miscellaneous stuff: - Add missing c++ headers, revealed by building without GHC FS. - Add missing dependencies on comgr on several test targets - Fix `find_package()` scoping issues. The problem and solution is described in more detail [here](#1243), I've applied the same fix of passing `GLOBAL` to `find_package()`. - Add options for using system dependencies for elfio, otf2 and nlohmann/json. This is the same problem & solution as applied recently for yaml-cpp. See [previous pull request](#2319). - Replaces most custom warning flags set on integration tests with rocprofiler-sdk::test-build-flags or a related dependency. The motivation for this was to disable -Werror, and these targets already handle that. Some targets also required -Wpedantic, I've left those were they were to avoid changing anything VS the normal CI build. Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies). ## JIRA ID Closes #4721 ## Test Plan Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine. ## Test Result Works on my machine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
…ng (#4721) ## Motivation These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820. ## Technical Details There should be nothing shocking, but its a mixed bag of miscellaneous stuff: - Add missing c++ headers, revealed by building without GHC FS. - Add missing dependencies on comgr on several test targets - Fix `find_package()` scoping issues. The problem and solution is described in more detail [here](#1243), I've applied the same fix of passing `GLOBAL` to `find_package()`. - Add options for using system dependencies for elfio, otf2 and nlohmann/json. This is the same problem & solution as applied recently for yaml-cpp. See [previous pull request](#2319). - Replaces most custom warning flags set on integration tests with rocprofiler-sdk::test-build-flags or a related dependency. The motivation for this was to disable -Werror, and these targets already handle that. Some targets also required -Wpedantic, I've left those were they were to avoid changing anything VS the normal CI build. Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies). ## JIRA ID Closes #4721 ## Test Plan Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine. ## Test Result Works on my machine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
…ng (#4721) ## Motivation These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820. ## Technical Details There should be nothing shocking, but its a mixed bag of miscellaneous stuff: - Add missing c++ headers, revealed by building without GHC FS. - Add missing dependencies on comgr on several test targets - Fix `find_package()` scoping issues. The problem and solution is described in more detail [here](#1243), I've applied the same fix of passing `GLOBAL` to `find_package()`. - Add options for using system dependencies for elfio, otf2 and nlohmann/json. This is the same problem & solution as applied recently for yaml-cpp. See [previous pull request](#2319). - Replaces most custom warning flags set on integration tests with rocprofiler-sdk::test-build-flags or a related dependency. The motivation for this was to disable -Werror, and these targets already handle that. Some targets also required -Wpedantic, I've left those were they were to avoid changing anything VS the normal CI build. Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies). ## JIRA ID Closes #4721 ## Test Plan Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine. ## Test Result Works on my machine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
…ng (#4721) ## Motivation These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820. ## Technical Details There should be nothing shocking, but its a mixed bag of miscellaneous stuff: - Add missing c++ headers, revealed by building without GHC FS. - Add missing dependencies on comgr on several test targets - Fix `find_package()` scoping issues. The problem and solution is described in more detail [here](#1243), I've applied the same fix of passing `GLOBAL` to `find_package()`. - Add options for using system dependencies for elfio, otf2 and nlohmann/json. This is the same problem & solution as applied recently for yaml-cpp. See [previous pull request](#2319). - Replaces most custom warning flags set on integration tests with rocprofiler-sdk::test-build-flags or a related dependency. The motivation for this was to disable -Werror, and these targets already handle that. Some targets also required -Wpedantic, I've left those were they were to avoid changing anything VS the normal CI build. Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies). ## JIRA ID Closes #4721 ## Test Plan Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine. ## Test Result Works on my machine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
…ng (#4721) ## Motivation These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820. ## Technical Details There should be nothing shocking, but its a mixed bag of miscellaneous stuff: - Add missing c++ headers, revealed by building without GHC FS. - Add missing dependencies on comgr on several test targets - Fix `find_package()` scoping issues. The problem and solution is described in more detail [here](#1243), I've applied the same fix of passing `GLOBAL` to `find_package()`. - Add options for using system dependencies for elfio, otf2 and nlohmann/json. This is the same problem & solution as applied recently for yaml-cpp. See [previous pull request](#2319). - Replaces most custom warning flags set on integration tests with rocprofiler-sdk::test-build-flags or a related dependency. The motivation for this was to disable -Werror, and these targets already handle that. Some targets also required -Wpedantic, I've left those were they were to avoid changing anything VS the normal CI build. Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies). ## JIRA ID Closes #4721 ## Test Plan Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine. ## Test Result Works on my machine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
…ng (#4721) ## Motivation These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820. ## Technical Details There should be nothing shocking, but its a mixed bag of miscellaneous stuff: - Add missing c++ headers, revealed by building without GHC FS. - Add missing dependencies on comgr on several test targets - Fix `find_package()` scoping issues. The problem and solution is described in more detail [here](#1243), I've applied the same fix of passing `GLOBAL` to `find_package()`. - Add options for using system dependencies for elfio, otf2 and nlohmann/json. This is the same problem & solution as applied recently for yaml-cpp. See [previous pull request](#2319). - Replaces most custom warning flags set on integration tests with rocprofiler-sdk::test-build-flags or a related dependency. The motivation for this was to disable -Werror, and these targets already handle that. Some targets also required -Wpedantic, I've left those were they were to avoid changing anything VS the normal CI build. Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies). ## JIRA ID Closes #4721 ## Test Plan Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine. ## Test Result Works on my machine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Tweak docs and min versions, targeting PR #4721 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.



Motivation
These changes were required in order to properly package rocprofiler-sdk for NixOS, see NixOS/nixpkgs#506820.
Technical Details
There should be nothing shocking, but its a mixed bag of miscellaneous stuff:
find_package()scoping issues. The problem and solution is described in more detail here, I've applied the same fix of passingGLOBALtofind_package().Most changes should either not affect the build by much (like adding missing includes or dependencies) or are optional and disabled by default (like using system versions of dependencies).
JIRA ID
Closes #4721
Test Plan
Built rocprofiler-sdk on NixOS and ran a few manual tests, did not build a non-NixOS version but I think that if it passes CI then it should be fine.
Test Result
Works on my machine.
Submission Checklist