build(deps): Bump ystdlib to y-scope/ystdlib-cpp@9ed78cd and install ystdlib via CMake; Lower minimum Clang version for Velox compatibility.
#1226
Conversation
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CI as CI
participant CMake as CMake (configure)
participant Utils as Toolchains/utils.cmake
participant Deps as taskfiles/deps
Dev->>CI: push / workflow run
CI->>CI: macOS matrix includes macos-14 & macos-15
Dev->>CMake: invoke configure
CMake->>CMake: project()
CMake->>Utils: include(utils.cmake)
Utils-->>CMake: provide validate_compiler_versions()
CMake->>Utils: validate_compiler_versions()
Utils-->>CMake: ok / fatal (unsupported or too-old)
CMake->>CMake: setup dependency flags & convert properties
CMake->>Deps: require ystdlib (find_package / taskfile-installed)
Deps-->>CMake: ystdlib available (installed via utils:install-remote-cmake-lib)
CMake-->>Dev: configure complete
Note over Utils,CMake: Validation replaces prior macOS Homebrew LLVM auto-setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/core/cmake/Toolchains/utils.cmake (1)
19-25: Fix multiline error message formatting (stray backslash will appear in output)The backslash in the quoted string is literal in CMake and will show up in the error. Simplify the string or split into adjacent literals.
Apply:
- message( - FATAL_ERROR - "${CMAKE_CXX_COMPILER_ID} version ${CMAKE_CXX_COMPILER_VERSION} is too low. Must be at \ - least ${CXX_COMPILER_MIN_VERSION}." - ) + message( + FATAL_ERROR + "${CMAKE_CXX_COMPILER_ID} version ${CMAKE_CXX_COMPILER_VERSION} is too low. " + "Must be at least ${CXX_COMPILER_MIN_VERSION}." + )
♻️ Duplicate comments (1)
.github/workflows/clp-core-build-macos.yaml (1)
48-50: Matrix expansion to macos-13 and macos-14 adopted — good callThis aligns with prior feedback and broadens coverage without disrupting the job flow.
📜 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 (6)
.github/workflows/clp-core-build-macos.yaml(1 hunks)components/core/CMakeLists.txt(1 hunks)components/core/cmake/Toolchains/llvm-clang-16-toolchain.cmake(0 hunks)components/core/cmake/Toolchains/utils.cmake(1 hunks)docs/src/dev-guide/components-core/index.md(1 hunks)taskfiles/deps/main.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- components/core/cmake/Toolchains/llvm-clang-16-toolchain.cmake
🧰 Additional context used
🧠 Learnings (5)
📚 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.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-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-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: 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.txt
🔇 Additional comments (4)
docs/src/dev-guide/components-core/index.md (1)
53-53: Docs synced with dependency bump — looks goodystdlib-cpp commit updated to 9ed78cd, matching the Taskfile change. No further action needed.
.github/workflows/clp-core-build-macos.yaml (1)
48-50: Confirm AppleClang versions on macos-13/14 to satisfy the new >=15 minimumGiven validate_compiler_versions() now requires AppleClang/Clang >= 15, please verify the runner images provide that (particularly macos-13, which can be Xcode 14 on some images).
To make this visible in CI, consider inserting a small diagnostic step after checkout:
- uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" with: # Fetch history so that the `clang-tidy-diff` task can compare against the main branch. fetch-depth: 0 submodules: "recursive" + - name: "Print compiler info" + shell: "bash" + run: | + echo "C++ compiler:" + c++ --version || true + echo "Clang:" + clang --version || true + echo "CC/CXX env:" + echo "CC=${CC:-unset}"; echo "CXX=${CXX:-unset}"taskfiles/deps/main.yaml (1)
476-479: Checksum verified — ready to merge
The computed SHA256 matches the expected value for the tarball at commit 9ed78cd. No further action required.components/core/CMakeLists.txt (1)
6-10: Toolchain validation inclusion and call are correctly positionedIncluding Toolchains/utils.cmake post-project() and invoking validate_compiler_versions() early in the configuration phase is appropriate and keeps concerns separated from toolchain setup logic that was removed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/core/CMakeLists.txt(1 hunks)docs/src/dev-guide/components-core/index.md(1 hunks)taskfiles/deps/main.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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.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-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-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: 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.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: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (3)
components/core/CMakeLists.txt (1)
8-10: Validate lowered Clang/AppleClang minimums in utils.cmake and CI matrixWe’ve confirmed that validate_compiler_versions() (now in components/core/cmake/Toolchains/utils.cmake) sets CXX_COMPILER_MIN_VERSION to “15” for both AppleClang and Clang, and errors out on anything older. Please manually verify that your CI workflows in .github/workflows/*.yml include matrix entries for Clang and AppleClang with version 15 or higher to match these new thresholds.
- components/core/cmake/Toolchains/utils.cmake: validate_compiler_versions() → Clang/AppleClang ≥ 15
- .github/workflows/*.yml: matrix.platform/version entries for Clang/AppleClang ≥ 15
docs/src/dev-guide/components-core/index.md (1)
53-53: ystdlib-cpp commit pin updated — looks consistent with deps configThe doc now references 9ed78cd, which matches the updated tarball in Taskfile. Good alignment.
taskfiles/deps/main.yaml (1)
478-481: Action Required: Verify ystdlib-cpp checksum and docs commit locallyThe SSL certificate verification failed when downloading the tarball in this sandbox environment. Please run the verification on your local machine (with a valid CA store) or adjust the script to bypass SSL checks. Then ensure:
- The FILE_SHA256 in taskfiles/deps/main.yaml
matches the actual SHA-256 of https://github.com/y-scope/ystdlib-cpp/archive/9ed78cd.tar.gz.- The docs entry in docs/src/dev-guide/components-core/index.md
lists commit9ed78cd(or the corresponding full 40-char SHA) for ystdlib-cpp.You can modify the Python snippet to disable SSL verification, for example:
import ssl, urllib.request ctx = ssl._create_unverified_context() with urllib.request.urlopen(url, context=ctx) as resp: # …Or use a local
curl -L … | sha256sumto compute the hash.
Bill-hbrhbr
changed the title
ystdlib-cpp to the newest version; Lower minimum Clang version for Velox compatibility.ystdlib to the newest version; Lower minimum Clang version for Velox compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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)
.github/workflows/clp-lint.yaml(1 hunks)
.github/workflows/clp-lint.yaml
Outdated
| - if: "matrix.os == 'ubuntu24.04'" | ||
| name: "Install pipx" | ||
| shell: "bash" | ||
| run: "DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y pipx" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broken OS condition prevents pipx install on Ubuntu runners
The matrix uses "ubuntu-24.04", but the condition checks for "ubuntu24.04" (missing dash), so this step never runs. Also, installing with apt on GitHub-hosted runners typically requires sudo and an apt-get update first.
Apply this patch to fix the condition and ensure a reliable install:
- - if: "matrix.os == 'ubuntu24.04'"
- name: "Install pipx"
- shell: "bash"
- run: "DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y pipx"
+ - if: startsWith(matrix.os, 'ubuntu-')
+ name: "Install pipx (Ubuntu)"
+ shell: bash
+ run: |
+ sudo apt-get update
+ DEBIAN_FRONTEND=noninteractive sudo apt-get install --no-install-recommends -y pipx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - if: "matrix.os == 'ubuntu24.04'" | |
| name: "Install pipx" | |
| shell: "bash" | |
| run: "DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y pipx" | |
| - if: startsWith(matrix.os, 'ubuntu-') | |
| name: "Install pipx (Ubuntu)" | |
| shell: bash | |
| run: | | |
| sudo apt-get update | |
| DEBIAN_FRONTEND=noninteractive sudo apt-get install --no-install-recommends -y pipx |
🤖 Prompt for AI Agents
.github/workflows/clp-lint.yaml around lines 34 to 38: the job condition
incorrectly matches "ubuntu24.04" (missing dash) so the pipx install step never
runs; update the if expression to check matrix.os == 'ubuntu-24.04' and make the
install robust by running apt-get update and using sudo for apt-get install
(e.g., run: sudo apt-get update && sudo DEBIAN_FRONTEND=noninteractive apt-get
install --no-install-recommends -y pipx), ensuring the step executes on the
intended Ubuntu runner and succeeds on GitHub-hosted images.
.github/workflows/clp-lint.yaml
Outdated
| - name: "Install cmake" | ||
| shell: "bash" | ||
| run: "pipx install cmake~=3.31" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unconditional cmake install via pipx will fail on macOS (pipx not present)
This step assumes pipx exists on all runners. Given the previous step is Ubuntu-only, macOS lacks pipx and the cmake install will fail.
Option A (OS-specific installs; minimal diff with current approach):
- - name: "Install cmake"
- shell: "bash"
- run: "pipx install cmake~=3.31"
+ - if: startsWith(matrix.os, 'macos-')
+ name: "Install pipx (macOS)"
+ shell: bash
+ run: brew install pipx
+
+ - name: "Install CMake via pipx"
+ shell: bash
+ run: pipx install "cmake~=3.31"Option B (preferred; cross-platform and simpler): use the maintained setup action for pipx, then install cmake. Replace both the pipx installation and cmake steps with:
- - if: "matrix.os == 'ubuntu24.04'"
- name: "Install pipx"
- shell: "bash"
- run: "DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y pipx"
-
- - name: "Install cmake"
- shell: "bash"
- run: "pipx install cmake~=3.31"
+ - name: "Set up pipx"
+ uses: pipx/actions/setup-pipx@<pin-a-specific-SHA>
+ - name: "Install CMake via pipx"
+ shell: bash
+ run: pipx install "cmake~=3.31"Note: align with your policy of pinning actions by commit SHA (replace accordingly). This keeps behaviour consistent across Ubuntu and macOS and avoids OS-conditional maintenance.
📝 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.
| - name: "Install cmake" | |
| shell: "bash" | |
| run: "pipx install cmake~=3.31" | |
| - if: startsWith(matrix.os, 'macos-') | |
| name: "Install pipx (macOS)" | |
| shell: bash | |
| run: brew install pipx | |
| - name: "Install CMake via pipx" | |
| shell: bash | |
| run: pipx install "cmake~=3.31" |
🤖 Prompt for AI Agents
In .github/workflows/clp-lint.yaml around lines 39-42, the workflow
unconditionally runs "pipx install cmake~=3.31" which fails on macOS because
pipx isn't installed; replace the current pipx+cmeake steps with a
cross-platform approach: add the maintained pipx setup action (pinned to a
specific commit SHA) before any pipx commands, then run pipx install cmake after
that, or alternately use OS-specific install steps (install pipx on
macOS/Ubuntu) — ensure the action is pinned by commit SHA and remove the
unconditional pipx install so the job succeeds on macOS and Ubuntu.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfiles/deps/main.yaml (1)
468-486: Making the ystdlib task public: check discoverability and docsExposing this task is useful for debugging and partial dependency builds. Ensure it’s discoverable in developer docs (e.g., mention “task deps:ystdlib”) and won’t be confused with higher-level “deps:core”.
I can draft a short paragraph for docs “Source Dependencies” explaining the new public task and when to use it.
Optionally, reduce maintenance churn by factoring the commit into a scoped variable, then interpolating it into the URL:
# Example refactor (illustrative) vars: YSTDLIB_COMMIT: "9ed78cd" tasks: ystdlib: cmds: - task: "yscope-dev-utils:remote:download-and-extract-tar" vars: FILE_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" URL: "https://github.com/y-scope/ystdlib-cpp/archive/{{.YSTDLIB_COMMIT}}.tar.gz"
📜 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)
docs/src/dev-guide/components-core/index.md(1 hunks)taskfiles/deps/main.yaml(1 hunks)
⏰ 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: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
docs/src/dev-guide/components-core/index.md (1)
54-54: ystdlib-cpp bump verified; adjust verification scriptThe docs and Taskfile both reference commit
9ed78cdfor ystdlib-cpp, so the revision bump itself is correct. The apparent “mismatch” in your script is due torg -nPoprepending the line number (e.g.54:9ed78cd), not an actual commit discrepancy.To fix, update the extraction commands to omit
-n, for example:--- task/check-ystd-sync.sh +++ task/check-ystd-sync.sh @@ -3,7 +3,7 @@ docs_file='docs/src/dev-guide/components-core/index.md' task_file='taskfiles/deps/main.yaml' -# Extract commits (includes line numbers) -docs_ystd_commit=$(rg -nPo '\[ystdlib-cpp.*\|\s*\K[0-9a-f]{7,40}' "$docs_file") +# Extract commits (omit line numbers) +docs_ystd_commit=$(rg -Po '\[ystdlib-cpp.*\|\s*\K[0-9a-f]{7,40}' "$docs_file") task_ystd_commit=$(rg -nPo 'ystdlib-cpp/archive/\K[0-9a-f]{7,40}' "$task_file" | tail -n1) echo "Docs ystdlib commit: $docs_ystd_commit"With this change, both variables will be
9ed78cd, and the test will pass:Docs ystdlib commit: 9ed78cd Taskfile ystdlib commit: 9ed78cd Docs and Taskfile are in sync.Revision bump approved.
taskfiles/deps/main.yaml (1)
479-482: Manual verification required — checksum tool unavailable
The sandbox lacked a SHA-256 utility, so the actual hash couldn’t be computed. Please manually verify that theFILE_SHA256matches the downloaded tarball, and consider pinning to the full 40-character commit for immutability.• File:
taskfiles/deps/main.yaml(lines 479–482)
• Current URL:
https://github.com/y-scope/ystdlib-cpp/archive/9ed78cd.tar.gz
• Expected SHA256 (in file):
f9baaae0886439862204e8062c69dd49354cb14ad4e59f7c35779d544384955e
• Resolved full commit SHA:
9ed78cd04fad63884b94b8ce55c465dd4009de77Action items:
- Download the tarball locally and run
sha256sum(or equivalent) to confirm the checksum matches.- Replace the URL’s short SHA (
9ed78cd) with the full commit SHA (9ed78cd04fad63884b94b8ce55c465dd4009de77) to prevent future ambiguity.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(2 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (2)
24-24: pipx is installed, but shims may not land on a global PATH — direct CMake/Task installs could be undiscoverable laterInstalling via pipx without setting PIPX_BIN_DIR typically places shims under ~/.local/bin, which is not guaranteed to be on PATH in CI or root shells. Set PIPX_HOME and PIPX_BIN_DIR before pipx installs so cmake and task land in /usr/local/bin.
Apply the environment initialisation once before the pipx installs (shown here adjacent to the CMake block below for locality):
# See also: https://github.com/y-scope/clp/issues/795 if command -v cmake ; then apt-get purge -y cmake fi -pipx install "cmake~=3.31" +export PIPX_HOME=/opt/pipx +export PIPX_BIN_DIR=/usr/local/bin +mkdir -p "$PIPX_HOME" "$PIPX_BIN_DIR" +pipx install --force "cmake~=3.31"
32-38: Align comment with constraint; make CMake install deterministic; verify version on PATH
- Comment says v3.31.6 while the constraint is ~=3.31. Prefer “v3.31.x” or pin exactly.
- Harden pipx placement (PIPX_HOME/BIN_DIR), force-reinstall for idempotency, and verify cmake is usable and within the expected minor.
- Consider not installing system cmake at all to avoid install-then-purge churn (see extra diff below).
Apply:
-# Install CMake v3.31.6 as ANTLR and yaml-cpp do not yet support CMake v4+. +# Install CMake v3.31.x as ANTLR and yaml-cpp do not yet support CMake v4+. # See also: https://github.com/y-scope/clp/issues/795 if command -v cmake ; then apt-get purge -y cmake fi -pipx install "cmake~=3.31" +export PIPX_HOME=/opt/pipx +export PIPX_BIN_DIR=/usr/local/bin +mkdir -p "$PIPX_HOME" "$PIPX_BIN_DIR" +pipx install --force "cmake~=3.31" +# Sanity checks +if ! command -v cmake >/dev/null ; then + echo "cmake shim not found on PATH (${PIPX_BIN_DIR})." >&2 + exit 1 +fi +cmake --version +cmake --version | grep -E '^cmake version 3\.31\.' >/dev/nullOutside this hunk: to avoid the install-then-purge cycle, drop cmake from the apt-get list:
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ ca-certificates \ checkinstall \ - cmake \ curl \ build-essential \ git \If you prefer an exact pin, update both the comment and installer to ==3.31.6 and adjust the grep accordingly. To validate across the repo, I can provide a script to scan CI logs for the reported CMake version.
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (2)
26-26: Bootstrap pipx safely: avoid PATH pitfalls and place shims in a global binInstalling pipx with --user puts the entry point under ~/.local/bin and a bare “pipx …” will likely fail. Use the module form and direct pipx shims to /usr/local/bin; create /opt/pipx for isolation.
-python3 -m pip install --user pipx +python3 -m pip install --user pipx +# Ensure pipx installs shims to a global bin dir on PATH and avoid PATH dependence for pipx +export PIPX_HOME=/opt/pipx +export PIPX_BIN_DIR=/usr/local/bin +mkdir -p "$PIPX_HOME" "$PIPX_BIN_DIR"
28-34: Align comment with constraint; make CMake install deterministic; avoid install-then-remove churn; verify version
- Comment claims v3.31.6 but installer uses ~=3.31 — align wording or pin exactly.
- Prefer python3 -m pipx to avoid PATH reliance; set PIPX_* as above; force install for idempotency; add version check.
- Consider dropping cmake from the initial dnf package list to avoid installing only to remove it.
Apply:
-# Install CMake v3.31.6 as ANTLR and yaml-cpp do not yet support CMake v4+. +# Install CMake v3.31.x as ANTLR and yaml-cpp do not yet support CMake v4+. # See also: https://github.com/y-scope/clp/issues/795 if command -v cmake ; then dnf remove -y cmake fi -pipx install "cmake~=3.31" +python3 -m pipx install --force "cmake~=3.31" +# Sanity checks +if ! command -v cmake >/dev/null ; then + echo "cmake shim not found on PATH (${PIPX_BIN_DIR})." >&2 + exit 1 +fi +cmake --version +cmake --version | grep -E '^cmake version 3\.31\.' >/dev/nullOutside this hunk: remove cmake from the dnf install list to avoid the remove step:
dnf install -y \ - cmake \ diffutils \ gcc-c++ \ git \
📜 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/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-05-06T09:48:55.408Z
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.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-07-01T14:52:15.217Z
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/ubuntu-jammy/install-prebuilt-packages.sh
📚 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/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 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/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-07-01T14:52:15.217Z
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/ubuntu-jammy/install-prebuilt-packages.sh
| # Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | ||
| if command -v task ; then | ||
| echo "placeholder" | ||
| exit 1 | ||
| fi | ||
| pipx install "go-task-bin>=3.40,<3.43" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Replace placeholder/exit with skip; install Task via python3 -m pipx with global shim path; verify version < 3.43
Mirror the Ubuntu flow for consistency across platform scripts, per team learnings.
-# Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872
-if command -v task ; then
- echo "placeholder"
- exit 1
-fi
-pipx install "go-task-bin>=3.40,<3.43"
+# Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872
+if command -v task ; then
+ echo "task already present; skipping install."
+else
+ python3 -m pipx install --force "go-task-bin>=3.40,<3.43"
+fi
+# Sanity check: ensure task is on PATH and within allowed range
+if ! command -v task >/dev/null ; then
+ echo "task not found on PATH (${PIPX_BIN_DIR})." >&2
+ exit 1
+fi
+task --version
+task --version | grep -E '[Vv]?3\.4[0-2]\.' >/dev/nullCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
around lines 35-40, replace the placeholder/exit flow with a skip-if-present
behavior and install Task using python3 -m pipx so the shim ends up on the
global pipx path (mirroring the Ubuntu script); specifically, if task is found
print a message and exit 0 (skip), otherwise run python3 -m pipx install
"go-task-bin>=3.40,<3.43" using the same pipx home/apps settings used in the
Ubuntu flow so the global shim is available, then run task --version and fail if
the installed version is >= 3.43 (exit non-zero) to enforce the <3.43
constraint.
| # Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | ||
| if command -v task ; then | ||
| echo "placeholder" | ||
| exit 1 | ||
| fi | ||
| pipx install "go-task-bin>=3.40,<3.43" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove placeholder/early-exit; install Task via pipx with the same path hardening; verify version < 3.43
Exiting when task exists is disruptive; just skip re-install. Also ensure the shim path is global and add a quick version guard to prevent regressions from yanked versions.
-# Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872
-if command -v task ; then
- echo "placeholder"
- exit 1
-fi
-pipx install "go-task-bin>=3.40,<3.43"
+# Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872
+if command -v task ; then
+ echo "task already present; skipping install."
+else
+ # Reuse the same PIPX_* dirs set above
+ : "${PIPX_BIN_DIR:=/usr/local/bin}"
+ : "${PIPX_HOME:=/opt/pipx}"
+ mkdir -p "$PIPX_HOME" "$PIPX_BIN_DIR"
+ pipx install --force "go-task-bin>=3.40,<3.43"
+fi
+# Sanity check: ensure task is on PATH and within allowed range
+if ! command -v task >/dev/null ; then
+ echo "task not found on PATH (${PIPX_BIN_DIR})." >&2
+ exit 1
+fi
+task --version
+task --version | grep -E '[Vv]?3\.4[0-2]\.' >/dev/null📝 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.
| # Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | |
| if command -v task ; then | |
| echo "placeholder" | |
| exit 1 | |
| fi | |
| pipx install "go-task-bin>=3.40,<3.43" | |
| # Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | |
| if command -v task ; then | |
| echo "task already present; skipping install." | |
| else | |
| : "${PIPX_BIN_DIR:=/usr/local/bin}" | |
| : "${PIPX_HOME:=/opt/pipx}" | |
| mkdir -p "$PIPX_HOME" "$PIPX_BIN_DIR" | |
| pipx install --force "go-task-bin>=3.40,<3.43" | |
| fi | |
| # Sanity check: ensure task is on PATH and within allowed range | |
| if ! command -v task >/dev/null ; then | |
| echo "task not found on PATH (${PIPX_BIN_DIR})." >&2 | |
| exit 1 | |
| fi | |
| task --version | |
| task --version | grep -E '[Vv]?3\.4[0-2]\.' >/dev/null |
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
around lines 39-44, remove the placeholder echo + exit and instead skip
reinstall when a suitable task binary already exists: check if command -v task
returns true and that task --version reports <3.43, and only proceed when
missing or out-of-range. When installing, invoke pipx with a hardened/shim path
(set PIPX_BIN_DIR to a global location like /usr/local/bin or the project
standard pipx bin dir) and use pipx install "go-task-bin>=3.40,<3.43" (use
--force if you need to upgrade), then add a post-install version guard that runs
task --version and exits nonzero if the installed version is >=3.43 to prevent
regressions from yanked versions.
Co-authored-by: kirkrodrigues <[email protected]>
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(1 hunks)docs/src/dev-docs/components-core/index.md(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-08-29T07:26:53.532Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:27-33
Timestamp: 2025-08-29T07:26:53.532Z
Learning: In CLP's build tool installation scripts, CMake version constraints should accommodate platform differences rather than using exact version pinning. Range constraints like "cmake>=3.23,<3.24" are preferred over exact pinning (cmake==3.23.5) to allow for platform-specific package availability while maintaining required version bounds.
Applied to files:
docs/src/dev-docs/components-core/index.mdcomponents/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/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-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/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/CMakeLists.txt
📚 Learning: 2025-08-20T23:25:15.863Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1226
File: components/core/CMakeLists.txt:6-6
Timestamp: 2025-08-20T23:25:15.863Z
Learning: In CMakeLists.txt files, CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_LIST_DIR have the same value since the CMakeLists.txt file is both the listfile being processed and defines the source directory. The distinction between these variables is more relevant for separate .cmake module files.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/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
📚 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.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-09-03T07:29:35.223Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
Applied to files:
components/core/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). (13)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
🔇 Additional comments (4)
docs/src/dev-docs/components-core/index.md (2)
59-59: ystdlib-cpp bump and URL formatting look correctCommit 9ed78cd matches the referenced dependency bump; URL format is consistent with other rows. LGTM.
59-59: Quick consistency check — ystdlib-cpp commit 9ed78cd confirmedDocs and taskfile reference the same commit:
- docs/src/dev-docs/components-core/index.md:59 — 9ed78cd
- taskfiles/deps/main.yaml:527 — URL contains 9ed78cd (FILE_SHA256 present)
No other differing commit hashes found.
components/core/CMakeLists.txt (2)
6-8: Post-project toolchain validation placement is correctIncluding Toolchains/utils.cmake and calling validate_compiler_versions() after project() avoids pre-project toolchain side-effects and aligns with the simplified approach in this PR. LGTM.
6-8: Confirmed: validate_compiler_versions enforces AppleClang ≥ 15, Clang ≥ 15, and GNU ≥ 11In components/core/cmake/Toolchains/utils.cmake the function sets CXX_COMPILER_MIN_VERSION to 15 for AppleClang/Clang and 11 for GNU, and issues a FATAL_ERROR when CMAKE_CXX_COMPILER_VERSION is VERSION_LESS the threshold; the error message includes the required version.
sitaowang1998
left a comment
sitaowang1998
left a comment
There was a problem hiding this comment.
Since latest ystdlib-cpp supports being installed as a cmake package, we should use this feature instead of importing the source files.
Apart from the suggestions in taskfile, please update the core's CMakeLists.txt to use find_package for ystdlib-cpp.
Bill-hbrhbr
changed the title
ystdlib to the newest version; Lower minimum Clang version for Velox compatibility.ystdlib to cmake library installation; Lower minimum Clang version for Velox compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
521-529: Use full commit SHA and a commit var; add LAZY install message; remove Catch2 settings.
- Short SHAs can collide; use the 40‑char SHA and store it in a var.
- Add -DCMAKE_INSTALL_MESSAGE=LAZY for quieter, consistent logs.
- If tests are off, don’t inject Catch2 settings.
ystdlib: internal: true run: "once" - deps: - - task: "boost" - - task: "catch2" + deps: + - task: "boost" cmds: - - task: "utils:install-remote-cmake-lib" + - task: "utils:install-remote-cmake-lib" vars: - CMAKE_GEN_ARGS: - - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" - - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_CATCH2_LIB_NAME}}.cmake" - - "-Dystdlib_BUILD_TESTING=OFF" - LIB_NAME: "ystdlib" - TARBALL_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" - TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/9ed78cd.tar.gz" + CMAKE_GEN_ARGS: + - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" + - "-DCMAKE_INSTALL_MESSAGE=LAZY" + - "-Dystdlib_BUILD_TESTING=OFF" + LIB_NAME: "ystdlib" + # TODO: replace with the full 40-char commit SHA and matching checksum + YSTDLIB_COMMIT: "9ed78cdXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" + TARBALL_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" + TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/{{.YSTDLIB_COMMIT}}.tar.gz"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/CMakeLists.txt(2 hunks)taskfiles/deps/main.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 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.txttaskfiles/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:
components/core/CMakeLists.txt
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/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/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/CMakeLists.txt
📚 Learning: 2025-08-20T23:25:15.863Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1226
File: components/core/CMakeLists.txt:6-6
Timestamp: 2025-08-20T23:25:15.863Z
Learning: In CMakeLists.txt files, CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_LIST_DIR have the same value since the CMakeLists.txt file is both the listfile being processed and defines the source directory. The distinction between these variables is more relevant for separate .cmake module files.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/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
📚 Learning: 2025-08-29T07:26:53.532Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:27-33
Timestamp: 2025-08-29T07:26:53.532Z
Learning: In CLP's build tool installation scripts, CMake version constraints should accommodate platform differences rather than using exact version pinning. Range constraints like "cmake>=3.23,<3.24" are preferred over exact pinning (cmake==3.23.5) to allow for platform-specific package availability while maintaining required version bounds.
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.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-09-03T07:29:35.223Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
Applied to files:
components/core/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-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
⏰ 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-14, true)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (4)
components/core/CMakeLists.txt (2)
6-8: Post-project toolchain validation is in the right place.Including utils and validating after project() avoids pre-project toolchain side-effects. Looks good.
296-298: Confirm ystdlib exported targets and add a minimum version
- Update components/core/CMakeLists.txt (around line 296) to require a minimum ystdlib version that contains the exported targets used by the repo (taskfiles/deps/main.yaml pins ystdlib-cpp at commit 9ed78cd). Example replacement:
-if(CLP_NEED_YSTDLIB) - find_package(ystdlib REQUIRED) +if(CLP_NEED_YSTDLIB) + # TODO: set the minimum version that contains the required targets/exports + find_package(ystdlib <MIN_VERSION> REQUIRED) message(STATUS "Found ystdlib-cpp ${ystdlib_VERSION}") endif()
- Confirm the upstream ystdlib package actually exports ystdlib::containers and ystdlib::error_handling (these targets are referenced throughout the repo — e.g. components/core/CMakeLists.txt lines ~762–763). If upstream uses different target names, either update the links to the correct targets or add alias targets.
taskfiles/deps/main.yaml (2)
22-22: Good: centralising Catch2 name.Promoting Catch2’s name to a global var improves consistency across tasks.
225-225: Good: use the shared Catch2 var in the task.Keeps task config DRY and aligned with other deps.
sitaowang1998
left a comment
sitaowang1998
left a comment
There was a problem hiding this comment.
LGTM. For the title, how about:
build(deps): Bump and switch `ystdlib` to package installation; Lower minimum Clang version for Velox compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
528-529: Pin ystdlib to a 40‑char commit and factor commit/SHA into vars.Short SHAs can collide; factoring also simplifies future bumps. Replace the inline URL and SHA with local vars.
ystdlib: internal: true + vars: + # Full 40-char commit to avoid ambiguity (replace with the actual full SHA for 9ed78cd…) + YSTDLIB_COMMIT: "9ed78cdXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" + YSTDLIB_TARBALL_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" run: "once" deps: - task: "boost" cmds: - task: "utils:install-remote-cmake-lib" vars: CMAKE_GEN_ARGS: - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" - "-DCMAKE_BUILD_TYPE=Release" - "-DCMAKE_INSTALL_MESSAGE=LAZY" - "-Dystdlib_BUILD_TESTING=OFF" LIB_NAME: "ystdlib" - TARBALL_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" - TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/9ed78cd.tar.gz" + TARBALL_SHA256: "{{.YSTDLIB_TARBALL_SHA256}}" + TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/{{.YSTDLIB_COMMIT}}.tar.gz"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: sitaowang1998
PR: y-scope/clp#1226
File: taskfiles/deps/main.yaml:518-520
Timestamp: 2025-09-15T17:27:39.113Z
Learning: The ystdlib dependency in taskfiles/deps/main.yaml requires Catch2 as a dependency even when built with -Dystdlib_BUILD_TESTING=OFF because ystdlib's cmake configuration references Catch2 internally.
📚 Learning: 2025-09-15T17:27:39.113Z
Learnt from: sitaowang1998
PR: y-scope/clp#1226
File: taskfiles/deps/main.yaml:518-520
Timestamp: 2025-09-15T17:27:39.113Z
Learning: The ystdlib dependency in taskfiles/deps/main.yaml requires Catch2 as a dependency even when built with -Dystdlib_BUILD_TESTING=OFF because ystdlib's cmake configuration references Catch2 internally.
Applied to files:
taskfiles/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:
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-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:
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). (6)
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
taskfiles/deps/main.yaml (2)
22-22: Parameterize Catch2 lib name — LGTM.Good move making Catch2’s lib name a public var for consistency with other deps.
225-225: Use of G_CATCH2_LIB_NAME here — LGTM.Keeps naming uniform and simplifies future renames.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
521-528: Pin to full commit SHA and factor commit/checksum into vars for safer bumps.Short SHAs can collide and are less auditable. Introduce ystdlib-local vars and reference them below.
Apply this diff:
ystdlib: internal: true + vars: + # Full 40-char commit for reproducibility + YSTDLIB_COMMIT: "REPLACE_WITH_FULL_40_CHAR_SHA" + YSTDLIB_TARBALL_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" run: "once" deps: - task: "boost" cmds: - task: "utils:install-remote-cmake-lib" vars: CMAKE_GEN_ARGS: - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" - "-DCMAKE_BUILD_TYPE=Release" - "-DCMAKE_INSTALL_MESSAGE=LAZY" - "-Dystdlib_BUILD_TESTING=OFF" LIB_NAME: "ystdlib" - TARBALL_SHA256: "65990dc2bcc4a355c2181bfe31a7800f492309d1bcd340f52a34e85047e61bc8" - TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/9ed78cd.tar.gz" + TARBALL_SHA256: "{{.YSTDLIB_TARBALL_SHA256}}" + TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/{{.YSTDLIB_COMMIT}}.tar.gz"After updating YSTDLIB_COMMIT, recompute and replace YSTDLIB_TARBALL_SHA256 with the checksum for the full-commit tarball.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-15T18:53:15.820Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.820Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.
Applied to files:
taskfiles/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:
taskfiles/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:
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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: package-image
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
taskfiles/deps/main.yaml (1)
517-528: Switch to utils:install-remote-cmake-lib for ystdlib looks good.Args are sane, Boost setting injection is correct, and disabling tests aligns with dropping Catch2 wiring here.
Please confirm CI covered AppleClang 15 and Clang 15 with this ystdlib commit (9ed78cd) and that no latent Catch2 references surface when BUILD_TESTING=OFF.
| CMAKE_GEN_ARGS: | ||
| - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" | ||
| - "-DCMAKE_BUILD_TYPE=Release" | ||
| - "-DCMAKE_INSTALL_MESSAGE=LAZY" | ||
| - "-Dystdlib_BUILD_TESTING=OFF" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add CMP0074=NEW to make Boost discovery deterministic across older CMake.
You’re injecting Boost settings via -C; if ystdlib relies on _ROOT semantics under CMake < 3.27, set CMP0074 to NEW like other deps (e.g., msgpack-cxx, spdlog).
Apply this diff:
CMAKE_GEN_ARGS:
- "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake"
+ - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW"
- "-DCMAKE_BUILD_TYPE=Release"
- "-DCMAKE_INSTALL_MESSAGE=LAZY"
- "-Dystdlib_BUILD_TESTING=OFF"Confirm ystdlib’s minimum required CMake version; if it’s >= 3.27, this is optional.
📝 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.
| CMAKE_GEN_ARGS: | |
| - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" | |
| - "-DCMAKE_BUILD_TYPE=Release" | |
| - "-DCMAKE_INSTALL_MESSAGE=LAZY" | |
| - "-Dystdlib_BUILD_TESTING=OFF" | |
| CMAKE_GEN_ARGS: | |
| - "-C {{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" | |
| - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW" | |
| - "-DCMAKE_BUILD_TYPE=Release" | |
| - "-DCMAKE_INSTALL_MESSAGE=LAZY" | |
| - "-Dystdlib_BUILD_TESTING=OFF" |
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 521 to 525, the CMake generator args
inject Boost settings via -C which can be non-deterministic on CMake < 3.27; add
the policy override CMP0074=NEW to the CMAKE_GEN_ARGS alongside the existing
flags so Boost_ROOT semantics are honored (matching how other deps set it), and
verify ystdlib's minimum required CMake version—if ystdlib requires >= 3.27 this
change is optional; otherwise add the policy to ensure deterministic Boost
discovery.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Deferring to @sitaowang1998's review.
For the PR title, how about:
build(deps): Bump `ystdlib` to y-scope/ystdlib-cpp@9ed78cd and install `ystdlib` via CMake; Lower minimum Clang version for Velox compatibility.
ystdlib to cmake library installation; Lower minimum Clang version for Velox compatibility.ystdlib to y-scope/ystdlib-cpp@9ed78cd and install ystdlib via CMake; Lower minimum Clang version for Velox compatibility.
4 participants
Description
Velox integration, which will depend on
ystdlib, requires us to lower our minimum compiler requirements to align withVelox's build constraints.Velox currently requires compiler versions of GNU11, Clang 15 and AppleClang15, so we must relax
ystdlibconstraints from LLVM/Apple Clang 16 down to 15 as well.We also remove the LLVM 16 toolchain upgrade and reintroduce
macos-14to the GitHub Actions workflows to ensure compatibility with AppleClang 15. In addition, since the same constraint adjustment was required and fixed inystdlib-cpp, we update this dependency to its latest version.On the other hand, since
ystdlib-cppis bumped to the current latest version, it now supports installation as a standard CMake library. Hence we update the installation task toutils:install-remote-cmake-libto simplify and standardize its installation process.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Chores
Refactor
Documentation