build(deps): Migrate fmt & spdlog dependencies to task-based installation (fixes #720).#870
Conversation
WalkthroughThis change set removes custom static linking logic and installation scripts for the Changes
Sequence Diagram(s)sequenceDiagram
participant TaskRunner
participant FmtInstaller
participant SpdlogInstaller
participant CMake
participant BuildTarget
TaskRunner->>FmtInstaller: Run fmt install task
FmtInstaller-->>TaskRunner: fmt installed
TaskRunner->>SpdlogInstaller: Run spdlog install task (depends on fmt)
SpdlogInstaller-->>TaskRunner: spdlog installed
TaskRunner->>CMake: Configure build (find_package fmt, spdlog)
CMake->>BuildTarget: Link against fmt::fmt, spdlog::spdlog
BuildTarget-->>CMake: Build with dependencies
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
taskfiles/deps/main.yaml (1)
240-266: Definespdloginstallation task and disable examples: Suggest optional improvementThe
spdlogtask correctly referencesfmtsettings and disables examples. To further speed up the build and reduce unnecessary artifacts, consider adding a flag to disable test builds as well:-CMAKE_GEN_ARGS: - - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_FMT_LIB_NAME}}.cmake" - - "-DCMAKE_BUILD_TYPE=Release" - - "-DCMAKE_INSTALL_MESSAGE=LAZY" - - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW" - - "-DSPDLOG_BUILD_EXAMPLE=OFF" - - "-DSPDLOG_BUILD_EXAMPLE_HO=OFF" - - "-DSPDLOG_FMT_EXTERNAL=ON" +CMAKE_GEN_ARGS: + - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_FMT_LIB_NAME}}.cmake" + - "-DCMAKE_BUILD_TYPE=Release" + - "-DCMAKE_INSTALL_MESSAGE=LAZY" + - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW" + - "-DSPDLOG_BUILD_EXAMPLE=OFF" + - "-DSPDLOG_BUILD_EXAMPLE_HO=OFF" + - "-DSPDLOG_BUILD_TESTS=OFF" + - "-DSPDLOG_FMT_EXTERNAL=ON"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/Modules/Findspdlog.cmake(0 hunks)components/core/src/clp_s/CMakeLists.txt(1 hunks)components/core/tools/scripts/lib_install/centos-stream-9/install-packages-from-source.sh(0 hunks)components/core/tools/scripts/lib_install/fmtlib.sh(0 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(0 hunks)components/core/tools/scripts/lib_install/spdlog.sh(0 hunks)components/core/tools/scripts/lib_install/ubuntu-focal/install-packages-from-source.sh(0 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh(0 hunks)docs/src/dev-guide/components-core/index.md(1 hunks)taskfiles/deps/main.yaml(4 hunks)
💤 Files with no reviewable changes (7)
- components/core/tools/scripts/lib_install/macos/install-all.sh
- components/core/tools/scripts/lib_install/ubuntu-focal/install-packages-from-source.sh
- components/core/tools/scripts/lib_install/centos-stream-9/install-packages-from-source.sh
- components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
- components/core/cmake/Modules/Findspdlog.cmake
- components/core/tools/scripts/lib_install/spdlog.sh
- components/core/tools/scripts/lib_install/fmtlib.sh
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, true)
🔇 Additional comments (9)
components/core/src/clp_s/CMakeLists.txt (1)
211-214: Linkspdlog::spdlogtoclp_s_TimestampPattern: ApprovedThe addition of
spdlog::spdlogensures that timestamp pattern components can leverage spdlog for logging. It matches the executable target’s linkage and uses the imported target from the core CMakeLists.docs/src/dev-guide/components-core/index.md (2)
39-39: Documentfmtas a source dependency: ApprovedListing
fmt(v8.0.1) under Source Dependencies aligns with the build tasks and CMake integration.
44-44: Documentspdlogas a source dependency: ApprovedIncluding
spdlog(v1.9.2) is consistent with the task-based installation approach and CMake targets.components/core/CMakeLists.txt (2)
151-154: Findfmtwithout version constraint: ApprovedSwitching to
find_package(fmt REQUIRED)without a hardcoded minimum version is intentional, as version control is delegated to the task installation. This streamlines the CMake logic.
171-174: Findspdlogwithout static flags: ApprovedUsing
find_package(spdlog REQUIRED)and linkingspdlog::spdlogrelies on the task-generated static library. The removal of custom static-handling logic is consistent with the PR objectives.taskfiles/deps/main.yaml (4)
20-22: IntroduceG_FMT_LIB_NAMEvariable: ApprovedDefining
G_FMT_LIB_NAMEcentralizes the fmt library name for reuse in dependent tasks and CMake settings.
61-63: Addfmtto the core parallel dependency: ApprovedIncluding
fmtincore-all-parallelensures the formatting library is built before other dependencies that rely on it.
65-68: Addspdlogto the core parallel dependency: Approved
spdlognow depends onfmtand is built as part of the core dependency graph. The ordering is correct.
148-161: Definefmtinstallation task: ApprovedThe task for downloading, building, and installing
fmt(v8.0.1) with-DFMT_TEST=OFFis well configured. It matches the CMake settings import and ensures consistent build behaviour.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the fmt and spdlog dependencies to a task-based installation approach to address conflicts with system-installed versions and enable static linking. Key changes include:
- Adding new variables and tasks in the dependency taskfile for fmt and spdlog.
- Removing the outdated Findspdlog.cmake script.
- Updating developer documentation to reflect the new installation tasks.
Reviewed Changes
Copilot reviewed 2 out of 11 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| taskfiles/deps/main.yaml | Introduces fmt and spdlog tasks with remote CMake library installation. |
| docs/src/dev-guide/components-core/index.md | Updates documentation to include fmt and spdlog as dependencies. |
Files not reviewed (9)
- components/core/CMakeLists.txt: Language not supported
- components/core/cmake/Modules/Findspdlog.cmake: Language not supported
- components/core/src/clp_s/CMakeLists.txt: Language not supported
- components/core/tools/scripts/lib_install/centos-stream-9/install-packages-from-source.sh: Language not supported
- components/core/tools/scripts/lib_install/fmtlib.sh: Language not supported
- components/core/tools/scripts/lib_install/macos/install-all.sh: Language not supported
- components/core/tools/scripts/lib_install/spdlog.sh: Language not supported
- components/core/tools/scripts/lib_install/ubuntu-focal/install-packages-from-source.sh: Language not supported
- components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh: Language not supported
Comments suppressed due to low confidence (1)
taskfiles/deps/main.yaml:260
- [nitpick] For consistency and maintainability, consider defining a variable for spdlog (similar to G_FMT_LIB_NAME for fmt) rather than using a hard-coded string.
LIB_NAME: "spdlog"
| cmds: | ||
| - task: "utils:install-remote-cmake-lib" | ||
| vars: | ||
| CMAKE_GEN_ARGS: |
There was a problem hiding this comment.
[nitpick] Explicitly referencing fmt's CMake settings for spdlog may be confusing. Consider adding a clarifying comment to explain that spdlog is configured to use fmt's settings file due to the dependency relationship.
| CMAKE_GEN_ARGS: | |
| CMAKE_GEN_ARGS: | |
| # Use fmt's CMake settings file because spdlog depends on fmt. |
Description
As #720 indicates, building & installing spdlog to the system can end up conflicting with the version already installed on the system. At the same time, we can't always use the version installed on the system, since we want to support statically linking spdlog (& fmt), and the installed versions usually don't have static libraries.
Now that we can install cmake-based libraries through tasks, this PR switches to building and installing, to a local directory, fmt & spdlog.
This PR also:
Removes
Findspdlog.cmakesince the custom script is no longer necessary to control static linking since we always static-link with fmt & spdlog now.Removes the minimum required version from CMakeLists.txt, since it's easier to control the installed versions through the install tasks; moreover, chore(core): lock exact version of spdlog and fmtlib transitive dependency #621 is meant to be a robust way of ensuring compatible versions of fmt & spdlog are installed.
Checklist
breaking change.
Validation performed
ldd <binary>for every core binary doesn't show fmt or spdlog as dynamically linked.Summary by CodeRabbit
Summary by CodeRabbit
fmtandspdloglibraries by removing static linking enforcement and custom CMake modules.fmtandspdlogon Linux and macOS platforms.fmtandspdloginto automated dependency installation via the build system with proper task dependencies.fmtandspdlogas required dependencies.