build(deps): Migrate Boost and msgpack-cxx dependencies to task-based installation (fixes #1243); Drop required Boost version back to 1.81; Add missing ZStd CMake target links. #1254
Conversation
Bill-hbrhbr
requested review from
a team,
gibber9809 and
wraymo
as code owners
WalkthroughUpdates CMake dependency discovery and target linkages to require specific Boost and msgpack-cxx versions, adds Boost::system and msgpack-cxx to multiple targets, removes local Boost/msgpack install scripts and platform installer steps, and introduces Taskfile tasks to build Boost 1.87.0 and msgpack-cxx 7.0.0 into the dependency graph. Changes
Sequence Diagram(s)sequenceDiagram
participant TF as Taskfile: deps (main.yaml)
participant B as Boost build (1.87.0)
participant M as msgpack-cxx build (7.0.0)
participant CMake as CMake configure / core
rect #f0f7ff
TF->>B: run "boost" (download & install)
B-->>TF: provide Boost CMake settings (G_BOOST_LIB_NAME)
end
rect #f7fff0
TF->>M: run "msgpack-cxx" (depends on Boost)
M-->>TF: build/install msgpack-cxx using Boost config and C++20 flags
end
rect #fffaf0
CMake->>B: find_package(Boost 1.81...1.88 REQUIRED)
CMake->>M: find_package(msgpack-cxx 7.0.0 REQUIRED)
CMake-->>CMake: link Boost::system, msgpack-cxx into targets (clp, clg, indexer, reducer, unitTest)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
taskfiles/deps/main.yaml (5)
21-21: Consistent naming for generated Boost settings fileDefining G_BOOST_LIB_NAME as "Boost" aligns with how other libs (fmt/GSL) export their CMake settings files and matches the -C .../{{.G_BOOST_LIB_NAME}}.cmake usage below. LGTM.
67-67: Wiring Boost into core-all-parallel is appropriateAdding boost to core-all-parallel keeps dependency resolution centralized and enables parallel installs. No concerns here given boost is marked internal and run: once.
76-76: Add msgpack-cxx to the build graph — ordering looks correctIncluding msgpack-cxx here is good, and the msgpack task itself depends on boost so there’s no risk of find_package resolution racing. Nice.
390-393: Please manually verify the msgpack-cxx tarball SHA256I attempted to fetch and checksum the
msgpack-cxx-7.0.0.tar.gzasset in the CI sandbox, but required checksum tools and SSL CA certs weren’t available. To avoid CI failures if the upstream archive changes, please run this locally and confirm the hash matches the pinned value:VER="7.0.0" URL="https://github.com/msgpack/msgpack-c/releases/download/cpp-$VER/msgpack-cxx-$VER.tar.gz" EXPECTED_SHA="7504b7af7e7b9002ce529d4f941e1b7fb1fb435768780ce7da4abaac79bb156f" curl -fsSL "$URL" -o msgpack.tar.gz # On Linux: sha256sum msgpack.tar.gz # On macOS: shasum -a 256 msgpack.tar.gzVerify the printed checksum matches the
EXPECTED_SHAabove.
370-389: Update msgpack-cxx options for v7.0.0
In v7.0.0 the CMake cache variables MSGPACK_CXX20 and MSGPACK_USE_BOOST have been removed/renamed. You’ll need to drop those flags and rely on standard CMake target properties (and any new v7-specific options documented in the repo’s README) to select C++20 and enable Boost support.— Remove the obsolete MSGPACK_CXX20 and MSGPACK_USE_BOOST entries
— Add standard CMake settings for C++20 (e.g. CMAKE_CXX_STANDARD)
— (Optional) Add -DCMAKE_INSTALL_MESSAGE=LAZY for quieter install logs, matching other depsProposed diff:
- task: "utils:install-remote-cmake-lib" vars: CMAKE_GEN_ARGS: - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW" - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" + - "-DCMAKE_CXX_STANDARD=20" + - "-DCMAKE_CXX_STANDARD_REQUIRED=ON" - "-DMSGPACK_BUILD_DOCS=OFF" - "-DMSGPACK_BUILD_EXAMPLES=OFF" - "-DMSGPACK_BUILD_TESTS=OFF" - - "-DMSGPACK_CXX20=ON" - - "-DMSGPACK_USE_BOOST=ON" + # Deprecated in v7.0.0: MSGPACK_CXX20 and MSGPACK_USE_BOOST + - "-DCMAKE_INSTALL_MESSAGE=LAZY"Likely an incorrect or invalid review comment.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/src/dev-guide/components-core/index.md (1)
49-50: msgpack-cxx entry looks correct; link matches prior suggestionThe link to cpp_master and version pin v7.0.0 align with the task installation. No changes needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/core/tools/scripts/lib_install/install-boost.sh(0 hunks)components/core/tools/scripts/lib_install/msgpack.sh(0 hunks)docs/src/dev-guide/components-core/index.md(2 hunks)taskfiles/deps/main.yaml(5 hunks)
💤 Files with no reviewable changes (2)
- components/core/tools/scripts/lib_install/install-boost.sh
- components/core/tools/scripts/lib_install/msgpack.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
taskfiles/deps/main.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
taskfiles/deps/main.yaml
📚 Learning: 2025-05-28T18:33:30.155Z
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfiles/deps/main.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (6)
taskfiles/deps/main.yaml (6)
21-21: Good: centralising the Boost settings filename via G_BOOST_LIB_NAMEThis eliminates hard-coded "Boost.cmake" strings in downstream tasks and reduces drift risk.
29-32: Helpful CMP0074 guidance commentThis is the right place for build-policy rationale; keeps future tasks consistent when rooting via _ROOT.
64-81: Wiring boost and msgpack-cxx into core-all-parallel is correct; ordering is safeHaving msgpack-cxx depend on boost plus listing both here ensures reproducible setup without relying on incidental transitive execution. Looks good.
191-211: Confirm Boost tarball URL rendering in YAML remains single-line at runtimeThe double-quoted string with a backslash-newline is consistent with other tasks and should join lines correctly. No change needed; just calling out that this matches house style.
191-211: Ignore pruning Boost.URL
Boost::url is explicitly linked in multiple CMakeLists and is required by our components. Removing it will break the build.• components/core/CMakeLists.txt (line 740): links
Boost::url
• components/core/src/clp_s/CMakeLists.txt (lines 203, 267, 328): each linksBoost::url
• components/core/src/clp_s/indexer/CMakeLists.txt (line 116): linksBoost::url[suggest_nitpick removed]
Likely an incorrect or invalid review comment.
374-395: Checksum verification completeThe SHA-256 checksum for msgpack-cxx v7.0.0 has been validated against the value specified in
taskfiles/deps/main.yamland matches exactly. No further changes are needed.
| | [Boost](https://github.com/boostorg/boost) | v1.87.0 | | ||
| | [Catch2](https://github.com/catchorg/Catch2.git) | v3.8.0 | |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify accepted Boost version range in docs to match CMake constraints
The table shows the task-installed Boost at v1.87.0, but CMake accepts 1.81–1.88. Adding a brief note prevents confusion when users bring their own Boost.
Apply either of the following:
Option A — inline parenthetical in the table cell:
-| [Boost](https://github.com/boostorg/boost) | v1.87.0 |
+| [Boost](https://github.com/boostorg/boost) | v1.87.0 (CMake accepts 1.81–1.88) |Option B — short note immediately below the table:
| [zstd](https://github.com/facebook/zstd) | v1.5.7 |
+Note: The Task-installed Boost default is v1.87.0. The CMake build accepts Boost versions in the range 1.81–1.88.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | [Boost](https://github.com/boostorg/boost) | v1.87.0 | | |
| | [Catch2](https://github.com/catchorg/Catch2.git) | v3.8.0 | | |
| | [Boost](https://github.com/boostorg/boost) | v1.87.0 (CMake accepts 1.81–1.88) | | |
| | [Catch2](https://github.com/catchorg/Catch2.git) | v3.8.0 | |
| | [Boost](https://github.com/boostorg/boost) | v1.87.0 | | |
| | [Catch2](https://github.com/catchorg/Catch2.git) | v3.8.0 | | |
| | [zstd](https://github.com/facebook/zstd) | v1.5.7 | | |
| Note: The Task-installed Boost default is v1.87.0. The CMake build accepts Boost versions in the range 1.81–1.88. |
Bill-hbrhbr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/core/src/clp_s/CMakeLists.txt (1)
200-208: Export zstd transitively from clp_s::io (PUBLIC), or downstreams that include Zstd.hpp will miss zstd includes.*clp_s::io exposes headers like ZstdCompressor.hpp / ZstdDecompressor.hpp via PUBLIC include dirs (Line 199). Those headers include zstd.h. Because ${zstd_TARGET} is currently linked as PRIVATE, consumers such as clp_s::archive_reader and clp_s::archive_writer (which link PUBLIC to clp_s::io) won’t receive zstd include directories/transitive usage, and will fail when they include io’s Zstd*.hpp in public headers (e.g., ArchiveReaderAdaptor.hpp includes ZstdDecompressor.hpp). Promote ${zstd_TARGET} to PUBLIC here.
Apply this minimal change:
target_link_libraries( clp_s_io - PRIVATE + PUBLIC + ${zstd_TARGET} + PRIVATE Boost::iostreams Boost::url clp_s::clp_dependencies fmt::fmt spdlog::spdlog - ${zstd_TARGET} )To confirm where zstd-dependent headers are pulled in, run:
#!/bin/bash # Show public headers that include Zstd*.hpp or zstd.h rg -n -C2 -g 'components/core/src/**' -P '^\s*#\s*include\s+["<].*(Zstd(?:De)?Compressor\.hpp|zstd\.h)[">]'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
-
components/core/src/clp_s/CMakeLists.txt(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (2)
components/core/src/clp_s/CMakeLists.txt (2)
21-21: Whitespace tidy-ups — looks good.Trailing-space removals in the CLP_S_CLP_SOURCES list are fine and reduce churn in future diffs.
Also applies to: 72-72, 80-80
112-118: Making ${zstd_TARGET} PUBLIC on clp_s_clp_dependencies is correct.Headers under clp_s_clp_dependencies (e.g., streaming_compression/zstd/*) transitively include zstd.h; promoting zstd to PUBLIC ensures downstreams compile when they include those headers.
| clp_s_json_constructor | ||
| PUBLIC | ||
| clp_s::archive_reader | ||
| ${zstd_TARGET} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Adding ${zstd_TARGET} as PUBLIC on clp_s::json_constructor: OK, but ensure the build option reflects this.
Linking zstd as PUBLIC here is acceptable if any public header (directly or indirectly) requires zstd.h. Regardless, because zstd is now a required dependency for this target, verify that CLP_NEED_ZSTD has been added to set_clp_s_json_constructor_dependencies in options.cmake as planned in this migration.
Use this quick check:
🏁 Script executed:
#!/bin/bash
# Locate the dependency setter and verify CLP_NEED_ZSTD is included
rg -nP 'function\s*\(\s*set_clp_s_json_constructor_dependencies' -C3
rg -n 'CLP_NEED_ZSTD' -C2 --glob '*.cmake' --glob '**/CMakeLists.txt'Length of output: 2941
Ensure CLP_NEED_ZSTD is added to set_clp_s_json_constructor_dependencies
The set_clp_s_json_constructor_dependencies function in components/core/cmake/Options/options.cmake currently only sets:
function(set_clp_s_json_constructor_dependencies)
set_clp_need_flags(
CLP_NEED_FMT
CLP_NEED_MONGOCXX
)
endfunction()Since ${zstd_TARGET} has been added as a PUBLIC dependency of clp_s_json_constructor, you must also expose it via the CLP_NEED_ZSTD option flag. Please update the function as follows:
--- a/components/core/cmake/Options/options.cmake
+++ b/components/core/cmake/Options/options.cmake
@@ -282,6 +282,7 @@ function(set_clp_s_json_constructor_dependencies)
set_clp_need_flags(
CLP_NEED_FMT
CLP_NEED_MONGOCXX
+ CLP_NEED_ZSTD
)
endfunction()• File: components/core/cmake/Options/options.cmake
• Function: set_clp_s_json_constructor_dependencies (around line 282)
Once this is in place, the public zstd dependency and corresponding build option will be properly wired.
🤖 Prompt for AI Agents
In components/core/cmake/Options/options.cmake around line 282, the function
set_clp_s_json_constructor_dependencies defines needed flags but omits
CLP_NEED_ZSTD; update the function to call set_clp_need_flags with CLP_NEED_FMT,
CLP_NEED_MONGOCXX and also CLP_NEED_ZSTD so the public ${zstd_TARGET} dependency
is exposed via the build option for clp_s_json_constructor.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
build(deps): Migrate Boost and msgpack-cxx dependencies to task-based installation (fixes #1243); Drop required Boost version back to 1.81; Add missing ZStd CMake target links.
2 participants
Description
As explained in #1243, we are standardizing on using Task to install Boost. Since
msgpack-cxxdepends on Boost being available, its installation is now switched to Task as well to preserve the correct dependency order.In addition, the Boost version is downgraded from
1.89to1.87, with an accepted version range of1.81–1.88in cmake builds. This avoids the breaking change introduced in Boost1.89, whereBoost::systemwas converted from a library to a header-only target (see #1223).Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Refactor
Chores
Tests
Documentation