Skip to content

fix(rocprofiler-sdk): add openmpi to BUILD_DEPS #6776

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.

Sign up for GitHub

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

Merged
araravik-psd merged 3 commits into from
Jul 22, 2026
Merged

fix(rocprofiler-sdk): add openmpi to BUILD_DEPS #6776

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b008218
Add BUILD_DEPS block with openmpi before RUNTIME_DEPS in the therock_…
aravind-ravi1206 Jul 22, 2026
74a5147
Add MPI build dep
aravind-ravi1206 Jul 22, 2026
2caa429
Moving to RUNTIME_DEPS
aravind-ravi1206 Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions profiler/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ if(THEROCK_ENABLE_ROCPROFV3)
rocprofiler-sdk-tests
COMPILER_TOOLCHAIN
amd-hip
BUILD_DEPS
${_openmpi_optional_dep}
RUNTIME_DEPS
aqlprofile

@ScottTodd ScottTodd Jul 22, 2026

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.

Should this _openmpi_optional_dep be a build dep or a runtime dep?

The other uses have it in RUNTIME_DEPS:

  • TheRock/profiler/CMakeLists.txt

    Lines 257 to 268 in 29faf3b

    INSTALL_RPATH_DIRS
    "lib/rocprofiler-compute"
    RUNTIME_DEPS
    rocprofiler-sdk
    aqlprofile
    hip-clr
    rocprofiler-register
    ${THEROCK_BUNDLED_ELFUTILS}
    ${THEROCK_BUNDLED_LIBDRM}
    ${THEROCK_BUNDLED_SQLITE3}
    ${_openmpi_optional_dep}
    TEST_SUBPROJECTS

  • TheRock/profiler/CMakeLists.txt

    Lines 348 to 361 in 29faf3b

    BUILD_DEPS
    therock-spdlog
    therock-fmt
    RUNTIME_DEPS
    amdsmi
    aqlprofile
    hip-clr
    rocprofiler-register
    rocprofiler-sdk
    ${THEROCK_BUNDLED_ELFUTILS}
    ${THEROCK_BUNDLED_LIBDRM}
    ${THEROCK_BUNDLED_SQLITE3}
    ${_openmpi_optional_dep}
    TEST_SUBPROJECTS

@araravik-psd araravik-psd Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe, It should stay in BUILD_DEPS. The find_package(MPI) in rocprofiler-sdk fires at configure time. the dep provider intercepts it before a single source file is compiled. I feel, the other subprojects that put _openmpi_optional_dep in RUNTIME_DEPS are most likely only using MPI for test execution, not as a compile-time linking dependency.

@ScottTodd ScottTodd Jul 22, 2026

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.

All RUNTIME_DEPS are also BUILD_DEPS, not all BUILD_DEPS are RUNTIME_DEPS, see

TheRock/cmake/therock_subproject.cmake

Lines 244 to 248 in 29faf3b

# BUILD_DEPS: Projects which must build and provide their packages prior to this
# one.
# RUNTIME_DEPS: Projects which must build prior to this one and whose install
# files must be distributed with this project's artifacts in order to
# function.

If MPI is only used at build time then BUILD_DEPS is appropriate. If it's also used at run time then it should go in RUNTIME_DEPS.

@araravik-psd araravik-psd Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ScottTodd for linking #6610 and #5627, Keeping BUILD_DEPS, MPI is intentionally not distributed with ROCm artifacts (from #6610 which added the dedicated OpenMPI test image for exactly this reason, and #5627 which wires rocprofiler-sdk tests to use it). libmpi.so is satisfied at test runtime via the container
image, not shipped with release artifacts, so BUILD_DEPS would be accurate here?

@ScottTodd ScottTodd Jul 22, 2026

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.

Maybe? I'd prefer local consistency with a small fix like this.

If we need MPI available at configure time but we don't want to distribute it (leaving it as an optionally dynamically linked runtime dependency), then I think BUILD_DEPS could work, but we'd want to be careful about how we represent that in the build and packaging files. Pretty much all of our other dependencies expect to be vendored and distributed together with the libraries and tests.

@araravik-psd araravik-psd Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point on local consistency, I'll move it to RUNTIME_DEPS to match rocprofiler-compute and rocprofiler-systems. The proper long-term fix (guarding find_package(MPI) in rocprofiler-sdk's cmake so it's not reached in standard builds at all) will come as a follow-up PR in rocprofiler-sdk, which will make the dep declaration here moot when THEROCK_ENABLE_MPI=OFF. Moving now to unblock RCCL CI.

hip-clr
Expand Down
Loading