build(core): Add flags that external projects can use to disable any part of the build. #955
Conversation
## Walkthrough
This change introduces conditional CMake logic throughout the build system, allowing selective enabling or disabling of binaries, libraries, tests, and dependencies using dedicated `CLP_BUILD_*` flags. All affected targets and dependency checks are now wrapped in conditionals, providing fine-grained control over the build process without altering code interfaces.
## Changes
| Files/Paths | Change Summary |
|-------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------|
| components/core/CMakeLists.txt | Increased CMake minimum version; included new options file; wrapped all dependency checks and test target creation in `if(CLP_NEED_*)` and `if(CLP_BUILD_TESTING)` conditionals. |
| components/core/src/clp/clg/CMakeLists.txt<br>components/core/src/clp/clo/CMakeLists.txt<br>components/core/src/clp/clp/CMakeLists.txt<br>components/core/src/clp/make_dictionaries_readable/CMakeLists.txt<br>components/core/src/clp_s/indexer/CMakeLists.txt<br>components/core/src/glt/glt/CMakeLists.txt<br>components/core/src/reducer/CMakeLists.txt | Wrapped executable target creation and configuration in `if(CLP_BUILD_EXECUTABLES)` blocks to allow optional binary builds. |
| components/core/src/clp/regex_utils/CMakeLists.txt | Wrapped `regex_utils` library creation/configuration in `if(CLP_BUILD_CLP_REGEX_UTILS)` block. |
| components/core/src/clp/string_utils/CMakeLists.txt | Wrapped `string_utils` library creation/configuration in `if(CLP_BUILD_CLP_STRING_UTILS)` block. |
| components/core/src/clp_s/CMakeLists.txt | Wrapped each library and executable target in component-specific `if(CLP_BUILD_*)` conditionals for selective builds. |
| components/core/src/clp_s/search/CMakeLists.txt | Wrapped `clp_s_search` library creation/configuration in `if(CLP_BUILD_CLP_S_SEARCH)` block. |
| components/core/src/clp_s/search/ast/CMakeLists.txt | Wrapped `clp_s_search_ast` library creation/configuration in `if(CLP_BUILD_CLP_S_SEARCH_AST)` block. |
| components/core/src/clp_s/search/kql/CMakeLists.txt | Wrapped `clp_s_search_kql` library creation/configuration in `if(CLP_BUILD_CLP_S_SEARCH_KQL)` block. |
| components/core/src/clp_s/search/sql/CMakeLists.txt | Wrapped `clp_s_search_sql` library creation/configuration in `if(CLP_BUILD_CLP_S_SEARCH_SQL)` block. |
| components/core/cmake/Options/options.cmake | Added new CMake options to control building of binaries, libraries, tests, and external dependencies with descriptive flags; includes validation and setup functions for dependency flags. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant CMake
participant Dependency
participant Target
User->>CMake: Configure build with flags (e.g., CLP_BUILD_EXECUTABLES)
CMake->>CMake: Validate and setup CLP_BUILD_* and CLP_NEED_* flags
alt Dependency needed
CMake->>Dependency: find_package(...)
Dependency-->>CMake: Provide dependency
else Dependency not needed
CMake-->>Dependency: Skip find_package
end
alt Target build enabled
CMake->>Target: add_executable/add_library(...)
else Target build disabled
CMake-->>Target: Skip target creation
endPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (3)
components/core/src/clp_s/search/kql/CMakeLists.txt (1)
1-8:⚠️ Potential issueWrap ANTLR_TARGET inside the disable flag
The
ANTLR_TARGETinvocation forKqlParserremains outside theif(NOT CLP_NO_BUILD_CLP_S_SEARCH_KQL)block, which still triggers grammar generation and antlr dependencies even when the library is disabled. To fully honourCLP_NO_BUILD_CLP_S_SEARCH_KQL, move theANTLR_TARGETcall inside the conditional.components/core/src/clp_s/CMakeLists.txt (1)
387-422: 🧹 Nitpick (assertive)Verify executable toggle documentation
The top-levelclp-sexecutable is now wrapped inif(NOT CLP_NO_BUILD_BINARIES). Ensure:
CLP_NO_BUILD_BINARIESis declared and described via anoption.- Consider logging the skip case in an
else()block to aid debugging.components/core/CMakeLists.txt (1)
713-758: 🧹 Nitpick (assertive)Refactor and document test executable
Gating theunitTestexecutable underCLP_NO_BUILD_TESTSis appropriate. For maintainability:
- Declare
option(CLP_NO_BUILD_TESTS "Disable building unit tests" OFF).- Break the huge
SOURCE_FILES_unitTestlist into logical groups or usefile(GLOB_RECURSE ...)patterns to reduce manual upkeep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
components/core/CMakeLists.txt(4 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/regex_utils/CMakeLists.txt(1 hunks)components/core/src/clp/string_utils/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)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (9)
components/core/src/reducer/CMakeLists.txt (1)
41-61: Conditional build for reducer-server & per-target flag verificationThe
reducer-serverexecutable is correctly guarded byif(NOT CLP_NO_BUILD_BINARIES), aligning with the global binary-disable pattern. According to the PR objectives, consider whether a finer-grained flagCLP_NO_BUILD_REDUCER_SERVERshould also be provided to disable this specific target independently.components/core/src/clp/make_dictionaries_readable/CMakeLists.txt (1)
42-60: Conditional build for make-dictionaries-readable & per-target flag verificationThe
make-dictionaries-readabletarget is now correctly enclosed inif(NOT CLP_NO_BUILD_BINARIES). Please confirm if a dedicated flag likeCLP_NO_BUILD_MAKE_DICTIONARIES_READABLEis also desired for per-executable control, per the PR objectives.components/core/src/clp/regex_utils/CMakeLists.txt (1)
8-28: Per-target disable for regex_utils libraryThe
regex_utilslibrary is properly wrapped inif(NOT CLP_NO_BUILD_CLP_REGEX_UTILS), providing the intended per-component build control. This aligns well with the new flag conventions.components/core/src/clp_s/search/kql/CMakeLists.txt (1)
8-28: Conditional build for clp_s_search_kql libraryThe
clp_s_search_kqltarget is correctly wrapped inif(NOT CLP_NO_BUILD_CLP_S_SEARCH_KQL), providing the intended fine-grained disable capability for this component.components/core/src/clp_s/search/CMakeLists.txt (1)
31-49: Ensure CMake flag exists and aligns with naming conventionsThe conditional wrap of
clp_s_searchwithinif(NOT CLP_NO_BUILD_CLP_S_SEARCH)is correct. Please verify thatCLP_NO_BUILD_CLP_S_SEARCHis defined viaoption()with a clear description in the main build scripts and follows the establishedCLP_NO_BUILD_*naming convention.components/core/src/clp/clg/CMakeLists.txt (1)
126-156: Confirm consistent behaviour for CLP_NO_BUILD_BINARIESWrapping the entire
clgexecutable inif(NOT CLP_NO_BUILD_BINARIES)follows the PR’s design. Ensure that disabling this flag prevents all binaries (includingclp,clo, etc.) from building and that the flag is declared and documented at the project root.components/core/src/clp/clp/CMakeLists.txt (1)
168-200: Validate CLP_NO_BUILD_BINARIES gating forclpexecutableThe
clpexecutable target is correctly enclosed inif(NOT CLP_NO_BUILD_BINARIES). Please confirm that this flag is consistently applied across all top-level executables and that disabling it cleanly omits all binaries (includingclg,clo, etc.) without leaving orphaned build configuration.components/core/CMakeLists.txt (2)
1-1: Confirm bump to CMake 3.21 is supported
Raisingcmake_minimum_requiredto 3.21 unlocks newer features but may drop support for older CI images or developer environments. Please verify that all supported platforms (CI runners, Docker images, developer machines) provide at least CMake 3.21 and update documentation accordingly.
125-132: All otherif(NOT CLP_NO_NEED_*)guards wrappingfind_packagecalls consistently follow the intended pattern and require no further changes.Also applies to: 135-145, 154-159, 161-166, 168-173, 175-180, 182-187, 189-194, 197-207, 211-218, 221-228, 231-242, 245-258, 261-268, 272-278, 280-284, 298-312, 314-319
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/CMakeLists.txt(4 hunks)components/core/cmake/Options/options.cmake(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
components/core/cmake/Options/options.cmake (1)
1-91: Well-structured build toggles but verify usage ofCLP_NO_BUILD_BINARIES
The build-target options are clearly organised with consistent naming and OFF-by-default semantics. However, I didn’t spot any guard forCLP_NO_BUILD_BINARIESin the top-levelCMakeLists.txt; please confirm that this flag is actually honoured (either at the root or in submodule CMake files) or document where it’s consumed.Could you verify that all executable targets respect
CLP_NO_BUILD_BINARIES?components/core/CMakeLists.txt (3)
1-1: Bumped minimum CMake version to 3.21; confirm CI compatibility
Upgrading to 3.21 is necessary forPROJECT_IS_TOP_LEVELsupport, but please ensure all CI agents and supported platforms have CMake ≥3.21.Can you confirm our build environments meet this requirement?
12-13: Options inclusion right afterproject()is spot on
Includingoptions.cmakeimmediately after the project declaration guarantees all flags are available for downstream logic.
718-763: Test target guard is correctly scoped
Theif(NOT CLP_NO_BUILD_TESTS)wrapping around theunitTesttarget and its dependencies neatly disables tests when desired. Well done.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/CMakeLists.txt(4 hunks)components/core/src/clp_s/search/kql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/sql/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
components/core/src/clp_s/search/sql/CMakeLists.txt (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (6)
components/core/src/clp_s/search/kql/CMakeLists.txt (1)
14-37: Conditional guard added for optional build
Wrapping theclp_s_search_kqltarget and its related commands insideif(NOT CLP_NO_BUILD_CLP_S_SEARCH_KQL)correctly implements the new build‐time flag. Ensure thatCLP_NO_BUILD_CLP_S_SEARCH_KQLis declared inoptions.cmakewith a clear description so users can disable this component.components/core/src/clp_s/search/sql/CMakeLists.txt (1)
14-36: Conditional guard added for optional build
The entireclp_s_search_sqllibrary definition is now enclosed inif(NOT CLP_NO_BUILD_CLP_S_SEARCH_SQL), consistent with the PR’s goal of selective builds. Confirm thatCLP_NO_BUILD_CLP_S_SEARCH_SQLis declared inoptions.cmake.components/core/CMakeLists.txt (4)
1-1: Bumped minimum CMake version to 3.21
Updatingcmake_minimum_required(VERSION 3.21)is necessary forPROJECT_IS_TOP_LEVELandSOURCE_SUBDIRsupport. Verify that no downstream scripts or submodules rely on older CMake features.
12-13: Included centralized options file
Addinginclude("${CMAKE_CURRENT_SOURCE_DIR}/cmake/Options/options.cmake")properly brings in allCLP_NO_BUILD_*andCLP_NO_NEED_*flags. Path and order look correct.
270-270: Always-on Threads dependency
Retainingfind_package(Threads REQUIRED)without a guard aligns with the design choice that threading support is mandatory.
715-760: Guard for unit test target
Wrapping theunitTestexecutable inif(NOT CLP_NO_BUILD_TESTS)is correct. EnsureCLP_NO_BUILD_TESTSis declared inoptions.cmake(and documented) so external projects can disable all tests.
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
components/core/CMakeLists.txt (1)
710-754: 🧹 Nitpick (assertive)Suggest adding a skip message when tests are disabled
Theif(NOT CLP_NO_BUILD_TESTS)block correctly gates theunitTesttarget. Add anelse()branch withmessage(STATUS "Unit tests disabled by CLP_NO_BUILD_TESTS")to clarify when tests are skipped.
♻️ Duplicate comments (2)
components/core/src/clp/clo/CMakeLists.txt (1)
154-184: Guard clo executable with global binary-disable flag
Theif(NOT CLP_NO_BUILD_BINARIES)wrapper properly scopes theclotarget. Ensure theCLP_NO_BUILD_BINARIESoption is declared in your centralized options file and documented for external users.components/core/src/glt/glt/CMakeLists.txt (1)
174-203:⚠️ Potential issueVerify presence of CLP_NO_BUILD_BINARIES declaration
Without anoption(CLP_NO_BUILD_BINARIES …)in your root or shared options file, this guard has no effect. Please confirm and add the option declaration if missing.
Apply this snippet incomponents/core/cmake/Options/options.cmake:+option(CLP_NO_BUILD_BINARIES + "Disable building all CLP binaries" + OFF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
components/core/CMakeLists.txt(4 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/regex_utils/CMakeLists.txt(1 hunks)components/core/src/clp_s/CMakeLists.txt(8 hunks)components/core/src/glt/glt/CMakeLists.txt(1 hunks)
🔇 Additional comments (6)
components/core/src/clp/regex_utils/CMakeLists.txt (1)
8-27: Gate regex_utils library build on its flag
Wrapping theadd_library(regex_utils …)block inif(NOT CLP_NO_BUILD_CLP_REGEX_UTILS)correctly enables optional exclusion of this component. Confirm thatCLP_NO_BUILD_CLP_REGEX_UTILSis declared incomponents/core/cmake/Options/options.cmakeand described in your build documentation.components/core/src/clp/clp/CMakeLists.txt (1)
168-199: Wrap clp executable in binary-disable conditional
Theif(NOT CLP_NO_BUILD_BINARIES)block correctly prevents building theclptarget when the flag is set. Verify that downstream modules, installation rules, and dependent targets gracefully handle the absence ofclp.components/core/src/clp/clg/CMakeLists.txt (1)
126-156: Conditional build for clg executable
Theif(NOT CLP_NO_BUILD_BINARIES)guard correctly scopes theclgtarget. Ensure this global flag’s behavior is communicated in your central build options and documentation.components/core/CMakeLists.txt (3)
1-1: Approve minimum CMake version bump
Requiring CMake 3.21 aligns with the use ofPROJECT_IS_TOP_LEVELandSOURCE_SUBDIRfeatures.
12-14: Approve inclusion of centralized build options
Includingoptions.cmakeearly ensures allCLP_NO_*flags are declared before use in the configuration.
275-278: The ystdlib and sqlite toggle blocks appear correctly wrapped and use the intended subdirectory and dynamic‐library helper. No changes needed.Also applies to: 311-314
| if(NOT CLP_NO_BUILD_CLP_S_REDUCER_DEPENDENCIES) | ||
| add_library( | ||
| clp_s_reducer_dependencies | ||
| ${CLP_S_REDUCER_SOURCES} | ||
| ) | ||
| add_library(clp_s::reducer_dependencies ALIAS clp_s_reducer_dependencies) | ||
| target_compile_features(clp_s_reducer_dependencies PRIVATE cxx_std_20) | ||
| target_link_libraries( | ||
| clp_s_reducer_dependencies | ||
| PUBLIC | ||
| nlohmann_json::nlohmann_json | ||
| PRIVATE | ||
| clp_s::clp_dependencies | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
Wrap the clp_s_reducer_dependencies block with an else() branch to log when CLP_NO_BUILD_CLP_S_REDUCER_DEPENDENCIES is set, improving build-time clarity.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 133 to 147, the build
block for clp_s_reducer_dependencies lacks feedback when disabled. Add an else()
branch after the if(NOT CLP_NO_BUILD_CLP_S_REDUCER_DEPENDENCIES) condition that
outputs a message indicating the clp_s_reducer_dependencies build is skipped due
to the flag being set. Use CMake's message() function to log this information
for better build-time clarity.
| if(NOT CLP_NO_BUILD_CLP_S_ARCHIVEREADER) | ||
| add_library( | ||
| clp_s_archive_reader | ||
| ${CLP_S_ARCHIVE_READER_SOURCES} | ||
| ) | ||
| add_library(clp_s::archive_reader ALIAS clp_s_archive_reader) | ||
| target_compile_features(clp_s_archive_reader PRIVATE cxx_std_20) | ||
| target_include_directories(clp_s_archive_reader PUBLIC ../) | ||
| target_link_libraries( | ||
| clp_s_archive_reader | ||
| PUBLIC | ||
| absl::flat_hash_map | ||
| clp_s::io | ||
| nlohmann_json::nlohmann_json | ||
| PRIVATE | ||
| Boost::url | ||
| clp_s::clp_dependencies | ||
| clp_s::timestamp_pattern | ||
| ${CURL_LIBRARIES} | ||
| fmt::fmt | ||
| msgpack-cxx | ||
| spdlog::spdlog | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
Enclose the clp_s_archive_reader block in an else() with a status message to indicate when CLP_NO_BUILD_CLP_S_ARCHIVEREADER prevents its build.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 292 to 315, the build
block for clp_s_archive_reader lacks feedback when disabled by
CLP_NO_BUILD_CLP_S_ARCHIVEREADER. Add an else() clause after the existing if()
block that uses message(STATUS "...") to print a clear skip message indicating
the archive reader build is disabled. This will improve build transparency.
| if(NOT CLP_NO_BUILD_BINARIES) | ||
| add_executable( | ||
| clp-s | ||
| clp-s.cpp | ||
| ${CLP_S_EXE_SOURCES} | ||
| ) | ||
| target_compile_features(clp-s PRIVATE cxx_std_20) | ||
| target_link_libraries( | ||
| clp-s | ||
| PRIVATE | ||
| Boost::program_options | ||
| clp_s::archive_reader | ||
| clp_s::archive_writer | ||
| clp_s::clp_dependencies | ||
| clp_s::io | ||
| clp_s::json_constructor | ||
| clp_s::reducer_dependencies | ||
| clp_s::search | ||
| clp_s::search::ast | ||
| clp_s::search::kql | ||
| ${MONGOCXX_TARGET} | ||
| msgpack-cxx | ||
| spdlog::spdlog | ||
| ystdlib::error_handling | ||
| ) | ||
| set_target_properties( | ||
| clp-s | ||
| PROPERTIES | ||
| RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
Include an else() branch for if(NOT CLP_NO_BUILD_BINARIES) to message(STATUS "clp-s executable build disabled") when binaries are turned off.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 382 to 412, add an
else() branch to the existing if(NOT CLP_NO_BUILD_BINARIES) condition. Inside
this else() branch, insert a message(STATUS "clp-s executable build disabled")
statement to clearly indicate when the clp-s executable build is skipped due to
the CLP_NO_BUILD_BINARIES flag being set.
| if(NOT CLP_NO_BUILD_CLP_S_IO) | ||
| add_library( | ||
| clp_s_io | ||
| ${CLP_S_IO_SOURCES} | ||
| ) | ||
| add_library(clp_s::io ALIAS clp_s_io) | ||
| target_compile_features(clp_s_io PRIVATE cxx_std_20) | ||
| target_include_directories(clp_s_io PUBLIC ../) | ||
| target_link_libraries( | ||
| clp_s_io | ||
| PRIVATE | ||
| Boost::iostreams Boost::url | ||
| clp_s::clp_dependencies | ||
| fmt::fmt | ||
| spdlog::spdlog | ||
| ZStd::ZStd | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
Consider adding an else() branch after if(NOT CLP_NO_BUILD_CLP_S_IO) to output a status message when the IO library build is disabled.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 169 to 186, add an
else() branch after the existing if(NOT CLP_NO_BUILD_CLP_S_IO) condition to
output a message indicating that the clp_s_io library build is skipped. Use the
message() command to print a clear status message when the build is disabled,
improving build output clarity.
| if(NOT CLP_NO_BUILD_CLP_S_TIMESTAMPPATTERN) | ||
| add_library( | ||
| clp_s_timestamp_pattern | ||
| ${CLP_S_TIMESTAMP_PATTERN_SOURCES} | ||
| ) | ||
| add_library(clp_s::timestamp_pattern ALIAS clp_s_timestamp_pattern) | ||
| target_compile_features(clp_s_timestamp_pattern PRIVATE cxx_std_20) | ||
| target_include_directories(clp_s_timestamp_pattern PUBLIC ../) | ||
| target_link_libraries( | ||
| clp_s_timestamp_pattern | ||
| PRIVATE | ||
| clp::string_utils | ||
| date::date | ||
| spdlog::spdlog | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
To improve UX, add an else() branch after if(NOT CLP_NO_BUILD_CLP_S_TIMESTAMPPATTERN) that emits a status message when the timestamp pattern library is skipped.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 353 to 368, add an
else() branch after the existing if(NOT CLP_NO_BUILD_CLP_S_TIMESTAMPPATTERN)
block. Inside this else() branch, use the message() command to output a status
message indicating that the clp_s_timestamp_pattern library build is being
skipped. This will improve user experience by clearly communicating when the
build option is disabled.
| if(NOT CLP_NO_BUILD_CLP_S_JSONCONSTRUCTOR) | ||
| add_library( | ||
| clp_s_json_constructor | ||
| ${CLP_S_JSON_CONSTRUCTOR_SOURCES} | ||
| ) | ||
| add_library(clp_s::json_constructor ALIAS clp_s_json_constructor) | ||
| target_compile_features(clp_s_json_constructor PRIVATE cxx_std_20) | ||
| target_include_directories(clp_s_json_constructor PUBLIC ../) | ||
| target_link_libraries( | ||
| clp_s_json_constructor | ||
| PUBLIC | ||
| clp_s::archive_reader | ||
| PRIVATE | ||
| fmt::fmt | ||
| ${MONGOCXX_TARGET} | ||
| spdlog::spdlog | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
After the if(NOT CLP_NO_BUILD_CLP_S_JSONCONSTRUCTOR) block, include an else() branch that logs skipping the JSON constructor library.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 325 to 342, add an
else() branch after the existing if(NOT CLP_NO_BUILD_CLP_S_JSONCONSTRUCTOR)
block. In this else() branch, include a message() command to log that the JSON
constructor library build is being skipped when
CLP_NO_BUILD_CLP_S_JSONCONSTRUCTOR is set. This will provide clear feedback
during the build process when the library is not built.
| if(NOT CLP_NO_BUILD_CLP_S_CLP_DEPENDENCIES) | ||
| add_library( | ||
| clp_s_clp_dependencies | ||
| ${CLP_S_CLP_SOURCES} | ||
| ) | ||
| add_library(clp_s::clp_dependencies ALIAS clp_s_clp_dependencies) | ||
| target_compile_features(clp_s_clp_dependencies PRIVATE cxx_std_20) | ||
| target_link_libraries( | ||
| clp_s_clp_dependencies | ||
| PUBLIC | ||
| clp::string_utils | ||
| ystdlib::containers | ||
| PRIVATE | ||
| Boost::regex | ||
| ${CURL_LIBRARIES} | ||
| fmt::fmt | ||
| spdlog::spdlog | ||
| nlohmann_json::nlohmann_json | ||
| OpenSSL::Crypto | ||
| ystdlib::error_handling | ||
| ZStd::ZStd | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
Each if(NOT CLP_NO_BUILD_CLP_S_CLP_DEPENDENCIES) block correctly gates the target, but it’s helpful to include an else() branch with message(STATUS "... skipped ...") to inform users when it’s disabled.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt between lines 84 and 106, the
conditional block that skips building clp_s_clp_dependencies when
CLP_NO_BUILD_CLP_S_CLP_DEPENDENCIES is set lacks user feedback. Add an else()
branch after the if block with a message(STATUS "... skipped ...") statement to
inform users that the clp_s_clp_dependencies target build is disabled, improving
build transparency.
| if(NOT CLP_NO_BUILD_CLP_S_ARCHIVEWRITER) | ||
| add_library( | ||
| clp_s_archive_writer | ||
| ${CLP_S_ARCHIVE_WRITER_SOURCES} | ||
| ) | ||
| add_library(clp_s::archive_writer ALIAS clp_s_archive_writer) | ||
| target_compile_features(clp_s_archive_writer PRIVATE cxx_std_20) | ||
| target_include_directories(clp_s_archive_writer PUBLIC ../) | ||
| target_link_libraries( | ||
| clp_s_archive_writer | ||
| PUBLIC | ||
| absl::flat_hash_map | ||
| clp_s::io | ||
| simdjson::simdjson | ||
| PRIVATE | ||
| Boost::url | ||
| clp_s::clp_dependencies | ||
| clp_s::timestamp_pattern | ||
| ${CURL_LIBRARIES} | ||
| fmt::fmt | ||
| msgpack-cxx | ||
| nlohmann_json::nlohmann_json | ||
| spdlog::spdlog | ||
| ) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suggest adding a skip message when toggled off
The clp_s_archive_writer target is correctly guarded; add an else() branch to call message(STATUS "... skipped ...") for better feedback when disabled.
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt between lines 229 and 254, the build
of clp_s_archive_writer is conditionally disabled with if(NOT
CLP_NO_BUILD_CLP_S_ARCHIVEWRITER) but lacks feedback when skipped. Add an else()
branch after the endif() that calls message(STATUS "... skipped
clp_s_archive_writer ...") to provide clear status output when this target is
not built.
components/core/CMakeLists.txt
Outdated
| if(NOT CLP_NO_NEED_LZMA) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LIBLZMA_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibLZMA REQUIRED) | ||
| if(LIBLZMA_FOUND) | ||
| message(STATUS "Found Lzma ${LIBLZMA_VERSION_STRING}") | ||
| message(STATUS "Lzma library location: ${LIBLZMA_LIBRARIES}") | ||
| message(STATUS "Lzma Include Dir: ${LIBLZMA_INCLUDE_DIRS}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Lzma") | ||
| endif() | ||
| include_directories(${LIBLZMA_INCLUDE_DIRS}) | ||
| endif() |
There was a problem hiding this comment.
Avoid global include_directories for LZMA
Using include_directories(${LIBLZMA_INCLUDE_DIRS}) pollutes all targets. Instead, attach include paths to the specific targets that need LZMA via target_include_directories(<target> PRIVATE ${LIBLZMA_INCLUDE_DIRS}).
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt around lines 295 to 308, the use of
include_directories(${LIBLZMA_INCLUDE_DIRS}) applies the LZMA include paths
globally, which can cause unwanted side effects. To fix this, remove the global
include_directories call and instead add the LZMA include directories
specifically to the targets that require LZMA using
target_include_directories(<target> PRIVATE ${LIBLZMA_INCLUDE_DIRS}). Identify
the relevant targets and update their include directories accordingly.
components/core/CMakeLists.txt
Outdated
| if(NOT CLP_NO_NEED_ABSL) | ||
| find_package(absl REQUIRED) | ||
| if (absl_FOUND) | ||
| message(STATUS "Found absl ${absl_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup ANTLR Library | ||
| find_package(antlr4-runtime REQUIRED) | ||
| if (antlr4-runtime_FOUND) | ||
| message(STATUS "Found antlr4-runtime ${antlr4-runtime_VERSION}") | ||
| if(NOT CLP_NO_NEED_ANTLR) | ||
| find_package(antlr4-runtime REQUIRED) | ||
| if (antlr4-runtime_FOUND) | ||
| message(STATUS "Found antlr4-runtime ${antlr4-runtime_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup Boost Library | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(Boost_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(Boost 1.81 REQUIRED iostreams program_options filesystem system regex url) | ||
| if(Boost_FOUND) | ||
| message(STATUS "Found Boost ${Boost_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Boost") | ||
| if(NOT CLP_NO_NEED_BOOST) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(Boost_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(Boost 1.81 REQUIRED iostreams program_options filesystem system regex url) | ||
| if(Boost_FOUND) | ||
| message(STATUS "Found Boost ${Boost_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Boost") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(Catch2 REQUIRED) | ||
| if (Catch2_FOUND) | ||
| message(STATUS "Found Catch2 ${Catch2_VERSION}") | ||
| if(NOT CLP_NO_BUILD_TESTS) | ||
| find_package(Catch2 REQUIRED) | ||
| if (Catch2_FOUND) | ||
| message(STATUS "Found Catch2 ${Catch2_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(date REQUIRED) | ||
| if (date_FOUND) | ||
| message(STATUS "Found date ${date_VERSION}") | ||
| if(NOT CLP_NO_NEED_DATE) | ||
| find_package(date REQUIRED) | ||
| if (date_FOUND) | ||
| message(STATUS "Found date ${date_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(fmt REQUIRED) | ||
| if(fmt_FOUND) | ||
| message(STATUS "Found fmt ${fmt_VERSION}") | ||
| if(NOT CLP_NO_NEED_FMT) | ||
| find_package(fmt REQUIRED) | ||
| if(fmt_FOUND) | ||
| message(STATUS "Found fmt ${fmt_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(log_surgeon REQUIRED) | ||
| if(log_surgeon_FOUND) | ||
| message(STATUS "Found log_surgeon ${log_surgeon_VERSION}") | ||
| if(NOT CLP_NO_NEED_LOG_SURGEON) | ||
| find_package(log_surgeon REQUIRED) | ||
| if(log_surgeon_FOUND) | ||
| message(STATUS "Found log_surgeon ${log_surgeon_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(nlohmann_json REQUIRED) | ||
| if(nlohmann_json_FOUND) | ||
| message(STATUS "Found nlohmann_json ${nlohmann_json_VERSION}") | ||
| if(NOT CLP_NO_NEED_NLOHMANN_JSON) | ||
| find_package(nlohmann_json REQUIRED) | ||
| if(nlohmann_json_FOUND) | ||
| message(STATUS "Found nlohmann_json ${nlohmann_json_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(simdjson REQUIRED) | ||
| if(simdjson_FOUND) | ||
| message(STATUS "Found simdjson ${simdjson_VERSION}") | ||
| if(NOT CLP_NO_NEED_SIMDJSON) | ||
| find_package(simdjson REQUIRED) | ||
| if(simdjson_FOUND) | ||
| message(STATUS "Found simdjson ${simdjson_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(spdlog REQUIRED) | ||
| if(spdlog_FOUND) | ||
| message(STATUS "Found spdlog ${spdlog_VERSION}") | ||
| if(NOT CLP_NO_NEED_SPDLOG) | ||
| find_package(spdlog REQUIRED) | ||
| if(spdlog_FOUND) | ||
| message(STATUS "Found spdlog ${spdlog_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup libarchive | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LibArchive_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibArchive REQUIRED) | ||
| if(LibArchive_FOUND) | ||
| message(STATUS "Found LibArchive ${LibArchive_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for LibArchive") | ||
| if(NOT CLP_NO_NEED_LIBARCHIVE) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LibArchive_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibArchive REQUIRED) | ||
| if(LibArchive_FOUND) | ||
| message(STATUS "Found LibArchive ${LibArchive_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for LibArchive") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup libcurl | ||
| # By default, CURL does not provide static libraries | ||
| find_package(CURL 7.68.0 REQUIRED) | ||
| if(CURL_FOUND) | ||
| message(STATUS "Found CURL ${CURL_VERSION_STRING}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for CURL") | ||
| if(NOT CLP_NO_NEED_CURL) | ||
| find_package(CURL 7.68.0 REQUIRED) | ||
| if(CURL_FOUND) | ||
| message(STATUS "Found CURL ${CURL_VERSION_STRING}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for CURL") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find OpenSSL | ||
| find_package(OpenSSL REQUIRED) | ||
| if (OPENSSL_FOUND) | ||
| message(STATUS "Found OpenSSL (${OPENSSL_VERSION})") | ||
| else () | ||
| message(FATAL_ERROR "OpenSSL not found") | ||
| endif () | ||
| if(NOT CLP_NO_NEED_OPENSSL) | ||
| find_package(OpenSSL REQUIRED) | ||
| if(OPENSSL_FOUND) | ||
| message(STATUS "Found OpenSSL (${OPENSSL_VERSION})") | ||
| else() | ||
| message(FATAL_ERROR "OpenSSL not found") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup MariaDBClient library | ||
| 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) | ||
| if(MariaDBClient_FOUND) | ||
| message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient") | ||
| if(NOT CLP_NO_NEED_MARIADB) | ||
| 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) | ||
| if(MariaDBClient_FOUND) | ||
| message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup mongocxx | ||
| find_package(mongocxx REQUIRED) | ||
| message(STATUS "Found mongocxx ${mongocxx_VERSION}") | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(MONGOCXX_TARGET mongo::mongocxx_static) | ||
| else() | ||
| set(MONGOCXX_TARGET mongo::mongocxx_shared) | ||
| if(NOT CLP_NO_NEED_MONGOCXX) | ||
| find_package(mongocxx REQUIRED) | ||
| message(STATUS "Found mongocxx ${mongocxx_VERSION}") | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(MONGOCXX_TARGET mongo::mongocxx_static) | ||
| else() | ||
| set(MONGOCXX_TARGET mongo::mongocxx_shared) | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup msgpack | ||
| find_package(msgpack-cxx 7.0.0 REQUIRED) | ||
| if(msgpack-cxx_FOUND) | ||
| message(STATUS "Found msgpack-cxx ${msgpack-cxx_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find msgpack-cxx") | ||
| if(NOT CLP_NO_NEED_MSGPACKCXX) | ||
| find_package(msgpack-cxx 7.0.0 REQUIRED) | ||
| if(msgpack-cxx_FOUND) | ||
| message(STATUS "Found msgpack-cxx ${msgpack-cxx_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find msgpack-cxx") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(Threads REQUIRED) | ||
|
|
||
| find_package(yaml-cpp REQUIRED) | ||
| if(yaml-cpp_FOUND) | ||
| message(STATUS "Found yaml-cpp ${yaml-cpp_VERSION}") | ||
| if(NOT CLP_NO_NEED_YAMLCPP) | ||
| find_package(yaml-cpp REQUIRED) | ||
| if(yaml-cpp_FOUND) | ||
| message(STATUS "Found yaml-cpp ${yaml-cpp_VERSION}") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY up repetitive find_package guards with a helper macro
The pattern of if(NOT CLP_NO_NEED_…) wrapping each find_package block is consistent but verbose. Define a macro (e.g., clp_find_dependency(name, components)) in options.cmake to encapsulate the guard, find_package, and status message, then replace individual blocks with calls to that macro.
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt from lines 119 to 272, the repeated pattern of
wrapping each find_package call with if(NOT CLP_NO_NEED_…) guards and status
messages is verbose. To fix this, define a macro named clp_find_dependency in
options.cmake that takes the package name and optional components, performs the
if guard, calls find_package with REQUIRED, and prints the status message if
found. Then replace each individual guarded find_package block in this file with
calls to this macro, passing the appropriate package name and components, to DRY
up the code and reduce repetition.
There was a problem hiding this comment.
I'm not sure if we're giving the user too much flexibility by allowing them to enable or disable every dependency individually. Is there a clean approach where we can handle dependency resolution ourselves — for example, by automatically including only the dependencies required by the targets the user selects?
| PROPERTIES | ||
| RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}" | ||
| ) | ||
| if(NOT CLP_NO_BUILD_BINARIES) |
There was a problem hiding this comment.
Do we want to control each individual binary?
There was a problem hiding this comment.
Yeah it could be worthwhile. I think it doesn't matter for the immediate use-case (exposing enough options so that external projects can pull in individual libraries without needing to pull in all of our targets and dependencies), but it could be useful e.g. to create more targeted installs of clp.
I don't think cmake has functionality to do this automatically for the |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/core/CMakeLists.txt (1)
295-308: Avoid global include_directories
Usinginclude_directories(${LIBLZMA_INCLUDE_DIRS})applies LZMA headers to every target. Switch to target-specific imports:- include_directories(${LIBLZMA_INCLUDE_DIRS}) + target_include_directories(<your_target> PRIVATE ${LIBLZMA_INCLUDE_DIRS})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/CMakeLists.txt(4 hunks)components/core/src/clp_s/search/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (4)
components/core/src/clp_s/search/CMakeLists.txt (1)
33-52: Conditional build guard applied correctly
Wrapping the entireclp_s_searchtarget definition inif(NOT CLP_NO_BUILD_CLP_S_SEARCH)ensures users can disable this library. Verify that the corresponding option is declared inoptions.cmake.Please confirm that you have:
option(CLP_NO_BUILD_CLP_S_SEARCH "Disable building the clp_s_search library" OFF)in your options file.- Updated documentation to note the presence of this new flag.
components/core/CMakeLists.txt (3)
1-1: Bumped minimum CMake version
Updating to 3.21 aligns with the use ofPROJECT_IS_TOP_LEVELandSOURCE_SUBDIR.
12-13: Include centralized options early
Loadingoptions.cmakebefore any targets ensures allCLP_NO_*toggles are available throughout the build.
712-756: Test target conditional is correct
Enclosing the largeunitTesttarget inif(NOT CLP_NO_BUILD_TESTS)respects the new test toggle. EnsureCLP_NO_BUILD_TESTSis declared and documented inoptions.cmake.
components/core/CMakeLists.txt
Outdated
| if(NOT CLP_NO_NEED_ABSL) | ||
| find_package(absl REQUIRED) | ||
| if (absl_FOUND) | ||
| message(STATUS "Found absl ${absl_VERSION}") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY opportunity: consolidate dependency guards
The repeated pattern:
if(NOT CLP_NO_NEED_ABSL)
find_package(absl REQUIRED)
…
endif()
occurs throughout. Consider defining a helper macro (e.g. clp_find_dependency(name, components)) in options.cmake to reduce boilerplate.
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt around lines 119 to 124, there is repetitive
code for checking and finding dependencies like absl guarded by conditions such
as NOT CLP_NO_NEED_ABSL. To fix this, create a helper macro named
clp_find_dependency in options.cmake that takes the dependency name and
components as parameters, encapsulating the conditional find_package logic. Then
replace all repeated patterns in CMakeLists.txt with calls to this macro to
reduce boilerplate and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
components/core/src/clp/clo/CMakeLists.txt (1)
154-184: Duplicate: Confirm declaration and documentation ofCLP_BUILD_BINARIES
This guard depends on an option (inverted fromCLP_NO_BUILD_BINARIES)—ensureCLP_BUILD_BINARIESis defined viaoption(...)incomponents/core/cmake/Options/options.cmakewith a clear description and default ON. Without it, consumers can’t toggle binary builds.Shell script to verify:
#!/bin/bash # Check for option declaration of CLP_NO_BUILD_BINARIES and CLP_BUILD_BINARIES rg -n "option\(\s*CLP_NO_BUILD_BINARIES" components/core/cmake/Options/options.cmake rg -n "option\(\s*CLP_BUILD_BINARIES" components/core/cmake/Options/options.cmakecomponents/core/src/clp/string_utils/CMakeLists.txt (1)
5-14: Duplicate: Verify option declaration forCLP_BUILD_CLP_STRING_UTILS
EnsureCLP_BUILD_CLP_STRING_UTILSis defined viaoption(...)incomponents/core/cmake/Options/options.cmakewith a descriptive help string and default ON, so consumers can togglestring_utilsbuilds.Shell script:
#!/bin/bash rg -n "option\(\s*CLP_BUILD_CLP_STRING_UTILS" components/core/cmake/Options/options.cmakecomponents/core/src/clp_s/indexer/CMakeLists.txt (1)
83-108: Duplicate: ConfirmCLP_BUILD_BINARIESoption usage
Theindexerexecutable is wrapped inif(CLP_BUILD_BINARIES)—verify the corresponding option (inverted fromCLP_NO_BUILD_BINARIES) is declared and documented incomponents/core/cmake/Options/options.cmake.Shell script:
#!/bin/bash rg -n "option\(\s*CLP_NO_BUILD_BINARIES" components/core/cmake/Options/options.cmake rg -n "option\(\s*CLP_BUILD_BINARIES" components/core/cmake/Options/options.cmake
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
components/core/CMakeLists.txt(4 hunks)components/core/cmake/Options/options.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/regex_utils/CMakeLists.txt(1 hunks)components/core/src/clp/string_utils/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 (1)
components/core/src/clp_s/search/sql/CMakeLists.txt (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
🔇 Additional comments (14)
components/core/src/clp_s/search/CMakeLists.txt (1)
33-52: I couldn’t findCLP_BUILD_CLP_S_SEARCHdeclared in the central options file—let’s search the entire repo to verify its definition:#!/bin/bash # Search for any declaration or usage of the option rg -n "CLP_BUILD_CLP_S_SEARCH" .components/core/src/reducer/CMakeLists.txt (1)
41-61: ```shell
#!/bin/bashSearch for option declarations anywhere in the repo
rg -n 'option(.*CLP_BUILD_BINARIES' .
rg -n 'option(.*CLP_NO_BUILD_BINARIES' .Fallback: look for any mentions of the variables
rg -n 'CLP_BUILD_BINARIES' .
rg -n 'CLP_NO_BUILD_BINARIES' .</details> <details> <summary>components/core/src/clp_s/search/kql/CMakeLists.txt (1)</summary> `14-37`: Let’s try a simpler search to locate the option definition: ```shell #!/bin/bash # 1) Look for any mention of the flag grep -n 'CLP_BUILD_CLP_S_SEARCH_KQL' components/core/cmake/Options/options.cmake || true # 2) Specifically try to find an option declaration rg -n 'option.*CLP_BUILD_CLP_S_SEARCH_KQL' -A2 components/core/cmake/Options/options.cmake || truecomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txt (1)
42-60: Gatingmake-dictionaries-readablebehindCLP_BUILD_BINARIESis correct
Theif(CLP_BUILD_BINARIES)block cleanly encapsulates all executable creation, compile features, linkage, and output settings.components/core/src/clp/regex_utils/CMakeLists.txt (1)
8-27: Conditional build ofregex_utilsis correctly implemented
Theif(CLP_BUILD_CLP_REGEX_UTILS)guard wraps the library definition, alias, include paths, compile features, and PUBLIC/PRIVATE linkage as expected.components/core/src/clp_s/search/sql/CMakeLists.txt (1)
14-36: Optional inclusion ofclp_s_search_sqllooks good
Alladd_library, aliasing, compile features, include directories, and target linkage are scoped withinif(CLP_BUILD_CLP_S_SEARCH_SQL)following the project’s pattern.components/core/src/clp/clg/CMakeLists.txt (1)
126-157: Conditional build ofclgexecutable is consistent
Wrapping theclgtarget inif(CLP_BUILD_BINARIES)aligns with other binaries. Include directories, compile features, and linked libraries are properly scoped.components/core/src/clp_s/search/ast/CMakeLists.txt (1)
52-68: Conditional build guard forclp_s_search_astis correct
Enclosing the library creation inif(CLP_BUILD_CLP_S_SEARCH_AST)aligns with the centralized options approach and enables optional builds without modifying source code.components/core/src/glt/glt/CMakeLists.txt (1)
174-203: Guardgltexecutable withCLP_BUILD_BINARIES
Theadd_executable(glt ...)block has been properly wrapped inif(CLP_BUILD_BINARIES), giving external projects control over building this binary.components/core/src/clp_s/CMakeLists.txt (1)
84-106: Consistent application of build flags for CLP-S subcomponents
Alladd_libraryandadd_executabletargets in this file are now correctly gated by their respectiveCLP_BUILD_*flags, enabling fine-grained control as per the PR objectives.Also applies to: 133-147, 169-186, 229-253, 292-314, 325-342, 353-368, 382-412
components/core/CMakeLists.txt (4)
1-1: Bump minimum required CMake to 3.21
Updating to 3.21 is necessary for usingPROJECT_IS_TOP_LEVELandSOURCE_SUBDIRfeatures.
12-14: Include centralized options file
options.cmakenow holds allCLP_BUILD_*andCLP_NEED_*definitions, ensuring consistent flag declarations across the build.
15-17: Initialize and validate CLP flags
Callingvalidate_and_setup_all_clp_dependency_flags()andconvert_clp_dependency_properties_to_variables()establishes the new flag-based workflow.
715-759: Conditional creation ofunitTesttarget
TheunitTestexecutable is correctly wrapped inif(CLP_BUILD_TESTS), allowing test builds to be disabled.
| if(CLP_NEED_ABSL) | ||
| find_package(absl REQUIRED) | ||
| if (absl_FOUND) | ||
| message(STATUS "Found absl ${absl_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup ANTLR Library | ||
| find_package(antlr4-runtime REQUIRED) | ||
| if (antlr4-runtime_FOUND) | ||
| message(STATUS "Found antlr4-runtime ${antlr4-runtime_VERSION}") | ||
| if(CLP_NEED_ANTLR) | ||
| find_package(antlr4-runtime REQUIRED) | ||
| if (antlr4-runtime_FOUND) | ||
| message(STATUS "Found antlr4-runtime ${antlr4-runtime_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup Boost Library | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(Boost_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(Boost 1.81 REQUIRED iostreams program_options filesystem system regex url) | ||
| if(Boost_FOUND) | ||
| message(STATUS "Found Boost ${Boost_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Boost") | ||
| if(CLP_NEED_BOOST) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(Boost_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(Boost 1.81 REQUIRED iostreams program_options filesystem system regex url) | ||
| if(Boost_FOUND) | ||
| message(STATUS "Found Boost ${Boost_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Boost") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(Catch2 REQUIRED) | ||
| if (Catch2_FOUND) | ||
| message(STATUS "Found Catch2 ${Catch2_VERSION}") | ||
| if(CLP_NEED_CATCH2) | ||
| find_package(Catch2 REQUIRED) | ||
| if (Catch2_FOUND) | ||
| message(STATUS "Found Catch2 ${Catch2_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(date REQUIRED) | ||
| if (date_FOUND) | ||
| message(STATUS "Found date ${date_VERSION}") | ||
| if(CLP_NEED_DATE) | ||
| find_package(date REQUIRED) | ||
| if (date_FOUND) | ||
| message(STATUS "Found date ${date_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(fmt REQUIRED) | ||
| if(fmt_FOUND) | ||
| message(STATUS "Found fmt ${fmt_VERSION}") | ||
| if(CLP_NEED_FMT) | ||
| find_package(fmt REQUIRED) | ||
| if(fmt_FOUND) | ||
| message(STATUS "Found fmt ${fmt_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(log_surgeon REQUIRED) | ||
| if(log_surgeon_FOUND) | ||
| message(STATUS "Found log_surgeon ${log_surgeon_VERSION}") | ||
| if(CLP_NEED_LOG_SURGEON) | ||
| find_package(log_surgeon REQUIRED) | ||
| if(log_surgeon_FOUND) | ||
| message(STATUS "Found log_surgeon ${log_surgeon_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(nlohmann_json REQUIRED) | ||
| if(nlohmann_json_FOUND) | ||
| message(STATUS "Found nlohmann_json ${nlohmann_json_VERSION}") | ||
| if(CLP_NEED_NLOHMANN_JSON) | ||
| find_package(nlohmann_json REQUIRED) | ||
| if(nlohmann_json_FOUND) | ||
| message(STATUS "Found nlohmann_json ${nlohmann_json_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(simdjson REQUIRED) | ||
| if(simdjson_FOUND) | ||
| message(STATUS "Found simdjson ${simdjson_VERSION}") | ||
| if(CLP_NEED_SIMDJSON) | ||
| find_package(simdjson REQUIRED) | ||
| if(simdjson_FOUND) | ||
| message(STATUS "Found simdjson ${simdjson_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(spdlog REQUIRED) | ||
| if(spdlog_FOUND) | ||
| message(STATUS "Found spdlog ${spdlog_VERSION}") | ||
| if(CLP_NEED_SPDLOG) | ||
| find_package(spdlog REQUIRED) | ||
| if(spdlog_FOUND) | ||
| message(STATUS "Found spdlog ${spdlog_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup libarchive | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LibArchive_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibArchive REQUIRED) | ||
| if(LibArchive_FOUND) | ||
| message(STATUS "Found LibArchive ${LibArchive_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for LibArchive") | ||
| if(CLP_NEED_LIBARCHIVE) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LibArchive_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibArchive REQUIRED) | ||
| if(LibArchive_FOUND) | ||
| message(STATUS "Found LibArchive ${LibArchive_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for LibArchive") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup libcurl | ||
| # By default, CURL does not provide static libraries | ||
| find_package(CURL 7.68.0 REQUIRED) | ||
| if(CURL_FOUND) | ||
| message(STATUS "Found CURL ${CURL_VERSION_STRING}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for CURL") | ||
| if(CLP_NEED_CURL) | ||
| find_package(CURL 7.68.0 REQUIRED) | ||
| if(CURL_FOUND) | ||
| message(STATUS "Found CURL ${CURL_VERSION_STRING}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for CURL") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find OpenSSL | ||
| find_package(OpenSSL REQUIRED) | ||
| if (OPENSSL_FOUND) | ||
| message(STATUS "Found OpenSSL (${OPENSSL_VERSION})") | ||
| else () | ||
| message(FATAL_ERROR "OpenSSL not found") | ||
| endif () | ||
| if(CLP_NEED_OPENSSL) | ||
| find_package(OpenSSL REQUIRED) | ||
| if(OPENSSL_FOUND) | ||
| message(STATUS "Found OpenSSL (${OPENSSL_VERSION})") | ||
| else() | ||
| message(FATAL_ERROR "OpenSSL not found") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup MariaDBClient library | ||
| 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) | ||
| if(MariaDBClient_FOUND) | ||
| message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient") | ||
| if(CLP_NEED_MARIADB) | ||
| 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) | ||
| if(MariaDBClient_FOUND) | ||
| message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup mongocxx | ||
| find_package(mongocxx REQUIRED) | ||
| message(STATUS "Found mongocxx ${mongocxx_VERSION}") | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(MONGOCXX_TARGET mongo::mongocxx_static) | ||
| else() | ||
| set(MONGOCXX_TARGET mongo::mongocxx_shared) | ||
| if(CLP_NEED_MONGOCXX) | ||
| find_package(mongocxx REQUIRED) | ||
| message(STATUS "Found mongocxx ${mongocxx_VERSION}") | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(MONGOCXX_TARGET mongo::mongocxx_static) | ||
| else() | ||
| set(MONGOCXX_TARGET mongo::mongocxx_shared) | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup msgpack | ||
| find_package(msgpack-cxx 7.0.0 REQUIRED) | ||
| if(msgpack-cxx_FOUND) | ||
| message(STATUS "Found msgpack-cxx ${msgpack-cxx_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find msgpack-cxx") | ||
| if(CLP_NEED_MSGPACKCXX) | ||
| find_package(msgpack-cxx 7.0.0 REQUIRED) | ||
| if(msgpack-cxx_FOUND) | ||
| message(STATUS "Found msgpack-cxx ${msgpack-cxx_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find msgpack-cxx") | ||
| endif() | ||
| endif() | ||
|
|
||
| find_package(Threads REQUIRED) | ||
|
|
||
| find_package(yaml-cpp REQUIRED) | ||
| if(yaml-cpp_FOUND) | ||
| message(STATUS "Found yaml-cpp ${yaml-cpp_VERSION}") | ||
| if(CLP_NEED_YAMLCPP) | ||
| find_package(yaml-cpp REQUIRED) | ||
| if(yaml-cpp_FOUND) | ||
| message(STATUS "Found yaml-cpp ${yaml-cpp_VERSION}") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Add ystdlib | ||
| set(YSTDLIB_CPP_BUILD_TESTING OFF) | ||
| add_subdirectory("${CLP_YSTDLIB_SOURCE_DIRECTORY}" "${CMAKE_BINARY_DIR}/ystdlib" EXCLUDE_FROM_ALL) | ||
| if(CLP_NEED_YSTDLIB) | ||
| set(YSTDLIB_CPP_BUILD_TESTING OFF) | ||
| add_subdirectory("${CLP_YSTDLIB_SOURCE_DIRECTORY}" "${CMAKE_BINARY_DIR}/ystdlib" EXCLUDE_FROM_ALL) | ||
| endif() | ||
|
|
||
| # Find and setup ZStd Library | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(ZStd_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(ZStd 1.4.4 REQUIRED) | ||
| if(ZStd_FOUND) | ||
| message(STATUS "Found ZStd ${ZStd_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for ZStd") | ||
| if(CLP_NEED_ZSTD) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(ZStd_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(ZStd 1.4.4 REQUIRED) | ||
| if(ZStd_FOUND) | ||
| message(STATUS "Found ZStd ${ZStd_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for ZStd") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Find and setup LZMA Library | ||
| # TODO: Add a script in ./cmake/Modules to properly import LZMA in find_package()'s module mode | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LIBLZMA_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibLZMA REQUIRED) | ||
| if(LIBLZMA_FOUND) | ||
| message(STATUS "Found Lzma ${LIBLZMA_VERSION_STRING}") | ||
| message(STATUS "Lzma library location: ${LIBLZMA_LIBRARIES}") | ||
| message(STATUS "Lzma Include Dir: ${LIBLZMA_INCLUDE_DIRS}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Lzma") | ||
| if(CLP_NEED_LZMA) | ||
| if(CLP_USE_STATIC_LIBS) | ||
| set(LIBLZMA_USE_STATIC_LIBS ON) | ||
| endif() | ||
| find_package(LibLZMA REQUIRED) | ||
| if(LIBLZMA_FOUND) | ||
| message(STATUS "Found Lzma ${LIBLZMA_VERSION_STRING}") | ||
| message(STATUS "Lzma library location: ${LIBLZMA_LIBRARIES}") | ||
| message(STATUS "Lzma Include Dir: ${LIBLZMA_INCLUDE_DIRS}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Lzma") | ||
| endif() | ||
| include_directories(${LIBLZMA_INCLUDE_DIRS}) | ||
| endif() | ||
| include_directories(${LIBLZMA_INCLUDE_DIRS}) | ||
|
|
||
| # sqlite dependencies | ||
| set(sqlite_DYNAMIC_LIBS "dl;m;pthread") | ||
| include(cmake/Modules/FindLibraryDependencies.cmake) | ||
| FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") | ||
| if(CLP_NEED_SQLITE) | ||
| set(sqlite_DYNAMIC_LIBS "dl;m;pthread") | ||
| include(cmake/Modules/FindLibraryDependencies.cmake) | ||
| FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard external dependency resolution with flags
Every find_package call is now wrapped in if(CLP_NEED_*), preventing unnecessary dependency discovery and aligning with the PR’s goal of minimal external requirements.
Consider defining a helper macro in options.cmake (e.g., clp_find_dependency(name, components)) to DRY up repetitive guard + find_package blocks.
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt from lines 122 to 319, the code wraps each
find_package call with a CLP_NEED_* guard to conditionally include dependencies.
To improve maintainability and reduce repetition, define a helper macro in
options.cmake named clp_find_dependency that takes the package name and optional
components, and internally performs the if guard and find_package call. Then
replace each guarded find_package block with calls to this macro, passing the
appropriate arguments, to DRY up the code and keep consistent conditional
dependency resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/cmake/Options/options.cmake(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (3)
components/core/cmake/Options/options.cmake (3)
1-91: Well-structured build target options.The option declarations follow a consistent naming convention and logical hierarchy. Defaulting all options to ON provides good backward compatibility while enabling selective component disabling for external projects.
376-448: Excellent coordination function design.The master function provides a clean, systematic approach to dependency validation and setup. The conditional structure ensures efficient processing of only enabled targets, and the explanatory comment about
clp::string_utilshaving no dependencies adds helpful clarity.
450-493: Conversion function correctly implemented.The function properly uses the
get_property/setpattern withPARENT_SCOPEto expose dependency flags as variables. The alphabetical ordering and consistent structure make it maintainable and easy to verify completeness.
I'm probably missing something---why do we need |
Does moving the |
| CLP_NEED_LOG_SURGEON | ||
| CLP_NEED_LIBARCHIVE |
There was a problem hiding this comment.
| CLP_NEED_LOG_SURGEON | |
| CLP_NEED_LIBARCHIVE | |
| CLP_NEED_LIBARCHIVE | |
| CLP_NEED_LOG_SURGEON |
| set_clp_need_flags( | ||
| CLP_NEED_BOOST | ||
| CLP_NEED_FMT | ||
| CLP_NEED_MSGPACKCXX |
There was a problem hiding this comment.
I might be missing something, but is this required? I don't see it in target_link_libs.
There was a problem hiding this comment.
You're right -- missed that one as well.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/cmake/Options/options.cmake(1 hunks)components/core/src/clp_s/CMakeLists.txt(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: filter-relevant-changes
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (6)
components/core/src/clp_s/CMakeLists.txt (1)
84-106: LGTM: Conditional building properly implemented.All library and executable targets are correctly wrapped with their respective build flags, enabling selective component compilation. The implementation is consistent and follows the established pattern.
Also applies to: 133-147, 169-186, 229-253, 292-315, 325-342, 353-368, 382-412
components/core/cmake/Options/options.cmake (5)
1-90: LGTM: Comprehensive build options definition.The build options are well-organized, consistently named, and provide appropriate descriptions. The alphabetical organization within logical groupings makes the file maintainable.
92-106: LGTM: Robust dependency validation function.The validation logic properly checks that required dependencies are enabled and provides clear error messages. The parameter validation ensures the function is called correctly.
108-115: LGTM: Clean implementation for setting dependency flags.The helper function efficiently sets multiple dependency flags as directory properties using a simple loop pattern.
117-442: LGTM: Comprehensive dependency management system.The validation and dependency setting functions follow a consistent pattern and correctly model the inter-component dependencies. The master validation function
validate_and_setup_all_clp_dependency_flagsproperly orchestrates the entire dependency resolution process.
445-474: LGTM: Well-implemented property-to-variable conversion.The function efficiently converts directory properties to variables using the clean list + loop approach suggested in previous reviews. This addresses the CMake scoping issues mentioned in the PR objectives.
| if(CLP_BUILD_CLP_S_CLP_DEPENDENCIES) | ||
| add_library( | ||
| clp_s_clp_dependencies | ||
| ${CLP_S_CLP_SOURCES} | ||
| ) | ||
| add_library(clp_s::clp_dependencies ALIAS clp_s_clp_dependencies) | ||
| target_compile_features(clp_s_clp_dependencies PRIVATE cxx_std_20) | ||
| target_link_libraries( | ||
| clp_s_clp_dependencies | ||
| PUBLIC | ||
| clp::string_utils | ||
| ystdlib::containers | ||
| PRIVATE | ||
| Boost::regex | ||
| ${CURL_LIBRARIES} | ||
| fmt::fmt | ||
| spdlog::spdlog | ||
| nlohmann_json::nlohmann_json | ||
| OpenSSL::Crypto | ||
| ystdlib::error_handling | ||
| ZStd::ZStd | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding user feedback for disabled components.
While the conditional building works correctly, adding else() clauses with status messages would improve build transparency when components are disabled. For example:
if(CLP_BUILD_CLP_S_CLP_DEPENDENCIES)
add_library(clp_s_clp_dependencies ${CLP_S_CLP_SOURCES})
# ... rest of configuration
else()
message(STATUS "Skipping clp_s_clp_dependencies (CLP_BUILD_CLP_S_CLP_DEPENDENCIES=OFF)")
endif()Also applies to: 133-147, 169-186, 229-253, 292-315, 325-342, 353-368, 382-412
🤖 Prompt for AI Agents
In components/core/src/clp_s/CMakeLists.txt around lines 84 to 106, the
conditional blocks that build components lack else() clauses to provide user
feedback when those components are disabled. To fix this, add else() clauses
after each if() block that outputs a message(STATUS ...) indicating the
component is being skipped due to the corresponding build flag being OFF. This
improves build transparency and user awareness. Apply the same pattern to the
other specified line ranges as well.
3 participants
Description
This PR adds flags that external projects can use to disable almost every part of the clp core build. This allows external projects to depend on some parts of clp without needing to install dependencies used by components the external project does not need.
We also bump the required cmake version to 3.21 to ensure all the cmake features we use are supported (in particular the
SOURCE_SUBDIRargument forFetchContent_Declare/ExternalProject_Addin 3.18 andPROJECT_IS_TOP_LEVELin 3.21).These flags are all prefixed with
CLP_BUILD_and the corresponding targets are wrapped inif(CLP_BUILD_...). All of theseCLP_BUILD_flags are cache options defaulting to true, so by default are targets are built.These flags are used to derive a set of
CLP_NEED_Xflags that indicate which library dependencies are required to satisfy all of the targets being built. Eachfind_packagecall is wrapped by theCLP_NEED_Xflag for the corresponding library.We write one
validate_x_dependenciesandset_x_dependenciesfunction for eachCLP_BUILD_flag. The validate function for each target ensures that theCLP_BUILD_Xflags for all of its dependencies are true, and emits aFATAL_ERRORmessage otherwise. The set dependencies function for a target sets all of theCLP_NEED_flags directly linked against by the target.Together these functions ensure that
CLP_BUILD_Xoptionsfind_packageis called for only the set of libraries actually needed by the buildadd_libraryandadd_executableare called only for the targets whereCLP_BUILD_Xis trueFor convenience we wrap all of the validate/set functions in a function called
validate_and_setup_all_clp_dependency_flags. To allow flags set inside of these functions to be exposed as variables to the caller users need to follow a call to that function withconvert_clp_dependency_properties_to_variables.Checklist
breaking change.
Validation performed
clp_s::searchSummary by CodeRabbit
Summary by CodeRabbit
New Features
Chores