build(deps): Add task-based installations for LZ4 and Zstandard dependencies; Use latest versions for source-based installations. #1156
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 5835dbc.
Contributor
WalkthroughThis change modernizes and standardizes the handling of the Zstandard (zstd) and LZ4 compression libraries in the build system. It removes custom CMake find modules, updates dependency installation scripts and documentation, and introduces new dependency management tasks to automate building and linking these libraries with consistent target variables. Changes
Sequence Diagram(s)sequenceDiagram
participant BuildSystem as CMake/Task Runner
participant Zstd as Zstandard Library
participant LZ4 as LZ4 Library
BuildSystem->>Zstd: find_package(zstd 1.5.7 REQUIRED)
BuildSystem->>BuildSystem: Set zstd_TARGET (static/shared)
BuildSystem->>LZ4: (if needed) find_package(lz4 1.10.0)
BuildSystem->>BuildSystem: Use zstd_TARGET for target_link_libraries
BuildSystem->>BuildSystem: Use new dependency tasks for lz4/zstd install
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (12)📓 Common learnings📚 Learning: 2025-08-09T04:07:27.055ZApplied to files:
📚 Learning: 2025-07-23T09:54:45.185ZApplied to files:
📚 Learning: 2025-08-06T08:10:26.381ZApplied to files:
📚 Learning: 2025-08-06T07:32:28.462ZApplied to files:
📚 Learning: 2025-08-04T17:26:17.098ZApplied to files:
📚 Learning: 2025-07-24T21:56:05.806ZApplied to files:
📚 Learning: 2025-06-02T18:22:24.060ZApplied to files:
📚 Learning: 2025-08-04T17:23:14.646ZApplied to files:
📚 Learning: 2025-05-02T22:11:55.711ZApplied to files:
📚 Learning: 2025-07-07T17:41:15.655ZApplied to files:
📚 Learning: 2025-08-03T18:56:07.366ZApplied to files:
⏰ 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)
🔇 Additional comments (4)
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/core/src/clp/clo/CMakeLists.txt (1)
177-178: Same linking concern as forindexer– duplicate noteSee the comment on
indexer/CMakeLists.txtabout ensuringzstd_TARGETexports the real zstd link flags.components/core/src/clp_s/search/CMakeLists.txt (1)
57-58: Same linking concern as for other targets – duplicate note
clp_s_searchnow relies onzstd_TARGET; please verify the interface target is correctly defined as noted earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
components/core/CMakeLists.txt(3 hunks)components/core/cmake/Modules/FindLZ4.cmake(0 hunks)components/core/cmake/Modules/FindZStd.cmake(0 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(6 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/tools/scripts/lib_install/lz4.sh(0 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh(0 hunks)components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh(0 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh(0 hunks)components/core/tools/scripts/lib_install/zstandard.sh(0 hunks)docs/src/dev-guide/components-core/index.md(2 hunks)taskfiles/deps/main.yaml(4 hunks)taskfiles/deps/utils.yaml(1 hunks)
💤 Files with no reviewable changes (7)
- components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
- components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh
- components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh
- components/core/tools/scripts/lib_install/zstandard.sh
- components/core/cmake/Modules/FindZStd.cmake
- components/core/tools/scripts/lib_install/lz4.sh
- components/core/cmake/Modules/FindLZ4.cmake
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
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: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: 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: 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.
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
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/tools/scripts/lib_install/macos/install-all.shcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txttaskfiles/deps/utils.yamldocs/src/dev-guide/components-core/index.mdtaskfiles/deps/main.yamlcomponents/core/CMakeLists.txt
📚 Learning: in the clp project, antlr code generation at build time is being removed by another pr. when reviewi...
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/glt/glt/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtdocs/src/dev-guide/components-core/index.mdcomponents/core/CMakeLists.txt
📚 Learning: in clp's custom cmake find modules, `findstaticlibrarydependencies` populates the `*_dynamic_libs` v...
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/glt/glt/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtdocs/src/dev-guide/components-core/index.mdtaskfiles/deps/main.yamlcomponents/core/CMakeLists.txt
📚 Learning: in the clp project, manually setting ldflags for library paths can cause runtime errors because it i...
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/glt/glt/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txttaskfiles/deps/main.yamlcomponents/core/CMakeLists.txt
📚 Learning: in clp's custom cmake find modules (like findbzip2.cmake), when building with dynamic libraries, the...
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/glt/glt/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txttaskfiles/deps/main.yamlcomponents/core/CMakeLists.txt
📚 Learning: in the clp codebase, standard headers like `` and `` in alphabetical order (as seen...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtdocs/src/dev-guide/components-core/index.mdcomponents/core/CMakeLists.txt
📚 Learning: in the clp project, c++20 is used, allowing the utilization of c++20 features such as class template...
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: the team plans to systematically improve include paths by adding `${project_source_dir}/src` to cmak...
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.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/glt/glt/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txttaskfiles/deps/utils.yaml
📚 Learning: in 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floa...
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Applied to files:
components/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp/make_dictionaries_readable/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in clp installation scripts, consistency across platform scripts is prioritized over defensive progr...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
components/core/tools/scripts/lib_install/macos/install-all.shtaskfiles/deps/main.yaml
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
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/tools/scripts/lib_install/macos/install-all.shdocs/src/dev-guide/components-core/index.md
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across platfor...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Applied to files:
components/core/tools/scripts/lib_install/macos/install-all.sh
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across differe...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.
Applied to files:
components/core/tools/scripts/lib_install/macos/install-all.sh
📚 Learning: for temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_ins...
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_install/`, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
Applied to files:
components/core/tools/scripts/lib_install/macos/install-all.sh
📚 Learning: in cmakelists.txt files, anlowee prefers to explicitly list source files one by one rather than usin...
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-19
Timestamp: 2025-05-26T18:39:51.727Z
Learning: In CMakeLists.txt files, anlowee prefers to explicitly list source files one by one rather than using file(GLOB ...) or file(GLOB_RECURSE ...) to automatically include files, maintaining consistency with other CMake files in the project.
Applied to files:
components/core/src/clp/clo/CMakeLists.txtcomponents/core/src/clp/clg/CMakeLists.txt
📚 Learning: in the c++ `filedecompressor` implementations at `components/core/src/clp/clp/filedecompressor.cpp` ...
Learnt from: haiqi96
PR: y-scope/clp#619
File: components/core/src/clp/clp/decompression.cpp:313-313
Timestamp: 2024-12-05T16:32:21.507Z
Learning: In the C++ `FileDecompressor` implementations at `components/core/src/clp/clp/FileDecompressor.cpp` and `components/core/src/glt/glt/FileDecompressor.cpp`, the `temp_output_path` variable and associated logic are used to handle multiple compressed files with the same name, and should be kept. This logic is separate from the temporary output directory code removed in PR #619 and is necessary for proper file handling.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .c...
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfiles/deps/utils.yamltaskfiles/deps/main.yaml
📚 Learning: the clp project team has decided to refrain from using include directives in their documentation at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/dev-guide/components-core/index.md
📚 Learning: for the clp-core debian package creation script, strict version format validation is considered unne...
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Applied to files:
docs/src/dev-guide/components-core/index.md
📚 Learning: the clp project team prefers to use video content to demonstrate detailed procedural steps (like tar...
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
docs/src/dev-guide/components-core/index.md
📚 Learning: in the file `components/core/tests/test-ffi_keyvaluepairlogevent.cpp`, including `
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:
docs/src/dev-guide/components-core/index.md
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.
docs/src/dev-guide/components-core/index.md⏰ 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: 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)
🔇 Additional comments (13)
components/core/tools/scripts/lib_install/macos/install-all.sh (1)
42-52: Removal oflz4/zstdHomebrew formulas aligns with new task-based workflow—verify path propagation.Dropping the brew formulas avoids redundant system-wide installs and keeps the script consistent with the new Taskfile-driven builds.
Please double-check that the task output prefix for both libs (e.g.,${CLP_DEPS_INSTALL_PREFIX}) is injected intoCMAKE_PREFIX_PATHon macOS CI runners; otherwise,find_packagecould still resolve the brewed copies left on long-lived dev machines and mask configuration issues.components/core/src/glt/glt/CMakeLists.txt (1)
195-196: Switched tozstd_TARGET—looks correct.The interface target keeps linkage uniform after the Find module removal. No further action required here.
components/core/src/clp/clp/CMakeLists.txt (1)
192-193:zstd_TARGETreplacement approved.Change is mechanically-sound and consistent with the new root-level definition.
components/core/src/clp/clg/CMakeLists.txt (1)
149-150: Uniform compression target applied—LGTM.Linking against the consolidated interface target improves maintainability; no issues spotted.
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt (1)
52-53: Good update to shared interface target.The executable now links through
zstd_TARGET, matching the rest of the codebase.components/core/src/clp_s/indexer/CMakeLists.txt (1)
100-101: Verify thatzstd_TARGETis created before this point and carries link flagsThe previous imported target
ZStd::ZStdprovided the actualzstdlink libraries and usage requirements. The new interface target must be:
- Added before any sub-directory that consumes it, and
- Populated with the correct
$<LINK_ONLY:...>orINTERFACE_LINK_LIBRARIESso the final link line still contains-lzstd(or the static archive).Please confirm that
components/core/CMakeLists.txt(or an earlier include) callsfind_package(ZSTD 1.5.7 REQUIRED)and then populatesadd_library(zstd_TARGET INTERFACE) target_link_libraries(zstd_TARGET INTERFACE ZSTD::ZSTD)or an equivalent static/-shared switch.
If this is already in place, no further change is needed.docs/src/dev-guide/components-core/index.md (2)
45-46: Confirm that tagv1.10.0exists for LZ4At the time of writing, the latest official LZ4 release tag is
v1.9.4. Please double-check thatv1.10.0has actually been published (and that the SHA256 in the Taskfile matches it) to avoid broken downloads.
54-55: Good to see zstd listed – keep the table in sorted orderMinor nit: to keep the table alphabetically sorted, move the new zstd row just above yscope-log-viewer.
-| [yscope-log-viewer](https://github.com/y-scope/yscope-log-viewer.git) | 969ff35 | -| [ystdlib-cpp](https://github.com/y-scope/ystdlib-cpp.git) | d80cf86 | -| [zstd](https://github.com/facebook/zstd) | v1.5.7 | +| [zstd](https://github.com/facebook/zstd) | v1.5.7 | +| [yscope-log-viewer](https://github.com/y-scope/yscope-log-viewer.git) | 969ff35 | +| [ystdlib-cpp](https://github.com/y-scope/ystdlib-cpp.git) | d80cf86 |⛔ Skipped due to learnings
Learnt from: jackluo923 PR: y-scope/clp#1054 File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4 Timestamp: 2025-07-01T14:49:07.333Z Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.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.taskfiles/deps/utils.yaml (1)
70-72: Document or default CMAKE_SOURCE_DIR in the wrapper taskIn
taskfiles/deps/utils.yaml(Lines 70–72), the wrapper foryscope-dev-utils:cmake:install-remote-tarnow forwardsCMAKE_SOURCE_DIRbut neither documents it in the header nor provides a fallback. If a caller omits this variable, the downstream task will receive an empty value and may mis-configure the build.Please either:
• Add a sensible default (for example, the extracted source directory)
• Or enforce thatCMAKE_SOURCE_DIRis mandatory viash:assert/whenvalidationcomponents/core/src/clp_s/CMakeLists.txt (1)
105-106: Link-target swap looks good.All occurrences of the old
ZStd::ZStdtarget have been migrated to the newzstd_TARGETinterface library. This keeps the linkage consistent with the new top-level definition introduced incomponents/core/CMakeLists.txt, and no other changes are required in this file.Also applies to: 185-186, 253-254, 317-318, 345-346, 411-412
taskfiles/deps/main.yaml (1)
435-452: Minor: redundant duplicate flags for Zstandard.Both
ZSTD_BUILD_SHARED/ZSTD_BUILD_STATICand the genericBUILD_SHARED_LIBSflag control shared/static artefacts. Passing only the project-specific flags is sufficient and avoids ambiguity, but this is purely optional.⛔ Skipped due to learnings
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#1122 File: components/core/src/clp/clp/CMakeLists.txt:175-195 Timestamp: 2025-07-23T09:54:45.185Z Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.Learnt from: Bill-hbrhbr PR: y-scope/clp#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: 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: anlowee PR: y-scope/clp#925 File: taskfiles/deps/main.yaml:97-106 Timestamp: 2025-05-28T18:33:30.155Z Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.components/core/CMakeLists.txt (2)
195-202: Imported-target names may not exist.The interface library selects
LZ4::lz4_static/LZ4::lz4_shared, but the upstream 1.10.0 CMake exports targets namedlz4::lz4_static/lz4::lz4_shared(lower-case namespace). A typo in the namespace will break configuration time withTarget "LZ4::lz4_static" not foundPlease double-check the exported target names and adjust the case if necessary:
- target_link_libraries(lz4_TARGET INTERFACE LZ4::lz4_static) + target_link_libraries(lz4_TARGET INTERFACE lz4::lz4_static)(similar for the shared variant).
309-316: Same namespace issue for zstd?Upstream zstd exports
zstd::libzstd_static/zstd::libzstd_shared, which matches the code here.
Just ensure both variants are actually built (see the nitpick on the taskfile).
Contributor
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 (4)
components/core/CMakeLists.txt(3 hunks)components/core/src/clp/clg/CMakeLists.txt(1 hunks)components/core/src/clp/clo/CMakeLists.txt(1 hunks)docs/src/dev-guide/components-core/index.md(2 hunks)
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
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#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.414Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.414Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
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#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: 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: 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.
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
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:
docs/src/dev-guide/components-core/index.mdcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in the clp project, antlr code generation at build time is being removed by another pr. when reviewi...
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:
docs/src/dev-guide/components-core/index.mdcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in the clp codebase, standard headers like `` and `` in alphabetical order (as seen...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
docs/src/dev-guide/components-core/index.mdcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txt
📚 Learning: the clp project team has decided to refrain from using include directives in their documentation at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/dev-guide/components-core/index.md
📚 Learning: for the clp-core debian package creation script, strict version format validation is considered unne...
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Applied to files:
docs/src/dev-guide/components-core/index.md
📚 Learning: in clp's custom cmake find modules, `findstaticlibrarydependencies` populates the `*_dynamic_libs` v...
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:
docs/src/dev-guide/components-core/index.mdcomponents/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
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:
docs/src/dev-guide/components-core/index.md
📚 Learning: in the file `components/core/tests/test-ffi_keyvaluepairlogevent.cpp`, including `
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:
docs/src/dev-guide/components-core/index.md
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.
docs/src/dev-guide/components-core/index.md📚 Learning: in the clp project, manually setting ldflags for library paths can cause runtime errors because it i...
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/CMakeLists.txt
📚 Learning: the team plans to systematically improve include paths by adding `${project_source_dir}/src` to cmak...
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.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txt
📚 Learning: in clp's custom cmake find modules (like findbzip2.cmake), when building with dynamic libraries, the...
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/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in the clp project, c++20 is used, allowing the utilization of c++20 features such as class template...
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in clp's cmake configuration for bzip2, bill-hbrhbr prefers using hints parameter for path-based res...
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/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in lz4's cmake build system (build/cmake/cmakelists.txt), the option to build static libraries is `b...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.414Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Applied to files:
components/core/src/clp/clg/CMakeLists.txtcomponents/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: in 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floa...
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Applied to files:
components/core/src/clp/clo/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in cmake files....
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/CMakeLists.txt
📚 Learning: in the c++ `filedecompressor` implementations at `components/core/src/clp/clp/filedecompressor.cpp` ...
Learnt from: haiqi96
PR: y-scope/clp#619
File: components/core/src/clp/clp/decompression.cpp:313-313
Timestamp: 2024-12-05T16:32:21.507Z
Learning: In the C++ `FileDecompressor` implementations at `components/core/src/clp/clp/FileDecompressor.cpp` and `components/core/src/glt/glt/FileDecompressor.cpp`, the `temp_output_path` variable and associated logic are used to handle multiple compressed files with the same name, and should be kept. This logic is separate from the temporary output directory code removed in PR #619 and is necessary for proper file handling.
Applied to files:
components/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). (5)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build (macos-15)
🔇 Additional comments (4)
components/core/src/clp/clg/CMakeLists.txt (1)
153-154: Use of${zstd_TARGET}is correct and welcomeSwitching from the hard-coded
ZStd::ZStdto the variable${zstd_TARGET}keeps linkage consistent with the new top-level logic and honours the static/shared selection. No action required.components/core/src/clp/clo/CMakeLists.txt (1)
181-182: Consistent linkage with${zstd_TARGET}Same positive note as in
clg– the executable now respects the central static/shared selection for Zstandard.docs/src/dev-guide/components-core/index.md (1)
45-46: Verify that the pinned LZ4 v1.10.0 tag actually existsUpstream LZ4’s last published tag is v1.9.4. Please confirm that v1.10.0 is available (or will be shortly) in the official repository; otherwise the
deps:coretask will fail.Would you like a quick script that checks the remote Git tags before merging?
Also applies to: 54-55
components/core/CMakeLists.txt (1)
308-314:zstdblock looks goodThe package name and imported target names match upstream (
zstd::libzstd_*), and the static/shared toggle is handled correctly.
Contributor
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/CMakeLists.txt(2 hunks)components/core/src/clp_s/CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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#1156
File: components/core/CMakeLists.txt:195-202
Timestamp: 2025-08-06T08:10:26.381Z
Learning: According to Bill-hbrhbr, in LZ4's CMake configuration, the correct package name for find_package is lowercase "lz4", and the exported targets use uppercase namespace "LZ4::" with lowercase prefixes (LZ4::lz4_static, LZ4::lz4_shared), not lowercase namespace "lz4::" as initially assumed.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
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#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
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: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
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/src/clp_s/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_s/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_s/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_s/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_s/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-07-01T14:49:07.333Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2024-11-19T17:30:04.970Z
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/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.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-05-02T22:11:55.711Z
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.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-06T08:10:26.381Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:195-202
Timestamp: 2025-08-06T08:10:26.381Z
Learning: According to Bill-hbrhbr, in LZ4's CMake configuration, the correct package name for find_package is lowercase "lz4", and the exported targets use uppercase namespace "LZ4::" with lowercase prefixes (LZ4::lz4_static, LZ4::lz4_shared), not lowercase namespace "lz4::" as initially assumed.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2024-11-26T19:12:43.982Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Applied to files:
components/core/src/clp_s/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/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-08-06T07:32:28.462Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
components/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). (4)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (3)
components/core/CMakeLists.txt (2)
782-783: Good switch to${zstd_TARGET}in unit testsLinking the test binary against the centrally-selected zstd target keeps static
vs shared decisions consistent with the rest of the build.
300-307: Confirm zstd imported targets in CIUpstream zstd ≥ 1.5.x exports
zstd::libzstd_static/zstd::libzstd_shared, whereas older releases usedzstd::zstd*. Your mapping is correct, but if someone has an older zstd installed the configure step will fail. To catch this early, add a quick CI check, for example:cmake -P <<'CMAKE' cmake_minimum_required(VERSION 3.21) project(check_zstd LANGUAGES C) find_package(zstd 1.5.7 REQUIRED) get_property(staticTarget TARGET zstd::libzstd_static PROPERTY NAME) get_property(sharedTarget TARGET zstd::libzstd_shared PROPERTY NAME) message(STATUS "zstd static target: ${staticTarget}") message(STATUS "zstd shared target: ${sharedTarget}") CMAKEPlease run this locally or in your CI pipeline to verify that the expected targets exist for your minimum version.
components/core/src/clp_s/CMakeLists.txt (1)
183-186: Consistent linkage—looks goodThe IO target now defers to the central
${zstd_TARGET}decision, keeping
static/shared selection uniform across components.
…earch custom install paths
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/core/src/clp_s/CMakeLists.txt (1)
127-127: Fail fast if ${zstd_TARGET} is undefined in clp_s subdirThese libraries unconditionally use zstd sources; when CLP_NEED_ZSTD is OFF, ${zstd_TARGET} is undefined and linking fails with a cryptic error. Add an early guard in this CMakeLists (or in a common parent within this subdir).
+if(NOT DEFINED zstd_TARGET) + message(FATAL_ERROR "zstd is required for clp_s but CLP_NEED_ZSTD is OFF or zstd not found") +endif()Also applies to: 207-207
components/core/src/clp_s/indexer/CMakeLists.txt (1)
127-127: Add guard for ${zstd_TARGET} before linking indexerPrevent hard-to-debug “target not found” by failing early when zstd isn’t configured.
+if(NOT DEFINED zstd_TARGET) + message(FATAL_ERROR "zstd is required for indexer but CLP_NEED_ZSTD is OFF or zstd not found") +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/CMakeLists.txt(2 hunks)components/core/src/clp_s/CMakeLists.txt(2 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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#1156
File: components/core/CMakeLists.txt:195-202
Timestamp: 2025-08-06T08:10:26.381Z
Learning: According to Bill-hbrhbr, in LZ4's CMake configuration, the correct package name for find_package is lowercase "lz4", and the exported targets use uppercase namespace "LZ4::" with lowercase prefixes (LZ4::lz4_static, LZ4::lz4_shared), not lowercase namespace "lz4::" as initially assumed.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
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#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
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: haiqi96
PR: y-scope/clp#619
File: components/core/src/clp/clp/decompression.cpp:313-313
Timestamp: 2024-12-05T16:32:21.507Z
Learning: In the C++ `FileDecompressor` implementations at `components/core/src/clp/clp/FileDecompressor.cpp` and `components/core/src/glt/glt/FileDecompressor.cpp`, the `temp_output_path` variable and associated logic are used to handle multiple compressed files with the same name, and should be kept. This logic is separate from the temporary output directory code removed in PR #619 and is necessary for proper file handling.
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/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#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.
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/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_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/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_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-05-02T22:11:55.711Z
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.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-01T14:49:07.333Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2024-11-19T17:30:04.970Z
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/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_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/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_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/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.txtcomponents/core/src/clp_s/indexer/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-08-06T08:10:26.381Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:195-202
Timestamp: 2025-08-06T08:10:26.381Z
Learning: According to Bill-hbrhbr, in LZ4's CMake configuration, the correct package name for find_package is lowercase "lz4", and the exported targets use uppercase namespace "LZ4::" with lowercase prefixes (LZ4::lz4_static, LZ4::lz4_shared), not lowercase namespace "lz4::" as initially assumed.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/indexer/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/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
components/core/src/clp_s/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_s/CMakeLists.txt
📚 Learning: 2024-11-26T19:12:43.982Z
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Applied to files:
components/core/src/clp_s/CMakeLists.txtcomponents/core/CMakeLists.txt
📚 Learning: 2025-08-06T07:32:28.462Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Applied to files:
components/core/src/clp_s/indexer/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: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
301-307: Zstd references fully updatedThe
rg -n '\bZStd::ZStd\b'check returned no matches, confirming there are no lingering hardcodedZStd::ZStdreferences. No further action required on the zstd package section.
Bill-hbrhbr
changed the title
kirkrodrigues
requested changes
Aug 9, 2025
Co-authored-by: kirkrodrigues <[email protected]>
kirkrodrigues
approved these changes
Aug 10, 2025
Member
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
build(deps): Add task-based installations for LZ4 and Zstandard dependencies; Use latest versions for source-based installations.
This was referenced Aug 18, 2025
Merged
4 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
LZ4 and Zstandard are required LibArchive dependencies for CLP to ingest archives of these formats, both of which are actively used in production.
This PR introduces Taskfile-based installation for both libraries and updates them to their latest stable releases, improving compression I/O performance. It also prepares the build environment for upcoming LibArchive changes in #1122.
The existing LZ4 and Zstandard installations are retained for now, since the current LibArchive installation process does not allow specifying custom search paths for these libraries. They will be removed once #1122 is merged.
Checklist
breaking change.
Validation performed
lz4andzstdcmake targets are successfully exported and linked with variousCLPtargets.Summary by CodeRabbit
Summary by CodeRabbit