refactor(build): Remove CLP_NEEDS_X flags for marking required dependencies.
#1107
Conversation
…ies in functions in cmake/Utils/utils.cmake.
WalkthroughCentralized CMake dependency discovery into a new components/core/cmake/find_utils.cmake; replaced previous per-file find_package logic with clp_find_* helpers; simplified options validation (renamed aggregator); adjusted module include paths to PROJECT_SOURCE_DIR; added default Release build-type fallback and optional IPO; added discovery calls to many CMakeLists before target creation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CMake as CMake (configure)
participant Options as options.cmake
participant FindUtils as find_utils.cmake
participant Component as Component CMakeLists
participant Pkg as find_package / FindModules
Dev->>CMake: run cmake
CMake->>Options: validate_all_clp_dependency_flags()
Note right of Options: validates only enabled CLP_BUILD_* groups
CMake->>FindUtils: include(find_utils.cmake)
loop per component or target
Component->>FindUtils: call clp_find_*() or find_package()
FindUtils->>FindUtils: check clp_checked_find_<LIB> (one‑time guard)
alt not yet found
FindUtils->>Pkg: find_package(...) / custom locate logic
Pkg-->>FindUtils: targets/vars
FindUtils-->>Component: expose targets/vars (and static/shared config)
else already resolved
FindUtils-->>Component: reuse prior result
end
Component->>Component: add_library/add_executable + target_link_libraries(...)
end
CMake-->>Dev: configured build system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ 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 (
|
…forward results of find_package calls.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
components/core/src/clp/clo/CMakeLists.txt (1)
155-165: Same duplication remark as in clgThe dependency-resolution block can be factored into a helper macro to avoid repetition.
components/core/src/clp/clp/CMakeLists.txt (1)
169-180: Repeated dependency blocksSee earlier comment; consider consolidating the
clp_find_*sequence into a shared macro.
| find_package(absl REQUIRED) | ||
| find_package(simdjson REQUIRED) | ||
| find_package(spdlog REQUIRED) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use CONFIG look-ups for all three external deps for consistency
Abseil, simdjson, and spdlog all export config files; switch to CONFIG mode so the build doesn’t waste time on the find-module path and so that an unintended FindXXX.cmake can’t shadow the official config.
- find_package(absl REQUIRED)
- find_package(simdjson REQUIRED)
- find_package(spdlog REQUIRED)
+ find_package(absl CONFIG REQUIRED)
+ find_package(simdjson CONFIG REQUIRED)
+ find_package(spdlog CONFIG REQUIRED)📝 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.
| find_package(absl REQUIRED) | |
| find_package(simdjson REQUIRED) | |
| find_package(spdlog REQUIRED) | |
| find_package(absl CONFIG REQUIRED) | |
| find_package(simdjson CONFIG REQUIRED) | |
| find_package(spdlog CONFIG REQUIRED) |
🤖 Prompt for AI Agents
In components/core/src/clp_s/search/CMakeLists.txt around lines 38 to 41, the
find_package calls for absl, simdjson, and spdlog should use CONFIG mode for
consistency and reliability. Modify each find_package call to include the CONFIG
option, ensuring the build system uses the official config files directly and
avoids searching the find-module path or picking up unintended FindXXX.cmake
files.
| clp_find_boost() | ||
| find_package(date REQUIRED) | ||
| find_package(fmt REQUIRED) | ||
| find_package(log_surgeon REQUIRED) | ||
| find_package(spdlog REQUIRED) | ||
| clp_find_sqlite() | ||
| clp_find_libarchive() | ||
| clp_find_mariadb_client() | ||
| find_package(nlohmann_json REQUIRED) | ||
| find_package(yaml-cpp REQUIRED) | ||
| clp_find_ystdlib() | ||
| clp_find_zstd() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor readability tweak – group find_package next to use
Putting clp_find_libarchive() immediately above the target_link_libraries call that references LibArchive::LibArchive makes the dependency chain easier to follow. No functional change, purely a clarity win.
🤖 Prompt for AI Agents
In components/core/src/clp/clp/CMakeLists.txt between lines 169 and 180, move
the clp_find_libarchive() call to be immediately above the target_link_libraries
call that uses LibArchive::LibArchive. This groups the find_package and clp_find
calls closer to where their targets are linked, improving readability and making
the dependency chain easier to follow without changing functionality.
| if(CLP_BUILD_EXECUTABLES) | ||
| find_package(absl REQUIRED) | ||
| clp_find_boost() | ||
| clp_find_curl() | ||
| find_package(date REQUIRED) | ||
| clp_find_mariadb_client() | ||
| find_package(nlohmann_json REQUIRED) | ||
| find_package(OpenSSL REQUIRED) | ||
| find_package(simdjson REQUIRED) | ||
| find_package(spdlog REQUIRED) | ||
| find_package(yaml-cpp REQUIRED) | ||
| clp_find_ystdlib() | ||
| clp_find_zstd() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer imported target over ${CURL_LIBRARIES}
FindCURL exports CURL::libcurl, which automatically propagates include directories and required compile definitions. Using the imported target is less error-prone than a raw variable:
target_link_libraries(indexer PRIVATE CURL::libcurl …)🤖 Prompt for AI Agents
In components/core/src/clp_s/indexer/CMakeLists.txt around lines 83 to 95, the
code currently uses the raw variable `${CURL_LIBRARIES}` for linking curl, which
is error-prone. Replace this with the imported target `CURL::libcurl` in the
`target_link_libraries` call for the indexer target to ensure proper propagation
of include directories and compile definitions.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (6)
components/core/src/clp_s/search/CMakeLists.txt (1)
30-33: Switch absl/simdjson/spdlog to CONFIG mode for consistent, reliable discoveryThese libraries export CMake config packages; CONFIG avoids accidental Find-module shadowing and unnecessary searches.
Apply this diff:
- find_package(absl REQUIRED) + find_package(absl CONFIG REQUIRED) - find_package(log_surgeon REQUIRED) + find_package(log_surgeon CONFIG REQUIRED) - find_package(simdjson REQUIRED) + find_package(simdjson CONFIG REQUIRED) - find_package(spdlog REQUIRED) + find_package(spdlog CONFIG REQUIRED)components/core/src/reducer/CMakeLists.txt (1)
42-47: Prefer CONFIG mode for fmt (and align others for consistency)fmt almost always installs a config package; CONFIG improves portability. Align spdlog/nlohmann_json likewise for consistency across the repo.
Apply this diff:
- find_package(fmt REQUIRED) + find_package(fmt CONFIG REQUIRED) - find_package(nlohmann_json REQUIRED) + find_package(nlohmann_json CONFIG REQUIRED) - find_package(spdlog REQUIRED) + find_package(spdlog CONFIG REQUIRED)components/core/src/clp_s/indexer/CMakeLists.txt (1)
126-140: Link against CURL::libcurl and add OpenSSL::SSL explicitly
- Using CURL::libcurl propagates include dirs and compile defs correctly; raw ${CURL_LIBRARIES} is brittle.
- Many TLS paths require OpenSSL::SSL in addition to Crypto; relying on transitive linkage from libcurl is fragile.
Apply this diff:
absl::flat_hash_map Boost::iostreams Boost::program_options Boost::url - ${CURL_LIBRARIES} + CURL::libcurl clp::string_utils date::date MariaDBClient::MariaDBClient nlohmann_json::nlohmann_json - OpenSSL::Crypto + OpenSSL::SSL OpenSSL::Crypto simdjson::simdjson spdlog::spdlog ystdlib::containers ${zstd_TARGET}components/core/src/clp/clg/CMakeLists.txt (1)
133-145: Reduce boilerplate via a helper macro (repeat)Same suggestion as discussed previously: refactor the repeated clp_find_*/find_package block into a single helper to cut duplication and ease future maintenance.
components/core/src/clp/clp/CMakeLists.txt (1)
178-183: Minor readability: group find next to use (repeat)Consider moving clp_find_libarchive() closer to the target_link_libraries that references LibArchive::LibArchive to keep discovery and usage adjacent. Purely a readability tweak.
components/core/src/glt/glt/CMakeLists.txt (1)
175-186: Refactor repeated discovery boilerplate (repeat)Same optional refactor as elsewhere: extract the repeated dependency discovery calls into a shared helper to simplify future edits.
📜 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 (11)
-
components/core/CMakeLists.txt(2 hunks) -
components/core/cmake/find_utils.cmake(1 hunks) -
components/core/src/clp/clg/CMakeLists.txt(1 hunks) -
components/core/src/clp/clo/CMakeLists.txt(1 hunks) -
components/core/src/clp/clp/CMakeLists.txt(1 hunks) -
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt(1 hunks) -
components/core/src/clp_s/CMakeLists.txt(8 hunks) -
components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks) -
components/core/src/clp_s/search/CMakeLists.txt(1 hunks) -
components/core/src/glt/glt/CMakeLists.txt(1 hunks) -
components/core/src/reducer/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
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: 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
📚 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/indexer/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/CMakeLists.txt
📚 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/indexer/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/reducer/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/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/search/CMakeLists.txtcomponents/core/src/reducer/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 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:
components/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/reducer/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/CMakeLists.txt
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/CMakeLists.txt
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
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). (5)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (17)
components/core/cmake/find_utils.cmake (3)
6-12: Scoped “checked find” guard looks solidUsing a directory property on PROJECT_SOURCE_DIR to memoize dependency discovery is a clean approach and avoids global variable clutter. The parent-scope return of CLP_CHECKED_FIND is correct.
16-18: Good one-time execution guard setterset_property on the project directory keeps the guard centralised and consistent with the getter.
120-127: zstd target selection via zstd_TARGET is consistent with consumersThe version pin and target aliasing to zstd::libzstd_{static,shared} align with how downstream CMakeLists link via ${zstd_TARGET}.
components/core/src/clp/clo/CMakeLists.txt (1)
159-171: Dependency discovery aligns with linked targetsThe added clp_find_*/find_package calls match the libraries linked later (absl, Boost, date, fmt, log_surgeon, mongocxx, msgpack, nlohmann_json, spdlog, SQLite, ystdlib, zstd). No mismatches detected.
components/core/src/glt/glt/CMakeLists.txt (1)
175-186: LGTM — discovery matches linkingThe dependency resolution calls correspond to the linked targets (Boost, date, fmt, spdlog, SQLite, LibArchive, MariaDBClient, nlohmann_json, yaml-cpp, ystdlib, zstd). Looks consistent.
components/core/src/clp_s/CMakeLists.txt (9)
106-116: Discovery set is consistent with links for clp_s_clp_dependenciesBoost, Curl, fmt, log_surgeon, msgpack, nlohmann_json, OpenSSL, spdlog, ystdlib, and zstd are discovered here and linked appropriately (PUBLIC/PRIVATE) later. No mismatches noted.
168-169: Reducer deps: minimal and correctnlohmann_json is the only REQUIRED dependency and is linked PUBLIC as expected. Private dependency on clp_s::clp_dependencies is appropriate.
Also applies to: 176-182
206-210: IO library: discovery matches usageBoost (iostreams, url), fmt, spdlog, and zstd are correctly discovered and linked. No gaps found.
Also applies to: 219-226
269-277: Archive writer: consistent dependency setabsl, Boost::url, Curl, fmt, msgpack, nlohmann_json, simdjson, and spdlog discovery aligns with the link list. PUBLIC/PRIVATE scoping looks reasonable.
Also applies to: 286-300
339-346: Archive reader: consistent dependency setabsl, Boost::url, Curl, fmt, msgpack, nlohmann_json, spdlog discovery and linking are aligned. Good separation of PUBLIC/PRIVATE.
Also applies to: 355-369
381-384: JSON constructor: OK, but double-check mongocxx transitive depsDiscovery and linking look correct. Ensure ${MONGOCXX_TARGET} encapsulates its transitive requirements on your supported platforms (esp. SSL/SASL) so consumers don’t need to add extras.
Also applies to: 392-400
413-415: Timestamp pattern: minimal and correctdate and spdlog are discovered and linked as expected; scoping is clean.
Also applies to: 423-429
445-450: clp-s executable: discovery matches usageBoost, mongocxx, msgpack, spdlog, ystdlib are discovered, and the linked targets align. No missing finds.
Also applies to: 458-474
106-116: Consider using imported targets for Curl (if available)If clp_find_curl() provides an imported target (e.g., CURL::libcurl), prefer linking that over ${CURL_LIBRARIES} for better transitive dependency handling. If your helper intentionally standardizes on ${CURL_LIBRARIES}, ignore this.
Also applies to: 123-139
⛔ Skipped due to learnings
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.Learnt from: Bill-hbrhbr PR: y-scope/clp#1184 File: taskfiles/deps/main.yaml:0-0 Timestamp: 2025-08-13T19:58:26.058Z Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.Learnt from: Bill-hbrhbr PR: y-scope/clp#1124 File: components/core/cmake/Modules/FindBZip2.cmake:60-60 Timestamp: 2025-07-24T21:56:05.806Z Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.Learnt from: Bill-hbrhbr PR: y-scope/clp#1124 File: components/core/CMakeLists.txt:167-170 Timestamp: 2025-08-04T17:26:17.098Z Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.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.components/core/CMakeLists.txt (3)
536-556: LGTM: Dependency discovery colocated with the test target and helper wrappers.This is consistent with the new approach and keeps the top-level concise. Also aligns with the learning to avoid redundant checks when using REQUIRED find calls.
15-18: Legacy helpers retired and new validator in place
Confirmed thatvalidate_all_clp_dependency_flags()is called at line 15 and there are no remaining references tovalidate_and_setup_all_clp_dependency_flagsorconvert_clp_dependency_properties_to_variables. All prior helpers have been fully retired.
536-556: Link yaml-cpp in unitTest or drop the redundant findThe
find_package(yaml-cpp REQUIRED)at components/core/CMakeLists.txt:553 isn’t paired with ayaml-cpp::yaml-cppentry in thetarget_link_libraries(unitTest…)block (around line 566). Either:• If any test sources pull in yaml-cpp symbols (e.g. components/core/src/glt/GlobalMetadataDBConfig.cpp includes
<yaml-cpp/yaml.h>), add it:target_link_libraries(unitTest PRIVATE … yaml-cpp::yaml-cpp )• Otherwise, remove the now-unused
find_package(yaml-cpp REQUIRED)at line 553.To confirm which path applies, search for yaml-cpp usage in your test sources:
rg -n '#include <yaml-cpp/' components/core/src
| set(sqlite_DYNAMIC_LIBS "dl;m;pthread") | ||
| include("${PROJECT_SOURCE_DIR}/cmake/Modules/FindLibraryDependencies.cmake") | ||
| FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") | ||
| endmacro() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make sqlite dependency discovery portable (dl/m/pthread vary by OS)
Hard-coding "dl;m;pthread" is Linux-centric and may break or be redundant on macOS and Windows. Prefer CMake’s helpers and per-platform guards.
Apply this diff to improve portability while keeping your current approach:
- set(sqlite_DYNAMIC_LIBS "dl;m;pthread")
+ # Compose a portable default list:
+ # - CMAKE_DL_LIBS resolves to 'dl' on Linux and is empty on macOS/Windows.
+ # - 'm' is needed on UNIX; Windows doesn't use it.
+ # - pthreads should be discovered via FindThreads.
+ set(sqlite_DYNAMIC_LIBS "")
+ if(UNIX)
+ list(APPEND sqlite_DYNAMIC_LIBS "${CMAKE_DL_LIBS}")
+ list(APPEND sqlite_DYNAMIC_LIBS "m")
+ find_package(Threads REQUIRED)
+ if(TARGET Threads::Threads)
+ # FindDynamicLibraryDependencies expects raw library names; Threads::Threads may expand transitively.
+ # If your helper supports imported targets, prefer that; otherwise fall back to 'pthread' on POSIX.
+ list(APPEND sqlite_DYNAMIC_LIBS "pthread")
+ endif()
+ endif()If you’re open to simplifying further, an alternative is to use find_package(SQLite3 REQUIRED) and the SQLite::SQLite3 imported target (and let its transitive requirements propagate), but that would be a larger behavioural change.
📝 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.
| set(sqlite_DYNAMIC_LIBS "dl;m;pthread") | |
| include("${PROJECT_SOURCE_DIR}/cmake/Modules/FindLibraryDependencies.cmake") | |
| FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") | |
| endmacro() | |
| # Compose a portable default list: | |
| # - CMAKE_DL_LIBS resolves to 'dl' on Linux and is empty on macOS/Windows. | |
| # - 'm' is needed on UNIX; Windows doesn't use it. | |
| # - pthreads should be discovered via FindThreads. | |
| set(sqlite_DYNAMIC_LIBS "") | |
| if(UNIX) | |
| list(APPEND sqlite_DYNAMIC_LIBS "${CMAKE_DL_LIBS}") | |
| list(APPEND sqlite_DYNAMIC_LIBS "m") | |
| find_package(Threads REQUIRED) | |
| if(TARGET Threads::Threads) | |
| # FindDynamicLibraryDependencies expects raw library names; Threads::Threads may expand transitively. | |
| # If your helper supports imported targets, prefer that; otherwise fall back to 'pthread' on POSIX. | |
| list(APPEND sqlite_DYNAMIC_LIBS "pthread") | |
| endif() | |
| endif() | |
| include("${PROJECT_SOURCE_DIR}/cmake/Modules/FindLibraryDependencies.cmake") | |
| FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") | |
| endmacro() |
🤖 Prompt for AI Agents
In components/core/cmake/find_utils.cmake around lines 99-102, the code
hard-codes "dl;m;pthread" which is Linux-specific and not portable; replace the
literal list with platform-aware checks and CMake helpers: use the built-in
CMAKE_DL_LIBS for dynamic loader libs where available, use find_package(Threads
REQUIRED) and add Threads::Threads for threading instead of "pthread", and guard
Windows/Apple paths with if(WIN32)/elseif(APPLE)/elseif(UNIX) to only append
appropriate libs; alternatively prefer using find_package(SQLite3 REQUIRED) and
its SQLite::SQLite3 target if you want to simplify dependency propagation.
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (9)
components/core/src/reducer/CMakeLists.txt (1)
43-43: Switch fmt to CONFIG mode for portability (repeat)Use the package’s CMake config when available to avoid module-vs-config mismatch issues.
- find_package(fmt REQUIRED) + find_package(fmt CONFIG REQUIRED)components/core/src/clp_s/indexer/CMakeLists.txt (1)
111-123: Use CONFIG where available; link curl via imported target; add OpenSSL::SSL (repeat)
- CONFIG mode: absl/date/nlohmann_json/simdjson/spdlog/yaml-cpp all provide config packages.
- Link: prefer CURL::libcurl over ${CURL_LIBRARIES}.
- TLS: add OpenSSL::SSL alongside OpenSSL::Crypto to avoid relying on libcurl’s transitive linkage. Past reviews raised the same points.
- find_package(absl REQUIRED) + find_package(absl CONFIG REQUIRED) clp_find_boost() clp_find_curl() - find_package(date REQUIRED) + find_package(date CONFIG REQUIRED) clp_find_mariadb_client() - find_package(nlohmann_json REQUIRED) + find_package(nlohmann_json CONFIG REQUIRED) find_package(OpenSSL REQUIRED) - find_package(simdjson REQUIRED) - find_package(spdlog REQUIRED) - find_package(yaml-cpp REQUIRED) + find_package(simdjson CONFIG REQUIRED) + find_package(spdlog CONFIG REQUIRED) + find_package(yaml-cpp CONFIG REQUIRED) clp_find_ystdlib() clp_find_zstd() @@ - ${CURL_LIBRARIES} + CURL::libcurl @@ - OpenSSL::Crypto + OpenSSL::SSL OpenSSL::CryptoPlease double-check CI images whose libcurl may be built against non-OpenSSL backends; explicitly linking OpenSSL is safe here because the build already requires OpenSSL. If any environment intentionally excludes OpenSSL, drop both the find_package(OpenSSL) and SSL/Crypto targets.
Also applies to: 126-141
components/core/src/clp/clp/CMakeLists.txt (2)
181-181: Remove unused yaml-cpp discovery or link it (repeat)yaml-cpp is REQUIRED but not linked to clp; this can fail builds unnecessarily. Either remove the find line or add yaml-cpp to target_link_libraries.
- find_package(yaml-cpp REQUIRED) + # yaml-cpp not used by clp; drop requirement
178-178: Minor readability: place clp_find_libarchive near its usage (repeat)Keeping discovery adjacent to the tll block that links LibArchive clarifies dependency flow. No functional change.
Also applies to: 191-201
components/core/src/clp/clg/CMakeLists.txt (1)
133-145: Use CONFIG mode where available; drop yaml-cpp unless used (repeat on yaml-cpp)Aligns with other targets and reduces spurious “module not found” issues.
- find_package(absl REQUIRED) + find_package(absl CONFIG REQUIRED) clp_find_boost() - find_package(date REQUIRED) + find_package(date CONFIG REQUIRED) - find_package(fmt REQUIRED) + find_package(fmt CONFIG REQUIRED) - find_package(log_surgeon REQUIRED) + find_package(log_surgeon CONFIG REQUIRED) clp_find_mariadb_client() - find_package(nlohmann_json REQUIRED) - find_package(spdlog REQUIRED) + find_package(nlohmann_json CONFIG REQUIRED) + find_package(spdlog CONFIG REQUIRED) clp_find_sqlite() - find_package(yaml-cpp REQUIRED) + # yaml-cpp not linked by clg; remove unless actually needed clp_find_ystdlib() clp_find_zstd()components/core/src/clp_s/CMakeLists.txt (1)
106-116: Reduce boilerplate: wrap repeated discovery into a helper (repeat)The same find/clp_find sequences recur across targets. A small helper (e.g., clp_s_resolve_common_deps()) would centralise this and simplify future edits.
I can draft a minimal macro in components/core/cmake/find_utils.cmake and replace the repeated blocks across this file in a follow-up commit.
Also applies to: 205-210, 269-277, 339-346, 381-385, 415-417, 447-452
components/core/CMakeLists.txt (1)
536-556: Narrow OpenSSL requirement to the Crypto component only.You only link
OpenSSL::Crypto, so don’t require the full OpenSSL package (which can unnecessarily forceSSL). This keeps environments withoutSSLviable.- find_package(OpenSSL REQUIRED) + find_package(OpenSSL COMPONENTS Crypto REQUIRED)components/core/cmake/find_utils.cmake (2)
65-73: Avoid overriding LibLZMA_ROOT with unset hyphenated variables; rely on the Find module’s CLP_CORE_DEPS_DIR-first logic.
LibLZMA-static_ROOT/LibLZMA-shared_ROOTaren’t defined universally; unconditionally settingLibLZMA_ROOTto empty degrades discovery. Prefer only settingLibLZMA_USE_STATIC_LIBSand honour explicit overrides if the per-mode roots are actually provided. This aligns with the project convention that FindLibLZMA.cmake prioritisesCLP_CORE_DEPS_DIR.macro(clp_find_lzma) - if(CLP_USE_STATIC_LIBS) - set(LibLZMA_ROOT ${LibLZMA-static_ROOT}) - set(LibLZMA_USE_STATIC_LIBS ON) - else() - set(LibLZMA_ROOT ${LibLZMA-shared_ROOT}) - endif() + if(CLP_USE_STATIC_LIBS) + set(LibLZMA_USE_STATIC_LIBS ON) + endif() + # Only honour explicit overrides if provided; otherwise rely on the Find module’s defaults. + if(CLP_USE_STATIC_LIBS AND NOT "${LibLZMA-static_ROOT}" STREQUAL "") + set(LibLZMA_ROOT "${LibLZMA-static_ROOT}") + elseif(NOT CLP_USE_STATIC_LIBS AND NOT "${LibLZMA-shared_ROOT}" STREQUAL "") + set(LibLZMA_ROOT "${LibLZMA-shared_ROOT}") + endif() # Version 5.8.1 and above address CVE-2024-3094 and CVE-2025-31115. find_package(LibLZMA 5.8.1 REQUIRED) endmacro()
109-112: Make SQLite’s dependency list portable (dl/m/pthread vary by OS).Hard-coding
"dl;m;pthread"is Linux-centric. Compose the list via CMake’s helpers and per-platform guards.macro(clp_find_sqlite) - set(sqlite_DYNAMIC_LIBS "dl;m;pthread") + # Compose a portable default list for SQLite’s transitive deps: + # - CMAKE_DL_LIBS resolves to 'dl' on Linux and is empty on macOS/Windows. + # - 'm' is needed on UNIX; Windows doesn’t use it. + # - Threads should be discovered via FindThreads. + set(sqlite_DYNAMIC_LIBS "") + if(UNIX) + list(APPEND sqlite_DYNAMIC_LIBS "${CMAKE_DL_LIBS}") + list(APPEND sqlite_DYNAMIC_LIBS "m") + find_package(Threads REQUIRED) + # If FindDynamicLibraryDependencies only accepts raw names, fall back to 'pthread'. + list(APPEND sqlite_DYNAMIC_LIBS "pthread") + endif() include("${PROJECT_SOURCE_DIR}/cmake/Modules/FindLibraryDependencies.cmake") FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") endmacro()
📜 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 (7)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/find_utils.cmake(1 hunks)components/core/src/clp/clg/CMakeLists.txt(1 hunks)components/core/src/clp/clp/CMakeLists.txt(1 hunks)components/core/src/clp_s/CMakeLists.txt(8 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks)components/core/src/reducer/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
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: 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.
📚 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/reducer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/indexer/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/reducer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/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/reducer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 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:
components/core/src/reducer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmake
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmake
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmake
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/find_utils.cmake
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-08-25T07:27:19.449Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.
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). (7)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (3)
components/core/CMakeLists.txt (1)
15-18: Order of dependency validation and helper include looks correct.
validate_all_clp_dependency_flags()runs before includingfind_utils.cmake, andCLP_USE_STATIC_LIBSis read at macro invocation time (later in the file), so defining it after the include is safe.components/core/cmake/find_utils.cmake (2)
6-12: One-time discovery guard is solid.Using a directory property on
${PROJECT_SOURCE_DIR}ensures cross-subdirectory idempotence, and returningCLP_CHECKED_FINDviaPARENT_SCOPEis appropriate.Also applies to: 16-18
106-113: Expose SQLite include/source directories in clp_find_sqlite macroThe
clp_find_sqlitemacro currently only sets up the dynamic libraries viaFindDynamicLibraryDependenciesand does not define any header search path or source‐file directory. Although the CI pipeline generatesset(CLP_SQLITE3_SOURCE_DIRECTORY …)andset(CLP_SQLITE3_INCLUDE_DIRECTORY …)into per‐dependency.cmakefiles under${G_DEPS_CORE_CMAKE_SETTINGS_DIR}, I could not find anyinclude(...)calls that actually pull those settings into the build beforeclp_find_sqliteis used. As a result, downstream targets (e.g.unitTest,glt,clo,clg,clp) may reference${CLP_SQLITE3_INCLUDE_DIRECTORY}and${CLP_SQLITE3_SOURCE_DIRECTORY}without them ever being defined.• components/core/cmake/find_utils.cmake
– Extend theclp_find_sqlitemacro to also locate headers and the amalgam source; for example:
cmake include("${G_DEPS_CORE_CMAKE_SETTINGS_DIR}/SQLite3.cmake" OPTIONAL) if(NOT DEFINED CLP_SQLITE3_INCLUDE_DIRECTORY) find_package(SQLite3 REQUIRED) set(CLP_SQLITE3_INCLUDE_DIRECTORY ${SQLite3_INCLUDE_DIRS}) set(CLP_SQLITE3_SOURCE_DIRECTORY "${SQLite3_INCLUDE_DIR}/../src") endif() target_include_directories(<caller> PRIVATE ${CLP_SQLITE3_INCLUDE_DIRECTORY} )• Top-level CMakeLists.txt (or wherever
G_DEPS_CORE_CMAKE_SETTINGS_DIRis set)
– Ensure the generated dependency-settings.cmakefiles (includingSQLite3.cmake) are included early in the configure step so thatCLP_SQLITE3_*variables are always defined before anyclp_find_sqliteinvocation.• components/core/CMakeLists.txt
– Verify that${CLP_SQLITE3_INCLUDE_DIRECTORY}and${CLP_SQLITE3_SOURCE_DIRECTORY}are defined (via the generated settings or via a fallbackfind_package) before being used intarget_include_directoriesand when compiling the amalgam.
| macro(clp_find_curl) | ||
| find_package(CURL 7.61.1 REQUIRED) | ||
| endmacro() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: encourage consumers to prefer CURL::libcurl.
find_package(CURL ...) is fine. For consistency across static/shared builds, downstream target_link_libraries should favour the imported target CURL::libcurl over ${CURL_LIBRARIES}.
🤖 Prompt for AI Agents
In components/core/cmake/find_utils.cmake around lines 48 to 50, the macro calls
find_package(CURL 7.61.1 REQUIRED) but downstream consumers may still use
${CURL_LIBRARIES}; update the macro to encourage/ensure use of the imported
target CURL::libcurl by, after find_package succeeds, prefer and export the
imported target (e.g., set a variable pointing to CURL::libcurl or document that
consumers should link to CURL::libcurl) and only fall back to ${CURL_LIBRARIES}
if the imported target is not available; make sure any helper variables or
documentation in the macro clearly instruct target_link_libraries(... PRIVATE
CURL::libcurl) as the preferred usage.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
components/core/src/clp/clg/CMakeLists.txt (2)
143-144: Remove unused yaml-cpp discovery (not linked).yaml-cpp is REQUIRED here but never linked into clg, creating an unnecessary hard requirement and potential configure failure on machines without yaml-cpp.
Apply this diff to drop the unused find:
- find_package(yaml-cpp REQUIRED)
132-146: Reduce repeated dependency-resolution boilerplate across executables.This block mirrors other targets; a small helper (e.g., clp_resolve_common_deps()) would cut duplication and ease future edits.
components/core/src/clp/clp/CMakeLists.txt (3)
182-183: Remove unused yaml-cpp discovery (not linked).yaml-cpp is REQUIRED but not linked into the clp target. This can cause needless build failures in environments lacking yaml-cpp.
Apply this diff to remove the unused find:
- find_package(yaml-cpp REQUIRED)
171-184: Prefer CONFIG mode for deps that provide CMake package configs.Helps ensure we pick the intended task-installed packages rather than ad-hoc Find modules.
Apply this diff:
- find_package(absl REQUIRED) + find_package(absl CONFIG REQUIRED) clp_find_boost() - find_package(date REQUIRED) - find_package(fmt REQUIRED) + find_package(date CONFIG REQUIRED) + find_package(fmt CONFIG REQUIRED) clp_find_libarchive() - find_package(log_surgeon REQUIRED) + find_package(log_surgeon CONFIG REQUIRED) clp_find_mariadb_client() clp_find_msgpack() - find_package(nlohmann_json REQUIRED) - find_package(spdlog REQUIRED) + find_package(nlohmann_json CONFIG REQUIRED) + find_package(spdlog CONFIG REQUIRED) clp_find_sqlite() - find_package(yaml-cpp REQUIRED) clp_find_ystdlib() clp_find_zstd()
175-180: Minor readability: group discovery next to usage.Optionally move clp_find_libarchive() immediately above target_link_libraries where LibArchive::LibArchive is used to keep discovery close to linkage.
components/core/CMakeLists.txt (1)
536-557: Tighten OpenSSL requirement to Crypto component and drop unused yaml-cpp.
- unitTest links OpenSSL::Crypto only; scoping find_package to COMPONENTS Crypto avoids pulling in full OpenSSL.
- yaml-cpp is REQUIRED here but not linked in unitTest, creating an avoidable hard dependency.
Apply this diff:
- find_package(OpenSSL REQUIRED) + find_package(OpenSSL COMPONENTS Crypto REQUIRED) @@ - find_package(yaml-cpp REQUIRED)Optionally standardise to CONFIG mode for other deps that ship configs:
- find_package(absl REQUIRED) + find_package(absl CONFIG REQUIRED) @@ - find_package(date REQUIRED) - find_package(fmt REQUIRED) + find_package(date CONFIG REQUIRED) + find_package(fmt CONFIG REQUIRED) @@ - find_package(log_surgeon REQUIRED) + find_package(log_surgeon CONFIG REQUIRED) @@ - find_package(nlohmann_json REQUIRED) + find_package(nlohmann_json CONFIG REQUIRED) @@ - find_package(simdjson REQUIRED) - find_package(spdlog REQUIRED) + find_package(simdjson CONFIG REQUIRED) + find_package(spdlog CONFIG REQUIRED)
📜 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 (3)
components/core/CMakeLists.txt(2 hunks)components/core/src/clp/clg/CMakeLists.txt(1 hunks)components/core/src/clp/clp/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
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: 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
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.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
📚 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/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/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/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/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/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 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:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/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). (6)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (2)
components/core/CMakeLists.txt (2)
15-19: LGTM – Centralised validation and discovery include order looks correct.validate_all_clp_dependency_flags() before including find_utils.cmake is consistent with options-first, utils-next initialisation.
536-557: Verification complete: yaml-cpp is only discovered, not linked, and only OpenSSL::Crypto is used.No
target_link_librariesinvocations referenceyaml-cppanywhere incomponents/(all occurrences are infind_packagecalls), and incomponents/core/src/clp_s/CMakeLists.txtonlyOpenSSL::Cryptois linked—there are no references toOpenSSL::SSL.
| # will be built. | ||
| function(validate_and_setup_all_clp_dependency_flags) | ||
| # the target's dependencies are also `ON`. | ||
| function(validate_all_clp_dependency_flags) | ||
| if (CLP_BUILD_EXECUTABLES) |
There was a problem hiding this comment.
Update validate_clp_dependencies_for_target so that we can remove this if (CLP_BUILD_EXECUTABLES). Ditto for all the following if blocks.
| get_clp_checked_find(ystdlib) | ||
| if (CLP_CHECKED_FIND) | ||
| return() | ||
| endif() |
There was a problem hiding this comment.
guard the other macros with get_clp_checked_find and set_clp_checked_find as well to remove duplicate library find_package() messages.
I guess you need if(NOT CLP_CHECKED_FIND) since macros don't have return()
As a side note, I've wondered if using the <lib_FOUND> flag can achieve the same header guarding purpose, e.g. guard clp_find_boost with if(NOT Boost_FOUND). However, in my own local experiments, I still see duplicate find_package() messages. Perhaps there are race conditions during the cmake generation phase.
There was a problem hiding this comment.
I don't think we can guard the entirety of the macros (at least for most of them) since generally find_package just makes things available in the current scope, so if we do a build where e.g. we disable unit testing then have header guards set up s.t. we only call find_package one time for each dependency they'll end up being made available in one subdirectory each and nowhere else.
I can try making warnings emit only one time each, though tbh it seems like the only thing emitting warnings is the version range on boost (which happens inside the find_package call. Maybe I can just fix our find_package call and version checking for boost? After that we should be in a state where developers only see each warning once (since we do have a header guard for each time we emit warnings, and we will no longer have warnings inside any find_package calls).
There was a problem hiding this comment.
I ended up doing this + changing the clp_find_boost macro s.t. we silence the output from inside of the boost find_package call after the first invocation. Note that you'll still see logs for boost being found twice, because we also find boost in tools/yscope-dev-utils/taskfiles/boost/CMakeLists.txt.
| @@ -130,6 +130,20 @@ set( | |||
| ) | |||
|
|
|||
| if(CLP_BUILD_EXECUTABLES) | |||
| find_package(absl REQUIRED) | |||
There was a problem hiding this comment.
We should change all find_package to clp_find_X for consistency and ease of maintainence.
There was a problem hiding this comment.
Ah, I was originally doing things that way, but @kirkrodrigues preferred not having macros wrapping the straightforward find_package calls, so I switched to this approach.
There was a problem hiding this comment.
If you both feel that's clearer, we could. I just felt like a 3-line macro for each of these was adding a lot more code.
There was a problem hiding this comment.
I see the reasoning. We could leave find_package calls without version requirements macro-less.
That said, if we eventually need to enforce a version requirement for one of the libraries, having already replaced all find_package(X) calls with clp_find_X() means it would just be a one-line change, rather than a grep and replace across all CMakeLists.
There was a problem hiding this comment.
Yeah I think I agree with Bill on this one, it isn't that much more code and it will make maintenance a bit easier, so we might as well use macros for everything.
Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
components/core/cmake/find_utils.cmake (4)
65-73: Stop overriding LibLZMA_ROOT; rely on the project’s CLP_CORE_DEPS_DIR-first FindLibLZMA.cmake pathingThe
LibLZMA-static_ROOT/LibLZMA-shared_ROOTvariables are not defined elsewhere and will often be empty, degrading discovery. Per project learnings, custom FindLibLZMA already prioritizes CLP_CORE_DEPS_DIR; overridingLibLZMA_ROOTis unnecessary and brittle. Also, the TODO is outdated (the Find module exists), and the CVE list should be accurate.Apply:
macro(clp_find_lzma) - if(CLP_USE_STATIC_LIBS) - set(LibLZMA_ROOT ${LibLZMA-static_ROOT}) - set(LibLZMA_USE_STATIC_LIBS ON) - else() - set(LibLZMA_ROOT ${LibLZMA-shared_ROOT}) - endif() - # Version 5.8.1 and above address CVE-2024-3094 and CVE-2025-31115. - find_package(LibLZMA 5.8.1 REQUIRED) + get_clp_checked_find(liblzma) + if(NOT CLP_CHECKED_FIND) + if(CLP_USE_STATIC_LIBS) + set(LibLZMA_USE_STATIC_LIBS ON) + endif() + # Our FindLibLZMA.cmake prioritizes CLP_CORE_DEPS_DIR; avoid overriding LibLZMA_ROOT. + # Version 5.8.1+ includes the XZ/liblzma backdoor fix (CVE-2024-3094). + find_package(LibLZMA 5.8.1 REQUIRED) + set_clp_checked_find(liblzma) + endif() endmacro()Action items:
- Remove/avoid
LibLZMA-*-ROOToverrides here.- Update the TODO to reflect the existing Find module.
- Verify the CVE reference(s) and keep only accurate ones. If unsure, drop the CVE IDs from the comment and just note “includes XZ/liblzma backdoor fix”.
48-50: Prefer the imported target and (optionally) respect static builds for CURL
- Encourage downstream linking via
CURL::libcurl. Expose it as a helper variable for consistency withzstd_TARGET/MONGOCXX_TARGET.- Optionally honour
CLP_USE_STATIC_LIBSby settingCURL_USE_STATIC_LIBS ON(many distros also ship static libcurl; if not available, the REQUIRED will correctly fail).macro(clp_find_curl) - find_package(CURL 7.61.1 REQUIRED) + get_clp_checked_find(curl) + if(NOT CLP_CHECKED_FIND) + if(CLP_USE_STATIC_LIBS) + set(CURL_USE_STATIC_LIBS ON) + endif() + find_package(CURL 7.61.1 REQUIRED) + set_clp_checked_find(curl) + endif() + set(CURL_TARGET CURL::libcurl) endmacro()
109-112: Make sqlite’s dependency list portable across platformsHard-coding
"dl;m;pthread"is Linux-centric. Use CMake helpers and platform guards so this works on macOS/Windows too.macro(clp_find_sqlite) - set(sqlite_DYNAMIC_LIBS "dl;m;pthread") + # Compose a portable default list: + set(sqlite_DYNAMIC_LIBS "") + if(UNIX) + # dl: dynamic loader (present on Linux; empty on macOS via CMAKE_DL_LIBS) + list(APPEND sqlite_DYNAMIC_LIBS "${CMAKE_DL_LIBS}") + # m: libm on UNIX + list(APPEND sqlite_DYNAMIC_LIBS "m") + # pthreads via FindThreads; fall back to 'pthread' name for raw libs lists. + find_package(Threads REQUIRED) + list(APPEND sqlite_DYNAMIC_LIBS "pthread") + endif() include("${PROJECT_SOURCE_DIR}/cmake/Modules/FindLibraryDependencies.cmake") FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") endmacro()Alternative (larger change): switch to
find_package(SQLite3 REQUIRED)and rely onSQLite::SQLite3’s transitive requirements. If you want that, I can draft it.
22-37: Guard all clp_find_ macros with the one-time “checked find” to suppress duplicate find_package chatter*Per Bill’s note, wrap each
find_package(...)withget_clp_checked_find()/set_clp_checked_find()and only run once. Keep any target/variable setup outside the guard if it must run on every invocation. Example fixes for a few macros below; please replicate the same pattern for the rest.Boost example (apply this exact pattern to Catch2, LibArchive, msgpack, etc.):
macro(clp_find_boost) - if(CLP_USE_STATIC_LIBS) - set(Boost_USE_STATIC_LIBS ON) - endif() - find_package( - Boost - 1.81...1.88 - REQUIRED - filesystem - iostreams - program_options - regex - system - url - ) + get_clp_checked_find(boost) + if(NOT CLP_CHECKED_FIND) + if(CLP_USE_STATIC_LIBS) + set(Boost_USE_STATIC_LIBS ON) + endif() + find_package( + Boost + 1.81...1.88 + REQUIRED + filesystem + iostreams + program_options + regex + system + url + ) + set_clp_checked_find(boost) + endif() endmacro()MariaDBClient example (ensure the warning is emitted only once):
macro(clp_find_mariadb_client) - # Only log static linking warning one time - get_clp_checked_find(mariadb_client) - if(CLP_USE_STATIC_LIBS AND NOT CLP_CHECKED_FIND) - # NOTE: We can't statically link to MariaDBClient since it's GPL - message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.") - endif() - find_package(MariaDBClient 3.1.0 REQUIRED) - set_clp_checked_find(mariadb_client) + get_clp_checked_find(mariadb_client) + if(NOT CLP_CHECKED_FIND) + if(CLP_USE_STATIC_LIBS) + # NOTE: We can't statically link to MariaDBClient since it's GPL + message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.") + endif() + find_package(MariaDBClient 3.1.0 REQUIRED) + set_clp_checked_find(mariadb_client) + endif() endmacro()mongocxx example (call find once; set MONGOCXX_TARGET every time):
macro(clp_find_mongocxx) - find_package(mongocxx REQUIRED) + get_clp_checked_find(mongocxx) + if(NOT CLP_CHECKED_FIND) + find_package(mongocxx REQUIRED) + set_clp_checked_find(mongocxx) + endif() if(CLP_USE_STATIC_LIBS) set(MONGOCXX_TARGET mongo::mongocxx_static) else() set(MONGOCXX_TARGET mongo::mongocxx_shared) endif() endmacro()zstd example (call find once; set zstd_TARGET every time):
macro(clp_find_zstd) - # v1.4.8 is the lowest version available in the package managers of the OSes we support. - find_package(zstd 1.4.8 REQUIRED) + get_clp_checked_find(zstd) + if(NOT CLP_CHECKED_FIND) + # v1.4.8 is the lowest version available in the package managers of the OSes we support. + find_package(zstd 1.4.8 REQUIRED) + set_clp_checked_find(zstd) + endif() if(CLP_USE_STATIC_LIBS) set(zstd_TARGET zstd::libzstd_static) else() set(zstd_TARGET zstd::libzstd_shared) endif() endmacro()Also applies to: 41-43, 48-50, 55-59, 65-73, 78-86, 91-98, 103-104, 129-137
📜 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/cmake/find_utils.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
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: 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
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.
📚 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/cmake/find_utils.cmake
📚 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/cmake/find_utils.cmake
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/cmake/find_utils.cmake
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/cmake/find_utils.cmake
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/cmake/find_utils.cmake
📚 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:
components/core/cmake/find_utils.cmake
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/cmake/find_utils.cmake
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/cmake/find_utils.cmake
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/cmake/find_utils.cmake
⏰ 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). (13)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
components/core/cmake/find_utils.cmake (1)
26-36: No changes required: CMake ≥3.21 supports version ranges
Project’scmake_minimum_required(VERSION 3.21)satisfies version-range syntax support (introduced in CMake 3.19) (cmake.org)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/core/CMakeLists.txt (1)
567-599: Use CURL::libcurl in tests tooPrefer the imported target over ${CURL_LIBRARIES} for correctness and portability.
target_link_libraries(unitTest PRIVATE absl::flat_hash_map Boost::filesystem Boost::iostreams Boost::program_options Boost::regex Boost::url Catch2::Catch2WithMain - ${CURL_LIBRARIES} + CURL::libcurl clp_s::search::ast clp_s::search::kql clp_s::search::sql clp_s::timestamp_patterncomponents/core/cmake/Options/options.cmake (1)
184-189: Minor naming nit: duplicate “dependencies”Clean up the function name for consistency.
-function(validate_clp_s_reducer_dependencies_dependencies) +function(validate_clp_s_reducer_dependencies) validate_clp_dependencies_for_target(CLP_BUILD_CLP_S_REDUCER_DEPENDENCIES CLP_BUILD_CLP_S_CLP_DEPENDENCIES ) endfunction()Also update the caller:
- validate_clp_s_reducer_dependencies_dependencies() + validate_clp_s_reducer_dependencies()Also applies to: 237-237
♻️ Duplicate comments (4)
components/core/src/clp_s/indexer/CMakeLists.txt (1)
127-141: Prefer imported target over ${CURL_LIBRARIES}Link against CURL::libcurl to get transitive include dirs/definitions and avoid env-dependent linkage.
Apply:
target_link_libraries(indexer PRIVATE absl::flat_hash_map Boost::iostreams Boost::program_options Boost::url - ${CURL_LIBRARIES} + CURL::libcurl clp::string_utils date::date MariaDBClient::MariaDBClient msgpack-cxx nlohmann_json::nlohmann_json OpenSSL::Crypto simdjson::simdjson spdlog::spdlog ystdlib::containers ${zstd_TARGET} )components/core/src/clp/clg/CMakeLists.txt (1)
133-144: Reduce repeated dependency-resolution boilerplateThe same find/clp_find block appears across executables; consider a small helper macro (e.g., clp_resolve_common_deps()) to cut duplication and ease edits.
components/core/src/clp/clp/CMakeLists.txt (1)
171-184: OK overall; keep find/grouping tidyLooks good. For readability, placing clp_find_libarchive immediately above its first consumer target_link_libraries can help trace deps.
components/core/CMakeLists.txt (1)
536-556: Narrow OpenSSL usage in tests to CryptounitTest links only OpenSSL::Crypto. Request just the Crypto component.
- find_package(OpenSSL REQUIRED) + find_package(OpenSSL REQUIRED COMPONENTS Crypto)
📜 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 (5)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/Options/options.cmake(2 hunks)components/core/src/clp/clg/CMakeLists.txt(1 hunks)components/core/src/clp/clp/CMakeLists.txt(1 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
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: 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
📚 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/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 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/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 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/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 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:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-09-03T07:29:35.223Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
components/core/src/clp/clg/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). (10)
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (3)
components/core/CMakeLists.txt (1)
15-18: Good centralisation hookvalidate_all_clp_dependency_flags() + find_utils.cmake inclusion aligns with the PR’s intent. LGTM.
components/core/cmake/Options/options.cmake (2)
101-104: Early return on disabled options is correctShort-circuiting validation when the target option is OFF avoids spurious errors. LGTM.
145-147: Do not remove the CLP_NEED_YSTDLIB setter – changes to theCLP_NEED_*mechanism will be addressed in the planned refactoring PR.Likely an incorrect or invalid review comment.
| clp_find_mariadb_client() | ||
| clp_find_msgpack() | ||
| find_package(nlohmann_json REQUIRED) | ||
| find_package(OpenSSL REQUIRED) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Scope OpenSSL to Crypto only
Indexing/auth signing typically needs libcrypto, not libssl. Tighten the requirement.
-find_package(OpenSSL REQUIRED)
+find_package(OpenSSL REQUIRED COMPONENTS Crypto)📝 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.
| find_package(OpenSSL REQUIRED) | |
| find_package(OpenSSL REQUIRED COMPONENTS Crypto) |
🤖 Prompt for AI Agents
In components/core/src/clp_s/indexer/CMakeLists.txt around line 118, the
CMakeLists currently requests the whole OpenSSL package but the indexer only
needs libcrypto; change the find_package call to request only the Crypto
component (find_package(OpenSSL REQUIRED COMPONENTS Crypto)) and update any
target_link_libraries or usages to link against OpenSSL::Crypto instead of the
full OpenSSL/SSL target so only libcrypto is required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
components/core/src/clp_s/indexer/CMakeLists.txt (1)
126-141: Prefer CURL::libcurl over ${CURL_LIBRARIES}Using the imported target ensures include dirs/defines propagate and avoids backend-specific surprises. Replace the raw variable with the target.
- ${CURL_LIBRARIES} + CURL::libcurlcomponents/core/src/clp/clg/CMakeLists.txt (1)
133-145: Reduce repeated boilerplate with a small helper macroThese
clp_find_*calls repeat across executables. Consider aclp_resolve_common_deps_for_exec()helper that wraps the common finds to cut churn and keep edits centralised.components/core/src/clp_s/CMakeLists.txt (3)
124-139: Prefer CURL::libcurl over ${CURL_LIBRARIES} for robustness.Modern imported target is less error‑prone across static/shared builds.
- ${CURL_LIBRARIES} + CURL::libcurl
294-300: Use CURL::libcurl for the writer, too.- ${CURL_LIBRARIES} + CURL::libcurl
363-369: Use CURL::libcurl for the reader as well.- ${CURL_LIBRARIES} + CURL::libcurlcomponents/core/CMakeLists.txt (1)
571-576: Prefer CURL::libcurl in tests for consistency with modern CMake.- ${CURL_LIBRARIES} + CURL::libcurlcomponents/core/cmake/find_utils.cmake (5)
6-12: Make “checked find” keys case-insensitive to avoid duplicate flags.function(get_clp_checked_find LIBRARY) - get_property(CLP_CHECKED_FIND - DIRECTORY "${PROJECT_SOURCE_DIR}" - PROPERTY "clp_checked_find_${LIBRARY}" - ) + string(TOLOWER "${LIBRARY}" _clp_lib_key) + get_property(CLP_CHECKED_FIND + DIRECTORY "${PROJECT_SOURCE_DIR}" + PROPERTY "clp_checked_find_${_clp_lib_key}" + ) set(CLP_CHECKED_FIND "${CLP_CHECKED_FIND}" PARENT_SCOPE) endfunction()
16-18: Setter must use the same normalised key.function(set_clp_checked_find LIBRARY) - set_property(DIRECTORY "${PROJECT_SOURCE_DIR}" PROPERTY "clp_checked_find_${LIBRARY}" TRUE) + string(TOLOWER "${LIBRARY}" _clp_lib_key) + set_property(DIRECTORY "${PROJECT_SOURCE_DIR}" PROPERTY "clp_checked_find_${_clp_lib_key}" TRUE) endfunction()
110-118: Avoid fragile LibLZMA_ROOT overrides; guard or rely on CLP_CORE_DEPS_DIR-first.macro(clp_find_lzma) - if(CLP_USE_STATIC_LIBS) - set(LibLZMA_ROOT ${LibLZMA-static_ROOT}) - set(LibLZMA_USE_STATIC_LIBS ON) - else() - set(LibLZMA_ROOT ${LibLZMA-shared_ROOT}) - endif() + if(CLP_USE_STATIC_LIBS) + if(DEFINED LibLZMA-static_ROOT AND LibLZMA-static_ROOT) + set(LibLZMA_ROOT "${LibLZMA-static_ROOT}") + endif() + set(LibLZMA_USE_STATIC_LIBS ON) + else() + if(DEFINED LibLZMA-shared_ROOT AND LibLZMA-shared_ROOT) + set(LibLZMA_ROOT "${LibLZMA-shared_ROOT}") + endif() + endif() # Version 5.8.1 and above address CVE-2024-3094 and CVE-2025-31115. find_package(LibLZMA 5.8.1 REQUIRED) endmacro()
175-181: Make sqlite’s extra libs portable (dl/m/pthread vary by platform).macro(clp_find_sqlite) - set(sqlite_DYNAMIC_LIBS "dl;m;pthread") + set(sqlite_DYNAMIC_LIBS "") + if(UNIX) + list(APPEND sqlite_DYNAMIC_LIBS "${CMAKE_DL_LIBS}") + list(APPEND sqlite_DYNAMIC_LIBS "m") + find_package(Threads REQUIRED) + # If your helper can’t consume imported targets, fall back to 'pthread' on POSIX. + list(APPEND sqlite_DYNAMIC_LIBS "pthread") + endif() include("${PROJECT_SOURCE_DIR}/cmake/Modules/FindLibraryDependencies.cmake") FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") endmacro()
191-201: Harden ystdlib add_subdirectory: cache the option and validate the path.function(clp_find_ystdlib) @@ - set(YSTDLIB_CPP_BUILD_TESTING OFF) - add_subdirectory("${CLP_YSTDLIB_SOURCE_DIRECTORY}" "${CMAKE_BINARY_DIR}/ystdlib" EXCLUDE_FROM_ALL) + set(YSTDLIB_CPP_BUILD_TESTING OFF CACHE BOOL "Disable ystdlib tests from CLP" FORCE) + if(NOT DEFINED CLP_YSTDLIB_SOURCE_DIRECTORY + OR NOT EXISTS "${CLP_YSTDLIB_SOURCE_DIRECTORY}/CMakeLists.txt") + message(FATAL_ERROR "CLP_YSTDLIB_SOURCE_DIRECTORY is not set or invalid.") + endif() + add_subdirectory("${CLP_YSTDLIB_SOURCE_DIRECTORY}" "${CMAKE_BINARY_DIR}/ystdlib" EXCLUDE_FROM_ALL) set_clp_checked_find(ystdlib) endfunction()
📜 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 (14)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/find_utils.cmake(1 hunks)components/core/src/clp/clg/CMakeLists.txt(1 hunks)components/core/src/clp/clo/CMakeLists.txt(1 hunks)components/core/src/clp/clp/CMakeLists.txt(1 hunks)components/core/src/clp/make_dictionaries_readable/CMakeLists.txt(1 hunks)components/core/src/clp_s/CMakeLists.txt(8 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/ast/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/kql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/sql/CMakeLists.txt(1 hunks)components/core/src/glt/glt/CMakeLists.txt(1 hunks)components/core/src/reducer/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
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.
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: 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
📚 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/reducer/CMakeLists.txtcomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/search/sql/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/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/reducer/CMakeLists.txtcomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/search/sql/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/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/reducer/CMakeLists.txtcomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 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:
components/core/src/reducer/CMakeLists.txtcomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp_s/search/ast/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/search/sql/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-09-03T07:29:35.223Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
Applied to files:
components/core/src/reducer/CMakeLists.txtcomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp_s/search/ast/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/search/sql/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/search/sql/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/src/clp_s/search/ast/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp_s/search/sql/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/cmake/find_utils.cmakecomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txt
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
components/core/src/clp/clg/CMakeLists.txt
📚 Learning: 2025-08-17T16:10:38.722Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.
Applied to files:
components/core/cmake/find_utils.cmake
📚 Learning: 2025-08-25T07:27:19.449Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2024-10-14T03:42:10.355Z
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-25T06:29:59.610Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.610Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
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). (12)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (22)
components/core/src/clp_s/search/ast/CMakeLists.txt (1)
53-54: clp_find_simdjson placement before target — looks goodDiscovery occurs before target creation and the link uses simdjson::simdjson; aligns with the new pattern.
components/core/src/clp_s/search/sql/CMakeLists.txt (1)
15-16: Added dependency discovery for ANTLR4 and spdlog — OKMatches the centralised clp_find_* approach and precedes target creation.
Please confirm clp_find_antlr4_runtime guarantees the antlr4_static target (not just antlr4-runtime), since this file links antlr4_static. If there are builds where only a shared target is available, ensure the helper enforces or maps to the correct one.
components/core/src/clp_s/search/kql/CMakeLists.txt (1)
15-16: Mirrored discovery for ANTLR4/spdlog — consistent with SQLNo issues; same note about antlr4_static applicability as in the SQL target.
If clp_find_antlr4_runtime can yield different target names per platform/build (static vs shared), consider normalising to a single imported target alias to keep target_link_libraries here stable.
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt (1)
43-47: Confirm Boost targets in clp_find_boost
clp_find_zstd correctly setszstd_TARGETto eitherzstd::libzstd_staticorzstd::libzstd_shared. Ensure thatclp_find_boostinvokesfind_package(Boost REQUIRED COMPONENTS filesystem program_options)(viaget_clp_checked_find(boost)) and exposes theBoost::filesystemandBoost::program_optionsimported targets.components/core/src/clp/clp/CMakeLists.txt (1)
171-184: Idempotency guards are correctly applied, no changes needed
The checked-find pattern is in place for boost, mariadb_client and ystdlib; all other clp_find_* calls rely on CMake’s built-in find_package caching and won’t emit duplicate warnings.components/core/src/reducer/CMakeLists.txt (2)
42-47: Early dependency resolution via clp_find_ looks good*Calls align with the linked imported targets; nice cleanup.
42-47: No action required: mongocxx/msgpack targets are correctly exposed
clp_find_mongocxx sets MONGOCXX_TARGET to mongo::mongocxx_{static|shared}, and clp_find_msgpack invokes find_package(msgpack-cxx 7.0.0 REQUIRED), which provides the imported target msgpack-cxx.components/core/src/clp_s/search/CMakeLists.txt (1)
30-33: LGTM: discovery calls match linked targets
absl,log_surgeon,simdjson, andspdlogdiscovery now precedes target creation and aligns with the imported targets used in linking.components/core/src/clp/clo/CMakeLists.txt (1)
159-171: Validate wrapper outputs used in linking
• clp_find_mongocxx always sets MONGOCXX_TARGET (both static and shared)
• clp_find_zstd always sets zstd_TARGET (both static and shared)
• Could not locate the definition of FindDynamicLibraryDependencies – manually confirm that clp_find_sqlite always populates sqlite_LIBRARY_DEPENDENCIES under all supported configurations
Per maintainers’ guidance, no guards needed around ${zstd_TARGET} in this PRcomponents/core/src/glt/glt/CMakeLists.txt (1)
175-186: LGTM: dependency discovery is now colocated with the target.Calls to clp_find_* align with the PR objective and cover all linked libs below. Nice and tidy.
components/core/src/clp_s/CMakeLists.txt (8)
106-116: LGTM: coherent per-target discovery.Good coverage and order before creating/linking clp_s_clp_dependencies.
168-169: LGTM: reducer deps discovery matches linkage.
206-210: LGTM: IO deps discovery aligns with later link usage.
269-277: LGTM: writer deps discovery is complete.
339-346: LGTM: reader deps discovery is complete.
381-385: LGTM: JSON constructor deps discovery matches linkage.
415-417: LGTM: timestamp pattern deps discovery matches linkage.
447-452: LGTM: executable deps discovery is colocated and consistent.components/core/CMakeLists.txt (2)
15-18: LGTM: centralised flag validation and find utils inclusion.This cleanly replaces the former NEEDS propagation.
536-556: LGTM: explicit discovery for testing fixes prior msgpack-cxx gap.The added clp_find_* calls align with linked targets and resolve the earlier configure-time risk.
components/core/cmake/find_utils.cmake (2)
35-64: LGTM: Boost range and QUIET-on-repeat are sensible; static toggle respected.
205-213: LGTM: zstd target selection via zstd_TARGET is consistent with downstream usage.
|
|
||
| if (CLP_BUILD_CLP_REGEX_UTILS) | ||
| validate_clp_regex_utils_dependencies() | ||
| set_clp_regex_utils_dependencies() |
There was a problem hiding this comment.
think you forgot to remove this function set_clp_regex_utils_dependencies
| # Finds and sets up the the absl library. | ||
| # @return Forwards any variables from the `find_package` call. | ||
| macro(clp_find_absl) | ||
| find_package(absl REQUIRED) |
There was a problem hiding this comment.
I feel like adding back the message("{<lib>_VERSION}") or message("{<lib>_VERSION_STRING}") is helpful. Especially as a person who's been working on deps a lot, it assures me when I see the version used is the version installed.
That being said, did you investigate the overly verbose messages when find_package() is called multiple times for libraries that emit CMake warnings?
Lastly, if we add the version logging, we would need to header guard all macros.
(Let me know if you and Kirk have discussed this point and reached a different conclusion about logging version strings)
There was a problem hiding this comment.
Yeah like I mention in my other comment I:
- Fixed the warning caused by our incorrect usage of
find_packagefor boost - Made the
find_packagecallQUIETfor boost after the first invocation since boost itself logs the found version withinFindBoost
For logging the version of each library I see where you're coming from, but I feel like if we have a specific version requirement we should just specify it in the find_package call, and if it isn't satisfied the user will get a message saying that we were unable to find the correct version anyway.
If you really feel like its useful as a maintainer of the cmake scripts to log the found versions anyway though I'd prefer splitting that change off into a smaller follow-up PR, since this one is already pretty big.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/core/cmake/Options/options.cmake (1)
101-104: Early return eliminates redundant if(CLP_BUILD_...) guards — nice.This addresses the prior request to move gating logic into the validator and removes boilerplate at call sites.
📜 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/cmake/Options/options.cmake(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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: 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
📚 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/cmake/Options/options.cmake
📚 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/cmake/Options/options.cmake
📚 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/cmake/Options/options.cmake
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/cmake/Options/options.cmake
⏰ 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). (14)
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (1)
components/core/cmake/Options/options.cmake (1)
220-239: Confirm aggregator scope matches PR summary
The functionvalidate_all_clp_dependency_flags()currently invokes not only binaries/tests/regex_utils but also allclp_s_*validators, whereas the PR summary describes it as “top-level only.” Either remove the extravalidate_clp_s_*()calls or update the summary to reflect exhaustive validation. This aggregator is invoked once atcomponents/core/CMakeLists.txt:15.
| if(NOT DEFINED TARGET_CLP_BUILD_OPTION OR TARGET_CLP_BUILD_OPTION STREQUAL "") | ||
| message(FATAL_ERROR "TARGET_CLP_BUILD_OPTION can't be unset or empty.") | ||
| endif() | ||
|
|
||
| # Only validate dependencies for this option if the option is enabled. | ||
| if(NOT "${${TARGET_CLP_BUILD_OPTION}}") | ||
| return() | ||
| endif() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against silent no‑ops when the option name is misspelled.
If the variable named by TARGET_CLP_BUILD_OPTION isn’t defined, ${${TARGET_CLP_BUILD_OPTION}} expands to empty and the validator returns early, masking the typo. Add an explicit DEFINED check.
function(validate_clp_dependencies_for_target TARGET_CLP_BUILD_OPTION)
- if(NOT DEFINED TARGET_CLP_BUILD_OPTION OR TARGET_CLP_BUILD_OPTION STREQUAL "")
+ if(NOT DEFINED TARGET_CLP_BUILD_OPTION OR TARGET_CLP_BUILD_OPTION STREQUAL "")
message(FATAL_ERROR "TARGET_CLP_BUILD_OPTION can't be unset or empty.")
endif()
+ # Ensure the referenced option variable actually exists (catch typos early).
+ if(NOT DEFINED ${TARGET_CLP_BUILD_OPTION})
+ message(FATAL_ERROR
+ "Unknown option '${TARGET_CLP_BUILD_OPTION}'. "
+ "Ensure it is declared with option(...)."
+ )
+ endif()
+
# Only validate dependencies for this option if the option is enabled.
if(NOT "${${TARGET_CLP_BUILD_OPTION}}")
return()
endif()📝 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.
| if(NOT DEFINED TARGET_CLP_BUILD_OPTION OR TARGET_CLP_BUILD_OPTION STREQUAL "") | |
| message(FATAL_ERROR "TARGET_CLP_BUILD_OPTION can't be unset or empty.") | |
| endif() | |
| # Only validate dependencies for this option if the option is enabled. | |
| if(NOT "${${TARGET_CLP_BUILD_OPTION}}") | |
| return() | |
| endif() | |
| function(validate_clp_dependencies_for_target TARGET_CLP_BUILD_OPTION) | |
| if(NOT DEFINED TARGET_CLP_BUILD_OPTION OR TARGET_CLP_BUILD_OPTION STREQUAL "") | |
| message(FATAL_ERROR "TARGET_CLP_BUILD_OPTION can't be unset or empty.") | |
| endif() | |
| # Ensure the referenced option variable actually exists (catch typos early). | |
| if(NOT DEFINED ${TARGET_CLP_BUILD_OPTION}) | |
| message(FATAL_ERROR | |
| "Unknown option '${TARGET_CLP_BUILD_OPTION}'. " | |
| "Ensure it is declared with option(...)." | |
| ) | |
| endif() | |
| # Only validate dependencies for this option if the option is enabled. | |
| if(NOT "${${TARGET_CLP_BUILD_OPTION}}") | |
| return() | |
| endif() | |
| …rest of function… | |
| endfunction() |
🤖 Prompt for AI Agents
In components/core/cmake/Options/options.cmake around lines 97 to 105, the guard
uses indirect expansion ${${TARGET_CLP_BUILD_OPTION}} which becomes empty if the
variable name is misspelled, causing a silent no-op; update the logic to first
check that the variable named by TARGET_CLP_BUILD_OPTION is DEFINED (e.g. using
if(NOT DEFINED ${TARGET_CLP_BUILD_OPTION}) or an equivalent check) and emit a
message(FATAL_ERROR) or return with a clear error when it is not, before
evaluating its boolean value, so typos or missing variable definitions are
caught explicitly.
| if(NOT "${${DEPENDENCY}}") | ||
| message(FATAL_ERROR "${TARGET_CLP_BUILD_OPTION} requires ${DEPENDENCY}=ON") | ||
| endif() | ||
| endforeach() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catch typos in dependency names; check DEFINED before value.
Currently, an undefined dependency variable expands to empty and triggers the “requires =ON” error, which can be misleading. Fail with a clearer message when the variable is not defined.
- foreach(DEPENDENCY IN LISTS ARGN)
- if(NOT "${${DEPENDENCY}}")
- message(FATAL_ERROR "${TARGET_CLP_BUILD_OPTION} requires ${DEPENDENCY}=ON")
- endif()
- endforeach()
+ foreach(DEPENDENCY IN LISTS ARGN)
+ if(NOT DEFINED ${DEPENDENCY})
+ message(FATAL_ERROR
+ "${TARGET_CLP_BUILD_OPTION} requires '${DEPENDENCY}' to be defined "
+ "(possible typo in dependency list)."
+ )
+ elseif(NOT ${${DEPENDENCY}})
+ message(FATAL_ERROR "${TARGET_CLP_BUILD_OPTION} requires ${DEPENDENCY}=ON")
+ endif()
+ endforeach()📝 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.
| if(NOT "${${DEPENDENCY}}") | |
| message(FATAL_ERROR "${TARGET_CLP_BUILD_OPTION} requires ${DEPENDENCY}=ON") | |
| endif() | |
| endforeach() | |
| foreach(DEPENDENCY IN LISTS ARGN) | |
| if(NOT DEFINED ${DEPENDENCY}) | |
| message(FATAL_ERROR | |
| "${TARGET_CLP_BUILD_OPTION} requires '${DEPENDENCY}' to be defined " | |
| "(possible typo in dependency list)." | |
| ) | |
| elseif(NOT ${${DEPENDENCY}}) | |
| message(FATAL_ERROR "${TARGET_CLP_BUILD_OPTION} requires ${DEPENDENCY}=ON") | |
| endif() | |
| endforeach() |
🤖 Prompt for AI Agents
In components/core/cmake/Options/options.cmake around lines 107 to 110, the
check currently expands undefined dependency variables to empty strings and
produces a misleading "requires =ON" error; change the logic to first test if
the dependency variable is DEFINED and, if not, emit a clear FATAL_ERROR
indicating the dependency variable is missing/undefined, otherwise check its
value and emit the existing "requires ...=ON" error if it is not ON; ensure the
error messages name the missing variable and the dependent build option for
clarity.
Description
This PR removes the
CLP_NEEDS_Xflags that were used to manually track library dependencies of each target and switches to simply callingfind_package(or equivalent) for each dependency of each target guarded by itsCLP_BUILD_Xflag. This makes it so that required dependencies can be resolved next to their targets instead of requiring book-keeping in a separate file.To simplify the process of resolving each dependency multiple times and avoid duplicating logic to set up flags specific to certain dependencies we add utility functions for resolving each dependency. These functions are named
clp_find_XwhereXis the name of the dependency.We also add two functions
get_clp_checked_find(X)andset_clp_checked_find(X)that theclp_find_Xfunctions can use to mark and check whether they have previously run. This is currently only used byclp_find_ystdlibto make the setup logic run only once, since the way we set upystdlibwill error out if run multiple times, and byclp_find_mariadbto only emit a warning message one time.Dev docs describing how to use
clp_find_Xvsfind_package(X)as well as theCLP_BUILD_Yflags will be added in a follow-up PR.Checklist
breaking change.
Validation performed
CLP_BUILD_TESTINGandCLP_BUILD_EXECUTABLESisOFFSummary by CodeRabbit
Refactor
Chores
New Features