Repository navigation
[rocprofiler-sdk] Improve build with system libraries. - #2319
Conversation
|
@lubosz Regarding:
I think what you actually want is |
7091bab to
f0b2a63
Compare
Good catch! I have updated the patch. |
f0b2a63 to
26aef84
Compare
26aef84 to
7a096df
Compare
7a096df to
27a5a70
Compare
There was a problem hiding this comment.
Pull request overview
Improve rocprofiler-sdk/rocprofv3 builds when preferring system-provided dependencies (yaml-cpp, fmt) and fix a CMake packaging issue when glog is disabled.
Changes:
- Add
ROCPROFILER_BUILD_YAML_CPPoption to switch between vendored yaml-cpp submodule and system yaml-cpp (>= 0.8.0). - Fix fmt 12.1.0+ compilation by including
<fmt/ranges.h>wherefmt::join/range formatting is used. - Fix packaging configuration by explicitly including
CPackComponentbefore using component-group macros; update fmt submodule to 12.1.0.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/metrics.hpp | Add missing standard header include. |
| projects/rocprofiler-sdk/source/lib/python/rocpd/source/sql_generator.hpp | Include <fmt/ranges.h> to support fmt 12.1.0+ range utilities. |
| projects/rocprofiler-sdk/source/lib/output/sql/common.hpp | Include <fmt/ranges.h> to support fmt 12.1.0+ range utilities. |
| projects/rocprofiler-sdk/external/fmt | Update fmt submodule revision to 12.1.0. |
| projects/rocprofiler-sdk/external/CMakeLists.txt | Add switch to use system yaml-cpp or build vendored yaml-cpp submodule. |
| projects/rocprofiler-sdk/cmake/rocprofiler_options.cmake | Add ROCPROFILER_BUILD_YAML_CPP CMake option. |
| projects/rocprofiler-sdk/cmake/rocprofiler_config_packaging.cmake | Include CPackComponent explicitly to avoid missing macro errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27a5a70 to
9f0bf40
Compare
9f0bf40 to
3b7dbe5
Compare
3b7dbe5 to
8135832
Compare
1ad781b to
598cd94
Compare
Add ROCPROFILER_BUILD_YAML_CPP cmake variable, defaulting to ON, which keeps the previous behaviour as default. This enables building with a system yaml-cpp. Link rocprofiler-sdk-yaml-cpp with yaml-cpp::yaml-cpp in that case, as is also done with fmt, glog, etc. Since the submodule uses yaml-cpp's master branch, set the required version to the latest release 0.8.0. Signed-off-by: Lubosz Sarnecki <lubosz@gmail.com>
Resolves following error when building with `-D ROCPROFILER_BUILD_GLOG=OFF`: ``` CMake Error at cmake/rocprofiler_config_packaging.cmake:124 (cpack_add_component_group): Unknown CMake command "cpack_add_component_group". Call Stack (most recent call first): CMakeLists.txt:135 (include) ``` Signed-off-by: Lubosz Sarnecki <lubosz@gmail.com>
This enables building with fmt versions 12.1.0 and higher by including the now required <fmt/ranges.h> header for fmt::join. This change is backwards compatible with the current submodule version of fmt which is an old main branch, 15 commits above 10.1.0. Signed-off-by: Lubosz Sarnecki <lubosz@gmail.com>
Signed-off-by: Lubosz Sarnecki <lubosz@gmail.com>
598cd94 to
52e825f
Compare
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_ELFIO, which can be set to OFF to make rocprofiler use a system dependency for ELFIO instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_ELFIO is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_OTF2, which can be set to OFF to make rocprofiler-sdk use a system dependency for OTF2 instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_OTF2 is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_ELFIO, which can be set to OFF to make rocprofiler use a system dependency for ELFIO instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_ELFIO is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_OTF2, which can be set to OFF to make rocprofiler-sdk use a system dependency for OTF2 instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_OTF2 is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_ELFIO, which can be set to OFF to make rocprofiler use a system dependency for ELFIO instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_ELFIO is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_OTF2, which can be set to OFF to make rocprofiler-sdk use a system dependency for OTF2 instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_OTF2 is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_ELFIO, which can be set to OFF to make rocprofiler use a system dependency for ELFIO instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_ELFIO is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_OTF2, which can be set to OFF to make rocprofiler-sdk use a system dependency for OTF2 instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_OTF2 is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_ELFIO, which can be set to OFF to make rocprofiler use a system dependency for ELFIO instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_ELFIO is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_OTF2, which can be set to OFF to make rocprofiler-sdk use a system dependency for OTF2 instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_OTF2 is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_ELFIO, which can be set to OFF to make rocprofiler use a system dependency for ELFIO instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_ELFIO is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_OTF2, which can be set to OFF to make rocprofiler-sdk use a system dependency for OTF2 instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_OTF2 is set to ON by default. Related: #2319
This adds a new option, ROCPROFILER_BUILD_JSON, which can be set to OFF to make rocprofiler-sdk use a system dependency for nlohmann_json instead of trying to build it manually. To mimick old behavior, ROCPROFILER_BUILD_JSON is set to ON by default. Related: #2319
…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.
…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.
…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.
…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.
…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.
…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.
…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
This series improves building rocprofiler-sdk / rocprofv3 with system libraries.
ROCPROFILER_BUILD_GLOGwas offThis series is the result of packaging
rocprofv3in Arch Linux.See https://gitlab.archlinux.org/archlinux/packaging/packages/rocprofiler/-/merge_requests/3
Technical Details
Add option to use system yaml-cpp.
Adds
ROCPROFILER_BUILD_YAML_CPPCMake variable, defaulting to ON, which keeps the previous behaviour as default.This enables building with a system yaml-cpp.
Since the submodule uses yaml-cpp's master branch, set the required version to the latest release 0.8.0.
Inlcude CPack before calling it
This resolves a bug in the CMake code, which relied on the glog submodule to include CPack, which was included too late.
Resolves following CMake error when building with
-D ROCPROFILER_BUILD_GLOG=OFF:Fix build with fmt 12.1.0.
This enables building with fmt versions 12.1.0 and higher by including
the now required <fmt/ranges.h> header for fmt::join.
This change is backwards compatible with the current submodule version
of fmt which is an old main branch, 15 commits above 10.1.0.
Update fmt submodule to 12.1.0
This patch is optional and I can drop it if required.
This updates the fmt submodule to 12.1.0, which is now possible with the previous patch and a good opportunity to change the submodule to a release version. Currently it uses an old main branch, 15 commits above the 10.1.0 release.
Test Plan
I am able to
rocprofiler-sdkwith and without system libraries with this patch set.Running a CI build should ensure the validity of this patch set.
Test Result
Works for me.
Submission Checklist