Reapply "Move non-sysdep third-party projects after base and compiler"#3440
Conversation
The spdlog add_subdirectory is now guarded by if(THEROCK_ENABLE_SPDLOG), so configure_stage.py must know about the dependency to enable the feature in downstream stages. Without this, math-libs, profiler-apps, and iree-libs stages fail to configure because the therock-spdlog target is never created. Changes: - Add "spdlog" to artifact_deps of blas, hipdnn, miopen-plugin, hipblaslt-plugin, hipdnn-samples, fusilli-plugin, and rocprofiler-systems 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| artifact_group = "math-libs" | ||
| type = "target-specific" | ||
| artifact_deps = ["core-runtime", "core-hip", "core-amdsmi", "host-blas", "host-suite-sparse", "rocprofiler-sdk"] | ||
| artifact_deps = ["core-runtime", "core-hip", "core-amdsmi", "host-blas", "host-suite-sparse", "rocprofiler-sdk", "spdlog"] |
There was a problem hiding this comment.
So far spdlog belonged to third-party-libs which covers optional deps. If we strictly want to pull spdlog in, we should rather consider moving it to third-party-sysdeps. However, it might just wrong how the components depend on it if this is optional?
There was a problem hiding this comment.
I don't understand what "optional deps" mean in the context of this file. There are subprojects that require these dependencies. This is also not a sysdep (it isn't under https://github.com/ROCm/TheRock/tree/main/third-party/sysdeps), so it shouldn't go in third-party-sysdeps, unless I'm missing some detail in how deps are defined...
There was a problem hiding this comment.
Yeah I think the descriptions here are just wrong:
Lines 195 to 202 in 1f5412f
- third-party-sysdeps are libraries that we build for portable distribution
- third-party-libs are build dependencies. they include some truly optional libraries (HOST_MATH: host-blas, suite-sparse, fftw3)
I can send a separate PR to update the docs. Raw notes from Claude Code: https://gist.github.com/ScottTodd/bb0ca24ff2a90698df3229a886d2611c
There was a problem hiding this comment.
Okay, that might have trapped me here. Doc update can be a follow up for sure if needed but it feels there might be more wrong. Will try to take a closer look.
|
(#3474 is stacked after this and it moves nearly all third-party subprojects to build with amd-llvm) |
| artifact_group = "math-libs" | ||
| type = "target-specific" | ||
| artifact_deps = ["core-runtime", "core-hip", "core-amdsmi", "host-blas", "host-suite-sparse", "rocprofiler-sdk"] | ||
| artifact_deps = ["core-runtime", "core-hip", "core-amdsmi", "host-blas", "host-suite-sparse", "rocprofiler-sdk", "spdlog"] |
There was a problem hiding this comment.
Okay, that might have trapped me here. Doc update can be a follow up for sure if needed but it feels there might be more wrong. Will try to take a closer look.
|
Synced and resolved some merge conflicts in the topology file. Triggered a multi-arch CI test run just to be sure: https://github.com/ROCm/TheRock/actions/runs/22232603395 |
|
CI runs seem to be far enough along to trust that they will succeed. |
) ## Motivation Following up on discussion at #3440 (comment). The descriptions in `BUILD_TOPOLOGY.toml` for `third-party-sysdeps` and `third-party-libs` were misleading. This adds more specific background details explaining what qualifies as a sysdep. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Follow-up to #2045 (technically #3440) and based on comments at #1284 (comment). The general goal here is to build _only_ sysdeps with the system toolchain/compiler and to build the rest of the third-party deps (and ROCm) using our own amd-llvm toolchain. This will let more subprojects build with sanitizers as those are generally not portable across toolchains. ## Technical Details * This required some additional features in the build topology system to add a `THEROCK_ENABLE_THIRD_PARTY_LIBS` CMake option that gets automatically enabled when an artifact includes the `third-party-libs` feature via `group_deps`. The existing `artifact_deps` was not sufficient for this as the third party libraries do not produce artifacts and are instead pure source dependencies, which now take a dependency on the amd-llvm (compiler) artifact. * I wasn't able to build `therock-eigen` or `therock-host-blas` on Windows using the amd-llvm toolchain without deeper changes (see the logs snippets in the source). I did not test those builds on Linux. ## Test Plan * Local build on Windows * Regular CI on this PR * Multi-arch CI manually triggered: * (recent) https://github.com/ROCm/TheRock/actions/runs/22598994883 * (older) https://github.com/ROCm/TheRock/actions/runs/22235154958 * (even older) https://github.com/ROCm/TheRock/actions/runs/22119457085 ## Test Result * Local build succeeded and used Clang from amd-llvm as expected for each of the third-party subprojects. * CI build https://github.com/ROCm/TheRock/actions/runs/22121852952?pr=3474 passed builds and most tests (preexisting flakes?) * Multi-arch CI build https://github.com/ROCm/TheRock/actions/runs/22119457085 passed builds and most tests (preexisting flakes/failures?) ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Marius Brehler <marius.brehler@amd.com>
## Motivation Follow-up to #2045 (technically #3440) and based on comments at #1284 (comment). The general goal here is to build _only_ sysdeps with the system toolchain/compiler and to build the rest of the third-party deps (and ROCm) using our own amd-llvm toolchain. This will let more subprojects build with sanitizers as those are generally not portable across toolchains. ## Technical Details * This required some additional features in the build topology system to add a `THEROCK_ENABLE_THIRD_PARTY_LIBS` CMake option that gets automatically enabled when an artifact includes the `third-party-libs` feature via `group_deps`. The existing `artifact_deps` was not sufficient for this as the third party libraries do not produce artifacts and are instead pure source dependencies, which now take a dependency on the amd-llvm (compiler) artifact. * I wasn't able to build `therock-eigen` or `therock-host-blas` on Windows using the amd-llvm toolchain without deeper changes (see the logs snippets in the source). I did not test those builds on Linux. ## Test Plan * Local build on Windows * Regular CI on this PR * Multi-arch CI manually triggered: * (recent) https://github.com/ROCm/TheRock/actions/runs/22598994883 * (older) https://github.com/ROCm/TheRock/actions/runs/22235154958 * (even older) https://github.com/ROCm/TheRock/actions/runs/22119457085 ## Test Result * Local build succeeded and used Clang from amd-llvm as expected for each of the third-party subprojects. * CI build https://github.com/ROCm/TheRock/actions/runs/22121852952?pr=3474 passed builds and most tests (preexisting flakes?) * Multi-arch CI build https://github.com/ROCm/TheRock/actions/runs/22119457085 passed builds and most tests (preexisting flakes/failures?) ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Marius Brehler <marius.brehler@amd.com>
Motivation
This reverts #3439 to reapply #2045 and fix #2003.
Technical Details
The original PR was missing changes in
BUILD_TOPOLOGY.toml. If we switch more third-party subprojects to use the amd-llvm toolchain they will need similar changes.I'm also testing a follow-up change to migrate (nearly) all other third-party subprojects to use amd-llvm.
We could re-evaluate the build stages as "Foundation" gets smaller and "Compiler Runtime" gets larger (could merge them or split out a new stage for third party deps that use the compiler 🤔).
Test Plan
Run multi-arch CI on this branch: https://github.com/ROCm/TheRock/actions/runs/22112365715
Submission Checklist