-
Notifications
You must be signed in to change notification settings - Fork 167
[rocprofiler-systems] Add third-party dependencies #2657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ers/dgaliffi/rocprof-sys-merged
| rocm-cmake | ||
| rocPRIM | ||
| therock-googletest | ||
| therock-tbb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, rocthrust configure kept failing due to this error:
Project contains find_package(TBB) for a package available in the super-project but not declared: Add a BUILD_DEPS or RUNTIME_DEPS appropriately
Not sure where this is coming from but added this for now as a workaround. Would appreciate some insight on what the root cause of this might be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure that thrust must have an optional dependency on TBB and is probing for it. TheRock doesn't let that happen (for this reason). You can add TBB to thrust to unblock BUILD_DEPS to unblock this PR but we should really have an explicit flag to enable/disable the TBB dep there vs having implicit behavior like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dread TBB more than Boost 🗡️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbonnell-amd can you file an issue and link it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done #2837
Co-authored-by: Marius Brehler <[email protected]>
Co-authored-by: Marius Brehler <[email protected]>
Co-authored-by: Marius Brehler <[email protected]>
geomin12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but I will let Laura or someone more familiar with build to review this
| "-DTHEROCK_BOOST_LIBRARIES=atomic,filesystem,multi_index,system" | ||
| "-DTHEROCK_BOOST_LIBRARIES=atomic,chrono,date_time,filesystem,multi_index,system,thread,timer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added
dyninstthird-party dependency
- [Feature]: Add DYNINST third-party dependency #1905
- Using latest release
v13.0.0- Required for
rocprofiler-systems- Modified
boostthird-party dependency
- Added additional packages
chrono, date_time, thread, timerwhich are needed as a dependency fordyninst
We really shouldn't be expanding our dependency on boost, especially not for relatively simple utilities like these. This is especially problematic if the dependency is indirect, as that could make migrating away more difficult.
Should we consider these "grandfathered in" and allow them to be added here, or draw a line and reject them? Either way, the boost dependency must be removed eventually - is the rocprofiler-systems team prioritizing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we weren't. But we need to get the whole gang in place before giving any of them a haircut.
| COMMAND | ||
| make -j 1 V=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this build step benefit from higher parallelism? Logs at https://therock-ci-artifacts.s3.amazonaws.com/20722897507-linux/logs/gfx94X-dcgpu/therock-binutils_build.log suggest this may be taking 5 minutes? Can we try with multithreading?
See how other dependencies use a PAR_JOBS variable:
TheRock/third-party/sysdeps/common/libbacktrace/CMakeLists.txt
Lines 68 to 69 in e07d037
| include(ProcessorCount) | |
| ProcessorCount(PAR_JOBS) |
TheRock/third-party/sysdeps/common/libbacktrace/CMakeLists.txt
Lines 84 to 85 in e07d037
| COMMAND | |
| make -j "${PAR_JOBS}" V=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've updated the make command to use the $PAR_JOBS var instead of just using 1. Thanks!
| feature_group = "MEDIA" | ||
| disable_platforms = ["windows"] | ||
|
|
||
| [artifacts.dyninst] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please set this up as an optional sysdep in the same way as artifacts.sysdeps-amd-mesa.
It is actually a requirement this is set up this way to comply with LGPL licensing (ensures dynamic linkage). And we enforce this for sysdeps (only).
| rocm-cmake | ||
| rocPRIM | ||
| therock-googletest | ||
| therock-tbb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure that thrust must have an optional dependency on TBB and is probing for it. TheRock doesn't let that happen (for this reason). You can add TBB to thrust to unblock BUILD_DEPS to unblock this PR but we should really have an explicit flag to enable/disable the TBB dep there vs having implicit behavior like this.
| add_subdirectory(grpc) | ||
|
|
||
| # dyninst depends on elfutils, boost, tbb | ||
| add_subdirectory(dyninst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to go into sysdeps/linux as an optional sysdep. Follow the same pattern as was done for amdmesa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on moving dyninst into sysdeps/linux on a side branch, I'll merge the changes back over here once it's all done and confirmed working. Thanks for the feedback!
| "-DTHEROCK_BOOST_LIBRARIES=atomic,filesystem,multi_index,system" | ||
| "-DTHEROCK_BOOST_LIBRARIES=atomic,chrono,date_time,filesystem,multi_index,system,thread,timer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we weren't. But we need to get the whole gang in place before giving any of them a haircut.
| ] | ||
|
|
||
| # binutils | ||
| [components.dev."third-party/sysdeps/linux/binutils/build/stage"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this an optional sysdep in the same pattern as amdmesa, with its own artifact.
| "${CMAKE_CURRENT_BINARY_DIR}/s/configure" | ||
| --prefix "${CMAKE_INSTALL_PREFIX}" | ||
| --enable-install-libiberty | ||
| --disable-shared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that this is only providing binaries, and even if GPL conditions are satisfied since nothing links to these.
| endif() | ||
|
|
||
| if(NOT TARGET LibIberty::LibIberty) | ||
| add_library(LibIberty::LibIberty STATIC IMPORTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libiberty is LGPL licensed. We have to use it as a shared library or not at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbonnell-amd please ping once these comments are resolved and you want another review.
7e9ba03 to
3077ce8
Compare

Motivation
This change will enable the use of third-party builds from TheRock for the dyninst and other dependenices that rocprofiler-systems uses. Previously, rocprofiler-systems was downloading and building these as part of its CMake build process.
If desired, I can split this PR up into smaller chunks based on the third-party dependency that is being added/modified. However, all of these are required to get rocprofiler-systems building in the new way, so it may lead to additional small PRs to get it all merged together in the end and enable the new build method.
Technical Details
tbbthird-party dependencyv2022.3.0dyninstbinutilsthird-party dependencybinutils-2.45libibertypackages that are required bydyninstdyninstthird-party dependencyv13.0.0rocprofiler-systemsboostthird-party dependencychrono, date_time, thread, timerwhich are needed as a dependency fordyninstelfutilsthird-party dependency-fPICoption to CFLAGS and CPPFLAGS to enable PIC (position-independent code) which is required forrocprofiler-systemsCMakeLists.txtto includeTHEROCK_BUNDLED_BINUTILSoptionCMakeLists.txtto removeDROCPROFSYS_BUILD_*CMake arguments, add the new third-party dependencies insteadTest Plan
rocprofiler-systems is able to be built properly with this change, both locally and in pipeline.
Test Result
rocprofiler-systems is able to build successfully without having to download and build its dependencies from source in its build process.
Submission Checklist