build(deps): Add task-based installation and cmake find module script for LibLZMA dependency. #1184
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
WalkthroughReplaces manual LibLZMA discovery with a Find module and find_package-based integration, renames LZMA-related options/variables, exposes an imported target Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CMake as CMake Configure
participant Finder as FindLibLZMA.cmake
participant PKG as pkg-config/System
participant Target as LibLZMA::LibLZMA
Dev->>CMake: cmake -S . -B build
CMake->>Finder: find_package(LibLZMA 5.8.1 REQUIRED MODULE)
Finder->>PKG: probe pkg-config / search include & lib (honour LibLZMA_ROOT, LibLZMA_USE_STATIC_LIBS)
PKG-->>Finder: return paths & version
Finder-->>CMake: set LibLZMA_FOUND, LibLZMA_VERSION and create LibLZMA::LibLZMA
CMake->>Target: link unit tests and targets against LibLZMA::LibLZMA
sequenceDiagram
participant CI as CI / Developer
participant Tasks as core-all-parallel
participant LZTask as liblzma
participant Installer as utils:install-remote-cmake-lib
participant Remote as xz v5.8.1 tarball
CI->>Tasks: run core-all-parallel
Tasks->>LZTask: run liblzma
LZTask->>Installer: invoke installer (shared & static variants)
Installer->>Remote: download & verify (SHA256)
Remote-->>Installer: build & install libs
Installer-->>LZTask: report success
LZTask-->>Tasks: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/Modules/FindLibLZMA.cmake(1 hunks)components/core/cmake/Options/options.cmake(2 hunks)taskfiles/deps/main.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.055Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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: 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: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.409Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR #1152 discussion.
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: 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: 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: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt: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/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-09T04:07:27.055Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.055Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmaketaskfiles/deps/main.yaml
📚 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/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmaketaskfiles/deps/main.yaml
📚 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.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmaketaskfiles/deps/main.yaml
📚 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/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/cmake/Options/options.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
components/core/CMakeLists.txtcomponents/core/cmake/Modules/FindLibLZMA.cmake
📚 Learning: 2024-10-14T03:42:10.355Z
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Applied to files:
components/core/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/CMakeLists.txtcomponents/core/cmake/Options/options.cmake
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/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/CMakeLists.txt
📚 Learning: 2025-02-12T22:24:17.723Z
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:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-06-18T20:48:48.990Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Applied to files:
components/core/cmake/Options/options.cmake
📚 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/cmake/Options/options.cmake
📚 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/cmake/Options/options.cmake
📚 Learning: 2025-05-28T18:33:30.155Z
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfiles/deps/main.yaml
📚 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:
taskfiles/deps/main.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (5)
taskfiles/deps/main.yaml (1)
69-69: Including liblzma in core-all-parallel – LGTMThis ensures LibLZMA is installed alongside other core deps. No concerns.
components/core/cmake/Modules/FindLibLZMA.cmake (1)
28-33: Require PkgConfig or provide a guarded fallback, and add lib64 search pathsThis module unconditionally relies on pkg-config to set
LibLZMA_VERSION. Since we don’t mark PkgConfig as REQUIRED anywhere else, addingfind_package(PkgConfig REQUIRED)here is a breaking change if any supported build environment lacks pkg-config. Please confirm that all CI and developer machines ship pkg-config; otherwise wrap thepkg_check_modulescall in anif(PkgConfig_FOUND)guard and fall back to parsing the<lzma.h>header forLibLZMA_VERSION.Additionally, broaden the include/library search to honour
${LibLZMA_ROOT}and 64-bit layouts:• File: components/core/cmake/Modules/FindLibLZMA.cmake
Lines ~28–33, 56–58-find_package(PkgConfig) +find_package(PkgConfig REQUIRED) # or guard fall-back if pkg-config is optional pkg_check_modules(liblzma_PKGCONF QUIET "${liblzma_PKGCONFIG_NAME}") # Set include directory find_path(LibLZMA_INCLUDE_DIR ${liblzma_HEADER} - HINTS ${liblzma_PKGCONF_INCLUDEDIR} + HINTS ${liblzma_PKGCONF_INCLUDEDIR} ${LibLZMA_ROOT} + PATH_SUFFIXES include ) # Set library find_library(LibLZMA_LIBRARY - NAMES "${liblzma_LIBNAME}" - HINTS ${liblzma_PKGCONF_LIBDIR} + NAMES "${liblzma_LIBNAME}" "liblzma" + HINTS ${liblzma_PKGCONF_LIBDIR} ${LibLZMA_ROOT} PATH_SUFFIXES lib lib64 )Please verify pkg-config availability across all supported environments and adjust accordingly.
components/core/CMakeLists.txt (2)
743-743: Switching to imported target LibLZMA::LibLZMA – LGTMThis aligns test linking with the new find module and removes manual include/library handling.
313-320: Ensure liblzma dependency task builds static librariesThe
FindLibLZMA.cmakemodule correctly switches to static‐only lookup whenCLP_USE_STATIC_LIBS=ON, but theliblzmatask intaskfiles/deps/main.yamlonly passes
-DBUILD_SHARED_LIBS=ON. On static builds, no.alibrary will be produced andfind_package(LibLZMA)will fail.Please update the
liblzmatask as follows:
- File:
taskfiles/deps/main.yaml(around theliblzma:section)- In
cmds[0].vars.CMAKE_GEN_ARGS, add-DBUILD_STATIC_LIBS=ONalongside-DBUILD_SHARED_LIBS=ONThis will ensure both shared and static XZ/liblzma libraries are installed and prevent static‐link build failures.
⛔ Skipped due to 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/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#1156 File: components/core/CMakeLists.txt:772-772 Timestamp: 2025-08-09T04:07:27.055Z Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.Learnt from: Bill-hbrhbr PR: y-scope/clp#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/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: 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: 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.components/core/cmake/Options/options.cmake (1)
174-175: Consistent flag rename to CLP_NEED_LIBLZMA – LGTMThe rename is consistent with the new find module and CMakeLists usage. No functional issues.
Also applies to: 460-462
Bill-hbrhbr
commented
Aug 13, 2025
jackluo923
approved these changes
Aug 14, 2025
jackluo923
previously approved these changes
Aug 14, 2025
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
235-235: Nit: naming preference (non-blocking)A prior reviewer preferred “lzma” over “LibLZMA” here. Not important, but consider for consistency with other LIB_NAME values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/core/CMakeLists.txt(2 hunks)components/core/cmake/Options/options.cmake(2 hunks)taskfiles/deps/main.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
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#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/cmake/Options/options.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/cmake/Options/options.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-08-13T19:58:26.033Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/cmake/Options/options.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/cmake/Options/options.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/cmake/Options/options.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 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/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/CMakeLists.txttaskfiles/deps/main.yaml
📚 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:
taskfiles/deps/main.yaml
📚 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:
taskfiles/deps/main.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
taskfiles/deps/main.yaml
📚 Learning: 2025-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:
taskfiles/deps/main.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
taskfiles/deps/main.yaml (2)
69-69: Wiring LibLZMA into core-all-parallel looks goodThis ensures the dependency is installed and settings are generated as part of the standard flow.
225-239: LibLZMA install task parameters are appropriate and align with the find-module design
- xz 5.8.1 tarball and checksum provided.
- Building shared-only is consistent with the custom FindLibLZMA module’s static-first-then-fallback-to-shared behaviour, so no need to build both variants here.
components/core/cmake/Options/options.cmake (2)
166-176: Adding CLP_NEED_LIBLZMA to test dependencies is correctThis cleanly gates LibLZMA discovery to test builds that need it.
459-461: No lingering CLP_NEED_LZMA references — only CLP_NEED_LIBLZMA presentRipgrep search returned no matches for CLP_NEED_LZMA. Instances found for CLP_NEED_LIBLZMA at:
- components/core/CMakeLists.txt:311 — if(CLP_NEED_LIBLZMA)
- components/core/cmake/Options/options.cmake:174 — CLP_NEED_LIBLZMA
- components/core/cmake/Options/options.cmake:459 — CLP_NEED_LIBLZMA
No action required.
components/core/CMakeLists.txt (2)
311-318: Module-based LibLZMA discovery with version gating is solid
- Respects CLP_USE_STATIC_LIBS via LibLZMA_USE_STATIC_LIBS, while the find module can fall back to shared if static isn’t present.
- Using REQUIRED MODULE with a minimum of 5.8.1 is clear and justifiable given security fixes.
744-744: Linking unitTest against LibLZMA imported target is correctThis aligns with the new FindLibLZMA module and ensures include/link settings propagate properly.
Bill-hbrhbr
commented
Aug 17, 2025
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/core/CMakeLists.txt(3 hunks)components/core/cmake/Modules/FindLibLZMA.cmake(1 hunks)taskfiles/deps/main.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
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#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
📚 Learning: 2025-08-13T19:58:26.033Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 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/cmake/Modules/FindLibLZMA.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmakecomponents/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 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/CMakeLists.txttaskfiles/deps/main.yaml
📚 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/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:
taskfiles/deps/main.yaml
📚 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:
taskfiles/deps/main.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
taskfiles/deps/main.yaml
📚 Learning: 2025-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:
taskfiles/deps/main.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (5)
taskfiles/deps/main.yaml (2)
69-69: LGTM: Wiring LibLZMA into core-all-parallelThe dependency is correctly added so the new tasks participate in the standard deps pipeline.
232-254: Sanity check: xz/liblzma options look correct for tool-free buildsFlags disable CLI/tools and docs, and split static/shared via BUILD_SHARED_LIBS as required by xz’s CMake. SHA256 and URL match v5.8.1. Looks good.
Also applies to: 255-277
components/core/CMakeLists.txt (3)
126-128: LGTM: Centralising deps settings path via CLP_CORE_DEPS_DIRThis aligns with other deps and keeps include logic consistent across platforms.
312-318: Ensure FindLibLZMA actually consumes LibLZMA_ROOTYou correctly set LibLZMA_ROOT (static/shared) before find_package. However, the current FindLibLZMA.cmake ignores LibLZMA_ROOT, risking selection of a system lib. Please incorporate LibLZMA_ROOT in the module’s HINTS and pkg-config path handling (see my module comment).
748-749: LGTM: Linking via LibLZMA::LibLZMA imported targetTarget-based linking is correct and consistent with the new Find module’s public API.
jackluo923
requested changes
Aug 18, 2025
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/core/cmake/Modules/FindLibLZMA.cmake (1)
92-96: Restore PKG_CONFIG_PATH only if it was actually modifiedTie restoration to the presence of the saved variable to avoid unintended environment clobbering.
-# Restore original value of PKG_CONFIG_PATH -if(DEFINED CLP_CORE_DEPS_DIR) - set(ENV{PKG_CONFIG_PATH} "$ENV{liblzma_ORIG_PKG_CONFIG_PATH}") - unset(ENV{liblzma_ORIG_PKG_CONFIG_PATH}) -endif() +# Restore original value of PKG_CONFIG_PATH only if we saved it +if(DEFINED ENV{liblzma_ORIG_PKG_CONFIG_PATH}) + set(ENV{PKG_CONFIG_PATH} "$ENV{liblzma_ORIG_PKG_CONFIG_PATH}") + unset(ENV{liblzma_ORIG_PKG_CONFIG_PATH}) +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/core/cmake/Modules/FindLibLZMA.cmake(1 hunks)taskfiles/deps/main.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
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#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#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: 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#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.840Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
📚 Learning: 2025-08-13T19:58:26.033Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 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/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-18T05:43:07.840Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.840Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
components/core/cmake/Modules/FindLibLZMA.cmaketaskfiles/deps/main.yaml
📚 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:
taskfiles/deps/main.yaml
📚 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:
taskfiles/deps/main.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
🔇 Additional comments (4)
components/core/cmake/Modules/FindLibLZMA.cmake (1)
49-53: Bug fix acknowledged: correct stash/restore variable namesGood catch on restoring CMAKE_FIND_LIBRARY_SUFFIXES with the liblzma-prefixed variable; the prior bzip2-prefixed typo would have left the global list mutated.
taskfiles/deps/main.yaml (3)
69-69: Wire LibLZMA into core-all-parallelAdding liblzma to the parallel deps is correct and keeps the pipeline consistent with other core deps.
225-241: Good split of common CMake args and explicit XZ feature disablesThis keeps the install lean and reproducible across both variants.
242-257: Build both variants via a single orchestrator is clear and reproducibleInstalling shared then static via the shared liblzma-install task is a clean approach given XZ’s one-variant-per-configure model.
Comment on lines
+21
to
+24
| if(DEFINED CLP_CORE_DEPS_DIR) | ||
| set(ENV{liblzma_ORIG_PKG_CONFIG_PATH} "$ENV{PKG_CONFIG_PATH}") | ||
| set(ENV{PKG_CONFIG_PATH} "${CLP_CORE_DEPS_DIR}:$ENV{PKG_CONFIG_PATH}") | ||
| endif() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use platform-appropriate PKG_CONFIG_PATH separator and handle empty PATH robustly
On Windows, PKG_CONFIG_PATH must use “;” instead of “:”. Also guard for an initially empty PKG_CONFIG_PATH to avoid leading/trailing separators.
Apply:
-if(DEFINED CLP_CORE_DEPS_DIR)
- set(ENV{liblzma_ORIG_PKG_CONFIG_PATH} "$ENV{PKG_CONFIG_PATH}")
- set(ENV{PKG_CONFIG_PATH} "${CLP_CORE_DEPS_DIR}:$ENV{PKG_CONFIG_PATH}")
-endif()
+if(DEFINED CLP_CORE_DEPS_DIR)
+ set(ENV{liblzma_ORIG_PKG_CONFIG_PATH} "$ENV{PKG_CONFIG_PATH}")
+ if(WIN32)
+ set(_liblzma_pkgconf_sep ";")
+ else()
+ set(_liblzma_pkgconf_sep ":")
+ endif()
+ if("$ENV{PKG_CONFIG_PATH}" STREQUAL "")
+ set(ENV{PKG_CONFIG_PATH} "${CLP_CORE_DEPS_DIR}")
+ else()
+ set(ENV{PKG_CONFIG_PATH} "${CLP_CORE_DEPS_DIR}${_liblzma_pkgconf_sep}$ENV{PKG_CONFIG_PATH}")
+ endif()
+ unset(_liblzma_pkgconf_sep)
+endif()🤖 Prompt for AI Agents
In components/core/cmake/Modules/FindLibLZMA.cmake around lines 21 to 24, the
code unconditionally concatenates CLP_CORE_DEPS_DIR into PKG_CONFIG_PATH with
":" which breaks on Windows and can produce stray separators when
PKG_CONFIG_PATH is empty; update it to choose the separator based on the
platform (use ";" on Windows (WIN32) and ":" otherwise), preserve the original
PKG_CONFIG_PATH in an env var as before, and build the new PKG_CONFIG_PATH by
checking if the existing PKG_CONFIG_PATH is empty—if empty, set it to
CLP_CORE_DEPS_DIR only, otherwise join CLP_CORE_DEPS_DIR and the existing value
with the selected separator—so no leading/trailing separators appear.
Comment on lines
+30
to
+35
| # Set include directory | ||
| find_path(LibLZMA_INCLUDE_DIR ${liblzma_HEADER} | ||
| HINTS ${liblzma_PKGCONF_INCLUDEDIR} | ||
| PATH_SUFFIXES include | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add CLP_CORE_DEPS_DIR and lib64 to HINTS/PATH_SUFFIXES to reduce reliance on pkg-config
If pkg-config doesn’t resolve (or resolves to the “other” variant), discovery may fail. Adding CLP_CORE_DEPS_DIR to HINTS and lib64 to PATH_SUFFIXES makes the module resilient and deterministic within your deps prefix.
find_path(LibLZMA_INCLUDE_DIR ${liblzma_HEADER}
- HINTS ${liblzma_PKGCONF_INCLUDEDIR}
+ HINTS ${liblzma_PKGCONF_INCLUDEDIR} ${CLP_CORE_DEPS_DIR}
PATH_SUFFIXES include
)
@@
find_library(LibLZMA_LIBRARY
NAMES "${liblzma_LIBNAME}"
- HINTS ${liblzma_PKGCONF_LIBDIR}
- PATH_SUFFIXES lib
+ HINTS ${liblzma_PKGCONF_LIBDIR} ${CLP_CORE_DEPS_DIR}
+ PATH_SUFFIXES lib lib64
)Also applies to: 43-47
🤖 Prompt for AI Agents
In components/core/cmake/Modules/FindLibLZMA.cmake around lines 30-35 (and
similarly lines 43-47), the find_path/find_library calls only use pkg-config
hints and "include" suffixes which can fail for non-pkg-config installs; add
CLP_CORE_DEPS_DIR to the HINTS list and add "lib64" to PATH_SUFFIXES so the
search will also look under the deps prefix and 64-bit lib directories; update
both the include and library search blocks to include these new HINTS and
PATH_SUFFIXES entries to make discovery deterministic.
Comment on lines
+36
to
+41
| # Handle static libraries | ||
| if(LibLZMA_USE_STATIC_LIBS) | ||
| set(liblzma_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) | ||
| set(CMAKE_FIND_LIBRARY_SUFFIXES "${CMAKE_STATIC_LIBRARY_SUFFIX}") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer static when requested but gracefully fall back to shared
Current logic restricts suffixes to static (when LibLZMA_USE_STATIC_LIBS is ON), which will hard-fail if the static variant isn’t installed or the resolved libdir points at the shared install. Align with the project’s pattern: prefer static, then fall back to shared.
Option A (single-pass preference via suffix ordering):
if(LibLZMA_USE_STATIC_LIBS)
- set(liblzma_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
- set(CMAKE_FIND_LIBRARY_SUFFIXES "${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(liblzma_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
+ # Prefer static by putting it first, but keep originals for fallback.
+ set(CMAKE_FIND_LIBRARY_SUFFIXES "${CMAKE_STATIC_LIBRARY_SUFFIX};${CMAKE_FIND_LIBRARY_SUFFIXES}")
endif()
@@
find_library(LibLZMA_LIBRARY
NAMES "${liblzma_LIBNAME}"
HINTS ${liblzma_PKGCONF_LIBDIR} ${CLP_CORE_DEPS_DIR}
- PATH_SUFFIXES lib
+ PATH_SUFFIXES lib lib64
)
if(LibLZMA_USE_STATIC_LIBS)
# Restore original value of CMAKE_FIND_LIBRARY_SUFFIXES
set(CMAKE_FIND_LIBRARY_SUFFIXES ${liblzma_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES})
unset(liblzma_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES)
endif()Option B (two-pass fallback if you want strict separation):
- First try with static-only suffix list.
- If not found, restore suffixes and try again without static preference, emitting a STATUS message.
Ask if you want me to draft this variant.
Also applies to: 43-53
🤖 Prompt for AI Agents
In components/core/cmake/Modules/FindLibLZMA.cmake around lines 36-41 (and
similar logic at 43-53): currently you replace CMAKE_FIND_LIBRARY_SUFFIXES with
the static suffix only, which hard-fails if static lib is not present; implement
a two-pass fallback instead: save the original CMAKE_FIND_LIBRARY_SUFFIXES, set
it to the static-only suffix and attempt the library find, and if that fails
restore the original suffixes, emit a STATUS message about falling back to
shared, and attempt the find again; in all cases ensure the original suffix list
is restored before returning.
jackluo923
approved these changes
Aug 18, 2025
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/core/CMakeLists.txt (1)
126-131: Make CLP_CORE_DEPS_DIR user-overridable (cache PATH).Exposing CLP_CORE_DEPS_DIR as a CACHE PATH improves discoverability and allows local overrides without editing the file.
Apply this diff:
- set(CLP_CORE_DEPS_DIR "${CMAKE_SOURCE_DIR}/../../build/deps/core") + set(CLP_CORE_DEPS_DIR "${CMAKE_SOURCE_DIR}/../../build/deps/core" CACHE PATH "Directory containing CLP core dependencies (generated by task-based deps)")
♻️ Duplicate comments (1)
components/core/CMakeLists.txt (1)
319-321: Correct the inaccurate CVE reference in the comment.xz/liblzma 5.8.1 does not “address CVE-2024-3094.” Simplify or mention CVE-2025-31115 only to avoid misinformation.
Apply this diff:
- # Version 5.8.1 and above address CVE-2024-3094 and CVE-2025-31115. + # Require xz/liblzma >= 5.8.1 (fixes CVE-2025-31115).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/core/CMakeLists.txt(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/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#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.
📚 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/CMakeLists.txt
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.033Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/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/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/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.840Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.840Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/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/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/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/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: centos-stream-9-static-linked-bins
- 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: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
components/core/CMakeLists.txt (2)
320-321: LGTM: find_package invocation and discovery message.Version gating and REQUIRED are appropriate; combined with CMAKE_MODULE_PATH this will use the project’s FindLibLZMA.cmake as intended.
748-749: Unconditional link against LibLZMA::LibLZMA could fail if CLP_NEED_LIBLZMA is OFF.If a build configuration disables LibLZMA discovery, this target may be undefined and cause link-time configuration errors. Given prior decisions to defer conditional linking improvements to separate PRs, no change requested here—just flagging for awareness.
Recommended checks:
- Ensure CI runs a job with CLP_NEED_LIBLZMA=ON and one with it=OFF (or with GENERAL_COMPRESSOR != lzma) to confirm this target is always defined where needed.
Comment on lines
+312
to
318
| if(CLP_NEED_LIBLZMA) | ||
| 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}") | ||
|
|
||
| # Version 5.8.1 and above address CVE-2024-3094 and CVE-2025-31115. | ||
| set(REQUIRED_LIBLZMA_VERSION "5.8.1") | ||
| if(LIBLZMA_VERSION_STRING VERSION_LESS ${REQUIRED_LIBLZMA_VERSION}) | ||
| message( | ||
| FATAL_ERROR | ||
| "Detected LibLZMA version ${LIBLZMA_VERSION_STRING} is older than required" | ||
| " ${REQUIRED_LIBLZMA_VERSION}" | ||
| ) | ||
| endif() | ||
| set(LibLZMA_ROOT ${LibLZMA-static_ROOT}) | ||
| set(LibLZMA_USE_STATIC_LIBS ON) | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Lzma") | ||
| set(LibLZMA_ROOT ${LibLZMA-shared_ROOT}) | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid per-variant LibLZMA_ROOT; rely on CLP_CORE_DEPS_DIR and toggle only LibLZMA_USE_STATIC_LIBS.
Per retrieved learnings, our Find modules already search within CLP_CORE_DEPS_DIR and handle static/shared selection via LibLZMA_USE_STATIC_LIBS. The LibLZMA-static_ROOT / LibLZMA-shared_ROOT overrides are unnecessary and add maintenance burden.
Apply this diff:
if(CLP_NEED_LIBLZMA)
if(CLP_USE_STATIC_LIBS)
- set(LibLZMA_ROOT ${LibLZMA-static_ROOT})
set(LibLZMA_USE_STATIC_LIBS ON)
else()
- set(LibLZMA_ROOT ${LibLZMA-shared_ROOT})
+ # Be explicit to avoid stale cache values from toolchain/user presets.
+ set(LibLZMA_USE_STATIC_LIBS OFF)
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if(CLP_NEED_LIBLZMA) | |
| 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}") | |
| # Version 5.8.1 and above address CVE-2024-3094 and CVE-2025-31115. | |
| set(REQUIRED_LIBLZMA_VERSION "5.8.1") | |
| if(LIBLZMA_VERSION_STRING VERSION_LESS ${REQUIRED_LIBLZMA_VERSION}) | |
| message( | |
| FATAL_ERROR | |
| "Detected LibLZMA version ${LIBLZMA_VERSION_STRING} is older than required" | |
| " ${REQUIRED_LIBLZMA_VERSION}" | |
| ) | |
| endif() | |
| set(LibLZMA_ROOT ${LibLZMA-static_ROOT}) | |
| set(LibLZMA_USE_STATIC_LIBS ON) | |
| else() | |
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for Lzma") | |
| set(LibLZMA_ROOT ${LibLZMA-shared_ROOT}) | |
| endif() | |
| if(CLP_NEED_LIBLZMA) | |
| if(CLP_USE_STATIC_LIBS) | |
| set(LibLZMA_USE_STATIC_LIBS ON) | |
| else() | |
| # Be explicit to avoid stale cache values from toolchain/user presets. | |
| set(LibLZMA_USE_STATIC_LIBS OFF) | |
| endif() |
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt around lines 312 to 318, remove the
per-variant assignments to LibLZMA_ROOT (LibLZMA-static_ROOT /
LibLZMA-shared_ROOT) and instead only toggle LibLZMA_USE_STATIC_LIBS when
CLP_USE_STATIC_LIBS is true; rely on the existing CLP_CORE_DEPS_DIR-aware Find
modules to locate the library. Concretely, eliminate the set(LibLZMA_ROOT ...)
calls and keep or add only set(LibLZMA_USE_STATIC_LIBS ON) under the
CLP_USE_STATIC_LIBS branch so the Find module picks static vs shared
automatically. Ensure no other code expects LibLZMA_ROOT to be set; if it does,
remove or adapt those expectations to use the Find module behavior.
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
This PR prepares the build environment for upcoming LibArchive changes in #1122.
Since
liblzmadoesn't support building static and shared libraries simultaneously, we install both versions and make the find_package() path selection during clp cmake generate phase based on the option value ofCLP_USE_STATIC_LIBS.The existing LibLZMA 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
Summary by CodeRabbit
Refactor
Chores