build(deps-dev): Use pipx to install version-constrained build tools; Bump minimum CMake version to 3.23 to support the latest ystdlib-cpp.#1271
Merged
Conversation
…atforms that people have access to
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Contributor
WalkthroughCentralizes installation of CMake, go-task, and uv via new pipx-based installers; removes legacy CMake-from-source and pre-check scripts; updates Dockerfiles and macOS CI to remove conflicting preinstalled packages and set pipx env; standardizes strict Bash options and script_dir resolution; updates docs and pins versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI / Docker Build
participant Image as Base Image
participant OS as OS Package Manager
participant Pipx as pipx
participant PxPk as pipx-packages scripts
participant Tools as cmake/task/uv
CI->>Image: docker build
rect rgba(230,240,255,0.5)
note right of Image: Pre-install cleanup
Image->>Pipx: pipx uninstall cmake
Image->>Pipx: pipx uninstall uv
end
Image->>OS: install system deps (including pipx where required)
Image->>PxPk: run pipx-packages/install-all.sh
PxPk->>Pipx: ensure/install cmake (>=3.23,<4)
PxPk->>Tools: cmake --system-information (version check)
PxPk->>Pipx: ensure/install go-task (==3.44.0)
PxPk->>Tools: task --silent (version check via Taskfile)
PxPk->>Pipx: ensure/install uv (>=0.8)
PxPk->>Tools: uv self version (json check)
alt Any constraint violated
PxPk-->>CI: exit 1 (fail build)
else All constraints satisfied
PxPk-->>CI: success
end
sequenceDiagram
autonumber
participant GH as GitHub Actions (macOS)
participant Brew as Homebrew
participant PxPk as pipx-packages scripts
participant Pipx as pipx
GH->>Brew: brew uninstall cmake (if present)
GH->>PxPk: run macOS install-all.sh -> pipx-packages/install-all.sh
PxPk->>Pipx: install/validate cmake, go-task, uv
PxPk-->>GH: success or failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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 |
Bill-hbrhbr
changed the title
pipx for build tool installation with precise versions; add scripts for cross-platform build tool version checkspipx to install version-constrained build tools.
…or false; Fix condition check.
Member
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
We should also update the minimum CMake version in CMakeLists.txt in this PR, right? (and update the PR title accordingly).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (5)
components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh (2)
16-21: Make uv invocable in the current shell; prefer module form of ensurepath; clarify flag name.
- After a fresh pipx install, uv may not be on PATH for the current shell. Use pipx which to resolve the binary and invoke that path.
- Use python3 -m pipx ensurepath for robustness/idempotency. (Matches prior feedback.)
- Rename package_preinstalled to installed_by_script for clarity.
Apply:
-package_preinstalled=0 +installed_by_script=0 if ! command -v uv >/dev/null 2>&1; then - package_preinstalled=1 + installed_by_script=1 pipx install --force "uv>=${required_version_min}" - pipx ensurepath + python3 -m pipx ensurepath || true fi
22-25: Guard jq dependency and resolve uv path via pipx which.Fail fast if jq is missing and avoid relying on PATH for uv.
+if ! command -v jq >/dev/null 2>&1; then + echo "Error: jq is required to parse uv version. Please install jq." + exit 1 +fi - -installed_version=$(uv self version --output-format json | jq --raw-output ".version") -IFS=. read -r installd_version_major installed_version_minor _ <<<"${installed_version}" +uv_bin="$(pipx which uv)" +installed_version="$("$uv_bin" self version --output-format json | jq -r '.version')" +IFS=. read -r installed_version_major installed_version_minor _ <<<"${installed_version}"components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh (2)
18-25: Fix inverted/unclear “preinstalled” flag and branch accordinglyThe flag meaning is inverted vs. its name, which is error‑prone. Use an explicit had_existing_cmake boolean and branch messages accordingly.
Apply:
-package_preinstalled=0 -if ! command -v cmake >/dev/null 2>&1; then - package_preinstalled=1 - # ystdlib requires CMake v3.23; ANTLR and yaml-cpp do not yet support CMake v4+ - # (see https://github.com/y-scope/clp/issues/795). - pipx install --force "cmake>=${required_version_min},<${required_version_major_max_plus_1}" - pipx ensurepath -fi +had_existing_cmake=0 +if command -v cmake >/dev/null 2>&1; then + had_existing_cmake=1 +else + # ystdlib requires CMake v3.23; ANTLR and yaml-cpp do not yet support CMake v4+ + # (see https://github.com/y-scope/clp/issues/795). + pipx install --force "cmake>=${required_version_min},<${required_version_major_max_plus_1}" + pipx ensurepath +fi @@ - if ((0 == "${package_preinstalled}")); then + if (( had_existing_cmake == 1 )); then echo "Please uninstall CMake and then re-run the install script." else echo "pipx failed to install the required version of CMake." echo "To uninstall, run:" echo " pipx uninstall cmake" fiAlso applies to: 40-46
23-25: Refresh PATH in the current shell after pipx installpipx ensurepath doesn’t affect the running non‑interactive shell; cmake may still be undiscoverable immediately after install.
Apply:
pipx install --force "cmake>=${required_version_min},<${required_version_major_max_plus_1}" pipx ensurepath + # Ensure this shell sees pipx-installed binaries + if [[ -n "${PIPX_BIN_DIR:-}" ]]; then + export PATH="${PIPX_BIN_DIR}:$PATH" + else + export PATH="$HOME/.local/bin:$PATH" + fi + hash -r + if ! command -v cmake >/dev/null 2>&1; then + echo "Error: CMake appears installed but is not on PATH for this shell." >&2 + echo "PATH=$PATH" >&2 + exit 1 + ficomponents/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh (1)
22-24: Harden version probe and detect Taskwarrior collisionCommand substitution won’t reliably trigger errexit; if Taskwarrior is first on PATH, this line either fails silently or returns garbage. Guard the probe and emit a clear error when Taskwarrior is detected.
Apply:
script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" -task_version=$(task --silent --taskfile "${script_dir}/print-go-task-version.yaml") +if ! task_version="$(task --silent --taskfile "${script_dir}/print-go-task-version.yaml" 2>/dev/null)"; then + if command -v task >/dev/null 2>&1 && task --version 2>&1 | grep -qi 'taskwarrior'; then + echo "Error: Found a different 'task' on PATH (likely Taskwarrior). Remove it or adjust PATH so go-task is used, then re-run." + else + echo "Error: Unable to invoke 'task'. Ensure pipx's bin directory is on PATH and go-task ${required_version} is installed." + fi + exit 1 +fi ```<!-- review_comment_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: ASSERTIVE **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 56a9a545d76844b12f01c3530ec970b75ed6b646 and 9297f67c8270fe8dc3b886126317a2c3a82421b7. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh` (1 hunks) * `components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh` (1 hunks) * `components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (4)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: Bill-hbrhbr
PR: #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.</details> <details> <summary>📚 Learning: 2025-08-29T07:31:24.032Z</summary>Learnt from: Bill-hbrhbr
PR: #1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:41-43
Timestamp: 2025-08-29T07:31:24.032Z
Learning: In CLP's build tool installation scripts, uv version constraints should use lower bound constraints (uv>=0.8) rather than exact version pinning, following the same philosophy used for other build tools to accommodate platform differences while ensuring minimum required functionality.**Applied to files:** - `components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh` </details> <details> <summary>📚 Learning: 2025-07-07T17:41:15.655Z</summary>Learnt from: jackluo923
PR: #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 ininstall-prebuilt-packages.sh, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like--strip-components=1to handle potential tarball layout changes.**Applied to files:** - `components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh` </details> <details> <summary>📚 Learning: 2025-08-29T07:26:53.532Z</summary>Learnt from: Bill-hbrhbr
PR: #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/tools/scripts/lib_install/pipx-packages/install-cmake.sh` </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: musllinux_1_2-x86_64-deps-image * GitHub Check: centos-stream-9-deps-image * GitHub Check: ubuntu-jammy-deps-image * GitHub Check: manylinux_2_28-x86_64-deps-image * GitHub Check: build-macos (macos-15, true) * GitHub Check: build-macos (macos-15, false) </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh (1)</summary><blockquote> `12-15`: **Good: version-floor constraint is aligned with team guidance.** Using uv>=0.8 matches the “lower-bound not exact pin” philosophy captured in prior learnings. No change needed. </blockquote></details> <details> <summary>components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh (1)</summary><blockquote> `12-17`: **Good: version range constraints match policy** Using a bounded range (>=3.23,<4) aligns with the project’s guidance to accommodate platform availability while enforcing requirements. </blockquote></details> <details> <summary>components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh (2)</summary><blockquote> `12-14`: **Pin to 3.44.0 is appropriate here** Given y-scope/clp-ffi-js#110, exact pinning is justified for Task. LGTM. <!-- review_comment_end --> --- `3-6`: **Minor bash strictness nit** Consider adding errtrace so traps propagate through functions/subshells if you later add them. Example: ```diff set -o errexit set -o nounset set -o pipefail +set -o errtrace ```<!-- review_comment_end --> <!-- file_end --> <details> <summary>⛔ Skipped due to learnings</summary>Learnt from: jackluo923
PR: #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 ininstall-prebuilt-packages.sh, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like--strip-components=1to handle potential tarball layout changes.</details> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+27
to
+30
| installed_version=$(cmake -E capabilities | jq --raw-output ".version.string") | ||
| installed_version_major=$(cmake -E capabilities | jq --raw-output ".version.major") | ||
| installed_version_minor=$(cmake -E capabilities | jq --raw-output ".version.minor") | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
jq is an unchecked hard dependency; add check or robust fallback and avoid triple capability calls
On systems without jq the script fails; also you invoke cmake -E capabilities three times. Parse once and fall back to cmake --version when jq is unavailable.
Apply:
-installed_version=$(cmake -E capabilities | jq --raw-output ".version.string")
-installed_version_major=$(cmake -E capabilities | jq --raw-output ".version.major")
-installed_version_minor=$(cmake -E capabilities | jq --raw-output ".version.minor")
+if command -v jq >/dev/null 2>&1; then
+ caps="$(cmake -E capabilities)"
+ installed_version="$(jq -r '.version.string' <<<"$caps")"
+ installed_version_major="$(jq -r '.version.major' <<<"$caps")"
+ installed_version_minor="$(jq -r '.version.minor' <<<"$caps")"
+else
+ ver_line="$(cmake --version | head -n1)"
+ installed_version="$(awk '{print $3}' <<<"$ver_line")"
+ installed_version_major="${installed_version%%.*}"
+ installed_version_minor="${installed_version#*.}"
+ installed_version_minor="${installed_version_minor%%.*}"
+fi
Comment on lines
+33
to
+36
| if (("${installed_version_major}" < "${required_version_major_min}")) \ | ||
| || (("${installed_version_major}" == "${required_version_major_min}" && \ | ||
| "${installed_version_minor}" < "${required_version_minor_min}")) \ | ||
| || (("${installed_version_major}" >= "${required_version_major_max_plus_1}")); then |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify and harden the version bound check
Current multi‑line arithmetic with quotes is hard to read and easy to mis-edit. Use a single arithmetic evaluation without quotes.
Apply:
-if (("${installed_version_major}" < "${required_version_major_min}")) \
- || (("${installed_version_major}" == "${required_version_major_min}" && \
- "${installed_version_minor}" < "${required_version_minor_min}")) \
- || (("${installed_version_major}" >= "${required_version_major_max_plus_1}")); then
+if (( installed_version_major < required_version_major_min
+ || (installed_version_major == required_version_major_min && installed_version_minor < required_version_minor_min)
+ || installed_version_major >= required_version_major_max_plus_1 )); then📝 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 (("${installed_version_major}" < "${required_version_major_min}")) \ | |
| || (("${installed_version_major}" == "${required_version_major_min}" && \ | |
| "${installed_version_minor}" < "${required_version_minor_min}")) \ | |
| || (("${installed_version_major}" >= "${required_version_major_max_plus_1}")); then | |
| # before (to be removed): | |
| -if (("${installed_version_major}" < "${required_version_major_min}")) \ | |
| - || (("${installed_version_major}" == "${required_version_major_min}" && "${installed_version_minor}" < "${required_version_minor_min}")) \ | |
| - || (("${installed_version_major}" >= "${required_version_major_max_plus_1}")); then | |
| # after (replacement): | |
| if (( installed_version_major < required_version_major_min | |
| || (installed_version_major == required_version_major_min && installed_version_minor < required_version_minor_min) | |
| || installed_version_major >= required_version_major_max_plus_1 )); then |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh
around lines 33 to 36, the multi-line quoted arithmetic test is hard to read and
fragile; replace it with a single arithmetic evaluation without quotes or
escaped newlines. Use one if (( ... )); then condition combining comparisons
with || and &&, e.g. no quotes around variables, no backslashes/newlines, and
ensure the variables are integer-safe (declare or validate if necessary) so the
condition reads as a single compact expression checking major < min_major OR
(major == min_major AND minor < min_minor) OR major >= max_plus_1.
Comment on lines
+37
to
+38
| echo "Error: CMake version ${installed_version} is unsupported (require" \ | ||
| "${required_version_min} ≤ version < ${required_version_major_max_plus_1})." |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor grammar fix in error message
“require” → “requires”.
Apply:
- echo "Error: CMake version ${installed_version} is unsupported (require" \
- "${required_version_min} ≤ version < ${required_version_major_max_plus_1})."
+ echo "Error: CMake version ${installed_version} is unsupported (requires ${required_version_min} ≤ version < ${required_version_major_max_plus_1})."📝 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
| echo "Error: CMake version ${installed_version} is unsupported (require" \ | |
| "${required_version_min} ≤ version < ${required_version_major_max_plus_1})." | |
| echo "Error: CMake version ${installed_version} is unsupported (requires ${required_version_min} ≤ version < ${required_version_major_max_plus_1})." |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh
around lines 37 to 38, the error message uses incorrect grammar
("require")—update the string to use "requires" so the message reads "requires
${required_version_min} ≤ version < ${required_version_major_max_plus_1}" while
preserving the variable interpolation and overall formatting.
Comment on lines
+7
to
+10
| if ! command -v pipx >/dev/null 2>&1; then | ||
| echo "Error: pipx not found." | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Offer auto-remediation guidance when pipx is missing
Optional: print a short hint for installing pipx to reduce user friction.
Example:
if ! command -v pipx >/dev/null 2>&1; then
- echo "Error: pipx not found."
+ echo "Error: pipx not found."
+ echo "Install pipx, e.g.: python3 -m pip install --user pipx && python3 -m pipx ensurepath"
exit 1
fi
```<!-- review_comment_end -->
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
if ! command -v pipx >/dev/null 2>&1; then
echo "Error: pipx not found."
echo "Install pipx, e.g.: python3 -m pip install --user pipx && python3 -m pipx ensurepath"
exit 1
fi🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh
around lines 7 to 10, the script exits with a terse "pipx not found" error;
update it to print a short actionable hint telling users how to install pipx
(for example, suggest installing pipx via the recommended user-level pip
installation and ensuring it's on PATH, or via common system package managers)
so the script still exits but provides clear remediation guidance.
Comment on lines
+15
to
+21
| package_preinstalled=0 | ||
| if ! command -v task >/dev/null 2>&1; then | ||
| package_preinstalled=1 | ||
| pipx install --force "go-task-bin==${required_version}" | ||
| pipx ensurepath | ||
| fi | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix PATH update after pipx install and clarify flag semantics
After a fresh pipx install, the current shell’s PATH won’t include pipx’s bin dir, so the immediate version probe can fail or hit another binary named task. Also, the flag name suggests “preinstalled == 1”, but you set it to 1 when task is absent, which is confusing.
Apply:
package_preinstalled=0
if ! command -v task >/dev/null 2>&1; then
package_preinstalled=1
pipx install --force "go-task-bin==${required_version}"
- pipx ensurepath
+ python3 -m pipx ensurepath || true
+ # Make pipx-installed binaries available in the current shell
+ export PATH="$HOME/.local/bin:$PATH"
+ hash -r
fi(Optional: rename to task_preexisted=0/1 to match meaning.)
📝 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
| package_preinstalled=0 | |
| if ! command -v task >/dev/null 2>&1; then | |
| package_preinstalled=1 | |
| pipx install --force "go-task-bin==${required_version}" | |
| pipx ensurepath | |
| fi | |
| package_preinstalled=0 | |
| if ! command -v task >/dev/null 2>&1; then | |
| package_preinstalled=1 | |
| pipx install --force "go-task-bin==${required_version}" | |
| python3 -m pipx ensurepath || true | |
| # Make pipx-installed binaries available in the current shell | |
| export PATH="$HOME/.local/bin:$PATH" | |
| hash -r | |
| fi |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh
around lines 15-21, the script sets package_preinstalled=1 when task is missing
and probes for task immediately after pipx install without updating PATH; change
the boolean to reflect actual meaning (e.g., task_preexisted=1 by default or
task_preexisted=0 then set to 1 only if command exists) and update PATH after
pipx install before re-checking the binary. Specifically: set a clearly named
flag (task_preexisted) initialized to 0, check if task exists and set it to 1 if
present; when installing, run pipx install, then add pipx's user bin to PATH
(for portability use export PATH="$(python3 -m site --user-base)/bin:$PATH") or
otherwise update PATH so the newly installed task is found, then re-check
command -v task; ensure the flag value reflects whether task existed prior to
installation.
Comment on lines
+24
to
+37
| if [[ "${task_version}" != "${required_version}" ]]; then | ||
| echo "Error: Task version ${task_version} is currently unsupported (must be" \ | ||
| "${required_version})." | ||
|
|
||
| if ((0 == "${package_preinstalled}")); then | ||
| echo "Please uninstall Task and then re-run the install script." | ||
| else | ||
| echo "pipx failed to install the required version of Task." | ||
| echo "To uninstall, run:" | ||
| echo " pipx uninstall go-task-bin" | ||
| fi | ||
|
|
||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve mismatch branch and messaging; simplify arithmetic test
Make the error actionable and avoid quoting inside arithmetic contexts. Also surface which binary is being picked up.
Apply:
-if [[ "${task_version}" != "${required_version}" ]]; then
- echo "Error: Task version ${task_version} is currently unsupported (must be" \
- "${required_version})."
+if [[ "${task_version}" != "${required_version}" ]]; then
+ echo "Error: go-task version ${task_version} is unsupported; required exactly ${required_version}."
- if ((0 == "${package_preinstalled}")); then
- echo "Please uninstall Task and then re-run the install script."
+ if (( package_preinstalled == 0 )); then
+ echo "A different 'task' is on PATH (likely Taskwarrior) at: $(command -v task)"
+ echo "Remove it or reorder PATH so go-task ${required_version} is used, then re-run."
else
- echo "pipx failed to install the required version of Task."
- echo "To uninstall, run:"
- echo " pipx uninstall go-task-bin"
+ echo "pipx-installed go-task not found at the required version. Check PATH or reinstall:"
+ echo " pipx uninstall go-task-bin && pipx install --force \"go-task-bin==${required_version}\""
fi
exit 1
fi
```<!-- review_comment_end -->
<!-- fingerprinting:phantom:triton:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+1
to
+10
| #!/usr/bin/env bash | ||
|
|
||
| set -o errexit | ||
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| if ! command -v pipx >/dev/null 2>&1; then | ||
| echo "Error: pipx not found." | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: add a short usage hint on failure.
Improve UX when pipx is missing.
if ! command -v pipx >/dev/null 2>&1; then
- echo "Error: pipx not found."
+ echo "Error: pipx not found. Install via: python3 -m pip install --user pipx && python3 -m pipx ensurepath"
exit 1
fi📝 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
| #!/usr/bin/env bash | |
| set -o errexit | |
| set -o nounset | |
| set -o pipefail | |
| if ! command -v pipx >/dev/null 2>&1; then | |
| echo "Error: pipx not found." | |
| exit 1 | |
| fi | |
| #!/usr/bin/env bash | |
| set -o errexit | |
| set -o nounset | |
| set -o pipefail | |
| if ! command -v pipx >/dev/null 2>&1; then | |
| echo "Error: pipx not found. Install via: python3 -m pip install --user pipx && python3 -m pipx ensurepath" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh around
lines 1 to 10, the script exits with a plain "Error: pipx not found." which
provides no guidance to the user; update the error handling to print a short
usage hint describing how to install pipx (e.g., pointing to pipx installation
command or documentation) and optionally suggest re-running the script after
installation, then exit with status 1. Ensure the message is concise and
user-friendly.
Comment on lines
+23
to
+40
| installed_version=$(uv self version --output-format json | jq --raw-output ".version") | ||
| IFS=. read -r installd_version_major installed_version_minor _ <<<"${installed_version}" | ||
|
|
||
| if (("${installd_version_major}" == "${required_version_major_min}" && \ | ||
| "${installed_version_minor}" < "${required_version_minor_min}")); then | ||
| echo "Error: uv version ${installed_version} is unsupported (require version" \ | ||
| "≥ ${required_version_min})." | ||
|
|
||
| if ((0 == "${package_preinstalled}")); then | ||
| echo "Please uninstall uv and then re-run the install script." | ||
| else | ||
| echo "pipx failed to install the required version of uv." | ||
| echo "To uninstall, run:" | ||
| echo " pipx uninstall uv" | ||
| fi | ||
|
|
||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: tolerate legacy uv JSON flag.
If older uv doesn’t support --output-format json, fall back to plain version and parse. Small robustness gain.
-installed_version="$("$uv_bin" self version --output-format json | jq -r '.version')"
+if installed_version_json="$("$uv_bin" self version --output-format json 2>/dev/null)"; then
+ installed_version="$(printf '%s' "$installed_version_json" | jq -r '.version')"
+else
+ installed_version="$("$uv_bin" --version | awk '{print $NF}')"
+fiCommittable suggestion skipped: line range outside the PR's diff.
Comment on lines
+26
to
+27
| if (("${installd_version_major}" == "${required_version_major_min}" && \ | ||
| "${installed_version_minor}" < "${required_version_minor_min}")); then |
Contributor
There was a problem hiding this comment.
Fix arithmetic test and variable typo; remove quotes in (( )).
The current test mixes quoted strings inside arithmetic context and uses a misspelled variable (installd_version_major).
-if (("${installd_version_major}" == "${required_version_major_min}" && \
- "${installed_version_minor}" < "${required_version_minor_min}")); then
+if (( installed_version_major == required_version_major_min && \
+ installed_version_minor < required_version_minor_min )); then📝 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 (("${installd_version_major}" == "${required_version_major_min}" && \ | |
| "${installed_version_minor}" < "${required_version_minor_min}")); then | |
| if (( installed_version_major == required_version_major_min && \ | |
| installed_version_minor < required_version_minor_min )); then |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh around
lines 26 to 27, the arithmetic test uses quoted variables and a misspelled
variable name; change the condition to use the correct variable name
(installed_version_major) and unquoted variables inside an arithmetic context,
e.g. use: if (( installed_version_major == required_version_major_min &&
installed_version_minor < required_version_minor_min )); then — ensure you
remove the quotes and fix the typo.
Comment on lines
+31
to
+37
| if ((0 == "${package_preinstalled}")); then | ||
| echo "Please uninstall uv and then re-run the install script." | ||
| else | ||
| echo "pipx failed to install the required version of uv." | ||
| echo "To uninstall, run:" | ||
| echo " pipx uninstall uv" | ||
| fi |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align branch with renamed flag; use numeric check in (( )).
Keep the messaging but switch to installed_by_script and proper numeric comparison.
- if ((0 == "${package_preinstalled}")); then
+ if (( installed_by_script == 0 )); then
echo "Please uninstall uv and then re-run the install script."
else
echo "pipx failed to install the required version of uv."
echo "To uninstall, run:"
echo " pipx uninstall uv"
fi🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh around
lines 31 to 37, the conditional uses the old package_preinstalled name and
quotes the variable inside (( )), producing an incorrect numeric comparison;
update the condition to use the renamed flag installed_by_script and a proper
numeric comparison (e.g., if (( installed_by_script == 0 )); then ...) and keep
the existing echo messages and branches unchanged.
pipx to install version-constrained build tools.pipx to install version-constrained build tools; Bump minimum required clp-core CMake version to 3.23.
Bill-hbrhbr
changed the title
pipx to install version-constrained build tools; Bump minimum required clp-core CMake version to 3.23.pipx to install version-constrained build tools; Bump minimum required clp-core CMake version to 3.23.
Member
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
build(deps-dev): Use `pipx` to install version-constrained build tools; Bump minimum `CMake` version to `3.23` to support latest ystdlib-cpp.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Bill-hbrhbr
changed the title
pipx to install version-constrained build tools; Bump minimum required clp-core CMake version to 3.23.pipx to install version-constrained build tools; Bump minimum CMake version to 3.23 to support latest ystdlib-cpp.
Bill-hbrhbr
changed the title
pipx to install version-constrained build tools; Bump minimum CMake version to 3.23 to support latest ystdlib-cpp.pipx to install version-constrained build tools; Bump minimum CMake version to 3.23 to support the latest ystdlib-cpp.
kirkrodrigues
approved these changes
Sep 10, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (3)
3-5: Add a lightweight ERR trap for fast failure context.Keeps strict mode while emitting the failing line for quicker triage. Apply across platform scripts for consistency.
set -o errexit set -o nounset set -o pipefail +trap 'echo "Error on line $LINENO of ${BASH_SOURCE[0]} (exit $?)" >&2' ERR
23-25: Install/upgrade pipx via module and run ensurepath via module.More robust in non-interactive shells and keeps logs quiet; aligns with prior guidance.
-if ! command -v pipx >/dev/null 2>&1; then - python3 -m pip install pipx -fi +if ! command -v pipx >/dev/null 2>&1; then + python3 -m pip install --upgrade pipx +fi +python3 -m pipx ensurepath
27-29: Standardise pipx install dirs before invoking installer.Avoid PATH surprises by aligning with Dockerfile defaults.
# Install remaining packages through pipx script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" +export PIPX_HOME="${PIPX_HOME:-/opt/_internal/pipx}" +export PIPX_BIN_DIR="${PIPX_BIN_DIR:-/usr/local/bin}" "${script_dir}/../pipx-packages/install-all.sh"components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (1)
17-19: Provision pipx and export PIPX_ before invoking the shared installer.*On manylinux images, pipx may be absent; calling the installer will fail. Exporting PIPX_HOME/BIN and PATH ensures predictable locations. This mirrors patterns requested across platform scripts.
Apply:
-# Install remaining packages through pipx -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" -"${script_dir}/../pipx-packages/install-all.sh" +# Install remaining packages through pipx +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" +if ! command -v pipx >/dev/null 2>&1; then + dnf install -y pipx python3-pip || python3 -m pip install --upgrade pipx +fi +export PIPX_HOME="${PIPX_HOME:-/opt/_internal/pipx}" +export PIPX_BIN_DIR="${PIPX_BIN_DIR:-/usr/local/bin}" +export PATH="${PIPX_BIN_DIR}:${PATH}" +"${script_dir}/../pipx-packages/install-all.sh"components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh (1)
18-20: Ensure pipx exists on Alpine/musl and export PIPX_ consistently.*Alpine images often lack pipx; add an idempotent guard and set PIPX dirs/Path before invoking the installer.
-# Install remaining packages through pipx -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" -"${script_dir}/../pipx-packages/install-all.sh" +# Install remaining packages through pipx +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" +if ! command -v pipx >/dev/null 2>&1; then + apk add --no-cache py3-pipx || python3 -m pip install --upgrade pipx +fi +export PIPX_HOME="${PIPX_HOME:-/opt/_internal/pipx}" +export PIPX_BIN_DIR="${PIPX_BIN_DIR:-/usr/local/bin}" +export PATH="${PIPX_BIN_DIR}:${PATH}" +"${script_dir}/../pipx-packages/install-all.sh"components/core/tools/scripts/lib_install/macos/install-all.sh (2)
9-18: Fix Homebrew formula: use openjdk@11, not java11.java11 isn’t a valid/core formula on macOS; openjdk@11 is the standard tap.
Apply:
- java11 \ + openjdk@11 \
28-30: Pre-flight pipx availability, export PIPX dirs, and add post-install diagnostics.Ensure pipx is usable in this session and make installs deterministic; echo tool versions to aid CI triage. Also, per retrieved learnings, confirm downstream pipx installers use range constraints (cmake>=3.23,<3.24 and uv>=0.8) rather than exact pins.
Apply:
-# Install remaining packages through pipx -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" -"${script_dir}/../pipx-packages/install-all.sh" +# Install remaining packages through pipx +export PIPX_HOME="${PIPX_HOME:-/opt/_internal/pipx}" +export PIPX_BIN_DIR="${PIPX_BIN_DIR:-/usr/local/bin}" +if ! command -v pipx >/dev/null 2>&1; then + brew install pipx +fi +python3 -m pipx ensurepath || true +hash -r +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" +"${script_dir}/../pipx-packages/install-all.sh" +# Diagnostics: show resolved tools and versions +command -v cmake >/dev/null 2>&1 && cmake --version || true +command -v task >/dev/null 2>&1 && task --version || true +command -v uv >/dev/null 2>&1 && (uv --version || uv -V) || trueUsing context from retrieved learnings: prefer version ranges for CMake (>=3.23,<3.24) and uv (>=0.8) across platform scripts to accommodate availability while meeting minimums.
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (2)
3-5: Standardize strict-mode flags to the project’s 2‑line pattern.Match other scripts for consistency.
-set -o errexit -set -o nounset -set -o pipefail +set -eu +set -o pipefail
31-33: Harden script_dir, preflight pipx, and set safe PIPX defaults before invoking installers.Prevents PATH/env drift and symlink surprises; keeps installers reproducible.
-# Install remaining packages through pipx -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" -"${script_dir}/../pipx-packages/install-all.sh" +# Install remaining packages through pipx +readonly script_dir="$( + cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P +)" +: "${PIPX_HOME:=/opt/_internal/pipx}" +: "${PIPX_BIN_DIR:=/usr/local/bin}" +mkdir -p "${PIPX_HOME}" "${PIPX_BIN_DIR}" +if ! command -v pipx >/dev/null 2>&1; then + echo "pipx not found on PATH; ensure it is installed." >&2 + exit 1 +fi +bash "${script_dir}/../pipx-packages/install-all.sh"Additionally, please verify the central installers enforce the agreed version bounds:
- cmake: cmake>=3.23,<3.24 (range, not exact pin)
- go-task: avoid Taskwarrior; constrain to a safe range that excludes v3.44.1 regression (per y-scope/clp-ffi-js#110) and <3.43 per #872 context
- uv: uv>=0.8 (lower bound)
#!/usr/bin/env bash set -euo pipefail dir="components/core/tools/scripts/lib_install/pipx-packages" echo "Checking version constraints in ${dir}…" fd -a 'install-*.sh' "$dir" \ | xargs -I{} sh -c 'echo "== {} =="; rg -nE "(cmake|go-task|go-task-bin|uv).*(>=|==|<)" {} || true' echo echo "Detecting explicit exclusion or notes about go-task v3.44.1…" rg -n "3\.44\.1" "$dir" || echo "No explicit 3.44.1 mention found" echo echo "Grepping for Taskwarrior disambiguation (version banner check)…" rg -nE 'task\s+--version|Task version v3' "$dir" || echo "No disambiguation logic detected"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(2 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh(2 hunks)components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh(2 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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.
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.
📚 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/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/macos/install-all.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
📚 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/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/macos/install-all.sh
📚 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/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/centos-stream-9/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.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
📚 Learning: 2025-08-25T00:45:05.464Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.464Z
Learning: Task v3.44.1 has a regression that breaks shell command processing, particularly rsync commands with brace expansion (e.g., `file.{d.ts,js,wasm}`). This causes CI failures in clp-ffi-js project (issue #110), so CLP should avoid v3.44.1 and use v3.44.0 instead, which fixes the dynamic variable bug without the shell processing regression.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-08-29T07:31:24.032Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:41-43
Timestamp: 2025-08-29T07:31:24.032Z
Learning: In CLP's build tool installation scripts, uv version constraints should use lower bound constraints (uv>=0.8) rather than exact version pinning, following the same philosophy used for other build tools to accommodate platform differences while ensuring minimum required functionality.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/macos/install-all.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/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.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
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.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/centos-stream-9/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.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/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/musllinux_1_2/install-prebuilt-packages.sh
📚 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/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
⏰ 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-deps-image
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (4)
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (1)
3-4: Strict mode change looks good.Consistent with other platform scripts and improves failure visibility.
components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh (1)
3-4: LGTM on stricter Bash options.Keeps behaviour consistent across install scripts.
components/core/tools/scripts/lib_install/macos/install-all.sh (2)
3-5: Strict Bash options: good hardening.errexit/nounset/pipefail are appropriate here.
24-26: pkg-config guard is correct.The conditional avoids conflicts on hosted runners that preinstall pkg-config.
Comment on lines
+9
to
+29
| ca-certificates \ | ||
| checkinstall \ | ||
| curl \ | ||
| build-essential \ | ||
| git \ | ||
| jq \ | ||
| libcurl4 \ | ||
| libcurl4-openssl-dev \ | ||
| liblzma-dev \ | ||
| libmariadb-dev \ | ||
| libssl-dev \ | ||
| openjdk-11-jdk \ | ||
| pipx \ | ||
| pkg-config \ | ||
| python3 \ | ||
| python3-dev \ | ||
| python3-pip \ | ||
| python3-venv \ | ||
| rsync \ | ||
| software-properties-common \ | ||
| unzip |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reduce image size: clean apt caches after install.
Saves dozens of MB in CI images.
software-properties-common \
unzip
+
+# Minimise image size
+apt-get clean
+rm -rf /var/lib/apt/lists/*📝 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
| ca-certificates \ | |
| checkinstall \ | |
| curl \ | |
| build-essential \ | |
| git \ | |
| jq \ | |
| libcurl4 \ | |
| libcurl4-openssl-dev \ | |
| liblzma-dev \ | |
| libmariadb-dev \ | |
| libssl-dev \ | |
| openjdk-11-jdk \ | |
| pipx \ | |
| pkg-config \ | |
| python3 \ | |
| python3-dev \ | |
| python3-pip \ | |
| python3-venv \ | |
| rsync \ | |
| software-properties-common \ | |
| unzip | |
| ca-certificates \ | |
| checkinstall \ | |
| curl \ | |
| build-essential \ | |
| git \ | |
| jq \ | |
| libcurl4 \ | |
| libcurl4-openssl-dev \ | |
| liblzma-dev \ | |
| libmariadb-dev \ | |
| libssl-dev \ | |
| openjdk-11-jdk \ | |
| pipx \ | |
| pkg-config \ | |
| python3 \ | |
| python3-dev \ | |
| python3-pip \ | |
| python3-venv \ | |
| rsync \ | |
| software-properties-common \ | |
| unzip | |
| # Minimise image size | |
| apt-get clean | |
| rm -rf /var/lib/apt/lists/* |
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
around lines 9 to 29, the apt packages are installed but the apt cache and lists
are not cleaned, increasing image size; after the apt-get install step add
commands to clean apt caches and lists (e.g., apt-get clean -y && rm -rf
/var/lib/apt/lists/*) and optionally run apt-get autoremove -y to remove
unneeded packages so the resulting image is smaller.
Merged
3 tasks
4 tasks
junhaoliao
pushed a commit
to junhaoliao/clp
that referenced
this pull request
May 17, 2026
…s; Bump minimum `CMake` version to `3.23` to support the latest ystdlib-cpp. (y-scope#1271) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 switches the installation of build tools
cmake,task, anduvfrom system package managers (apt,dnf,apk,brew) topipx. Usingpipxlets us avoid stale distro packages and makes it easy to pin exact versions to address the following dependency compatibility issues:task packagehangs with task >= 3.43.1 #872 (resolved)Adopting
pipxensures a uniform toolchain setup across Linux distributions and macOS. So this PR introduces shared installation scripts usingpipxso that all environments rely on the same process and install the same versions of tools.This PR provides version check scripts for the build tools, which alert users to manually remove any system-installed versions that don’t meet the required version constraints. This prevents mixing
pipx-installed tools with versions introduced by other package managers.Checklist
breaking change.
Validation performed
Summary by CodeRabbit