Skip to content

Move non-sysdep third-party projects after base and compiler. #2045

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Move non-sysdep third-party projects after base and compiler. #2045

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91de8fb
Move non-sysdep third-party projects a fter base and compiler.
ScottTodd Nov 6, 2025
a70fa6f
Switch spdlog to build with the amd-llvm toolchain.
ScottTodd Nov 7, 2025
0b74653
Hack googletest include order. Sorry x_x.
ScottTodd Nov 7, 2025
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
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,10 @@ endif()

# Add subdirectories in dependency DAG order (which happens to be semi-alpha:
# don't be fooled).
add_subdirectory(third-party)
add_subdirectory(third-party/sysdeps)
add_subdirectory(base)
add_subdirectory(compiler)
add_subdirectory(third-party)
add_subdirectory(core)
Comment on lines 461 to 466
Copy link
Member Author

@ScottTodd ScottTodd Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

amdsmi (which is Linux only) in base/ depends on therock-googletest which is not a sysdep, see https://github.com/ROCm/TheRock/actions/runs/19153803999/job/54750039189?pr=2045#step:10:119

-- Including subproject amdsmi (from /__w/TheRock/TheRock/base/amdsmi)
--   PROVIDE amd_smi = lib/cmake (from amdsmi)
--   RUNTIME_DEPS: rocm-core therock-libdrm
--   INJECT rocm-core = /__w/TheRock/TheRock/build/base/rocm-core/dist/lib/cmake/rocm-core (from rocm-core)
CMake Error at cmake/therock_subproject.cmake:1026 (get_target_property):
  get_target_property() called with non-existent target "therock-googletest".
Call Stack (most recent call first):
  cmake/therock_subproject.cmake:1039 (_therock_assert_is_cmake_subproject)
  cmake/therock_subproject.cmake:667 (_therock_cmake_subproject_setup_deps)
  base/CMakeLists.txt:76 (therock_cmake_subproject_activate)

Code:

TheRock/base/CMakeLists.txt

Lines 61 to 69 in 77e4a83

therock_cmake_subproject_declare(amdsmi
EXTERNAL_SOURCE_DIR "amdsmi"
USE_DIST_AMDGPU_TARGETS
BACKGROUND_BUILD
CMAKE_ARGS
# TODO: Enable tests
# -DBUILD_TESTS=${THEROCK_BUILD_TESTING}
BUILD_DEPS
therock-googletest

This will need more work on Linux in that case

Copy link
Member Author

@ScottTodd ScottTodd Nov 7, 2025

Choose a reason for hiding this comment

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

Looks like we can either

  1. Move therock-googletest to 'sysdeps'
  2. Move amdsmi and rocm_smi_lib from 'base' to 'core'

I'm leaning towards (2) architecturally (since the libraries may expect googletest to be compiled with clang too), though (1) looks easier in the context of this PR.

Copy link
Collaborator

@stellaraccident stellaraccident Nov 7, 2025

Choose a reason for hiding this comment

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

Let's do 2 and should be fine. That will also solve a problem for me with respect to enabling ASAN for amdsmi (which depends on the clang toolchain).

If we do that, we should create an amdsmi artifact vs lumping it into "base". It needs to be its own and will need its own package, etc (so I think this was done as-is in error).

I'd love to further eliminate base/ entirely but we can get there eventually.

Copy link
Member Author

@ScottTodd ScottTodd Nov 7, 2025

Choose a reason for hiding this comment

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

Okay, can proceed with (2).

I'm also trying a third option: fiddle with the include order even further for just googletest. Might be able to unblock bringup work for rocroller and hipdnn on Windows faster that way.

Copy link
Member Author

@ScottTodd ScottTodd Nov 10, 2025

Choose a reason for hiding this comment

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

Filed #2072 (assigned to @erman-gurses ) for moving amdsmi to 'core'. The include order hack for just googletest did pass CI here, so we could proceed with this if we don't want to wait for moving amdsmi around.

# Note that rocprofiler-register is in base and is what higher level clients
# depend on. The profiler itself is independent.
Expand Down
9 changes: 0 additions & 9 deletions third-party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,3 @@ endif()
if(THEROCK_ENABLE_HOST_SUITE_SPARSE)
add_subdirectory(SuiteSparse)
endif()

# TODO: Relocate non header-only libraries here (i.e. boost, host-blas).
if(THEROCK_BUNDLE_SYSDEPS)
if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
add_subdirectory(sysdeps/linux)
elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows")
add_subdirectory(sysdeps/windows)
endif()
endif()
2 changes: 2 additions & 0 deletions third-party/spdlog/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ therock_cmake_subproject_declare(therock-spdlog
NO_MERGE_COMPILE_COMMANDS
OUTPUT_ON_FAILURE
EXTERNAL_SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/source"
COMPILER_TOOLCHAIN
amd-llvm
BUILD_DEPS
therock-fmt
CMAKE_ARGS
Expand Down
8 changes: 8 additions & 0 deletions third-party/sysdeps/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# TODO: Relocate non header-only libraries here (i.e. boost, host-blas).
if(THEROCK_BUNDLE_SYSDEPS)
if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
add_subdirectory(linux)
elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows")
add_subdirectory(windows)
endif()
endif()
Loading