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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…atforms that people have access to
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/src/dev-docs/building-package.md (1)
11-12: Fix grammar (“a some” → “some”/“a bit of”).User-facing docs: remove the extra article.
Apply:
- * It should be possible to build a package for a different environment, it just requires a some - extra configuration. + * It should be possible to build a package for a different environment; it just requires some + extra configuration.docs/src/dev-docs/contributing-linting.md (1)
38-38: Remove unused reference anchor.
[clp-issue-872]isn’t referenced in this page anymore; drop the dead link to avoid confusion.Apply:
-[clp-issue-872]: https://github.com/y-scope/clp/issues/872docs/src/dev-docs/components-core/index.md (1)
125-131: References block: ensure all newly cited anchors are defined.Add missing anchors for any newly added “canonical source” links per above.
♻️ Duplicate comments (17)
docs/README.md (2)
17-17: Add canonical source link for the Task pin to avoid drift.Point readers to the single source of truth (version-check/install script) introduced in this PR so docs stay aligned with enforcement.
Apply:
* [Task] 3.44.0 - * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * Canonical source: see [go-task version check][check-go-task-version] and [pipx installer][install-go-task]. @@ [Task]: https://taskfile.dev/ [y-scope/clp-ffi-js#110]: https://github.com/y-scope/clp-ffi-js/issues/110 +[check-go-task-version]: components/core/tools/scripts/lib_install/lib_version_checks/check-go-task-version.sh +[install-go-task]: components/core/tools/scripts/lib_install/pipx_install/install-go-task.sh
48-48: Keep references centralised across docs.Nice addition. Consider also adding a short note that the version check script is authoritative (same as above) for consistency across pages.
docs/src/dev-docs/building-package.md (2)
17-17: Reference the canonical Task version source and installer.Link to the version-check script and pipx installer added in this PR to prevent doc/code divergence.
Apply:
* [Task] 3.44.0 - * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * Canonical source: see [go-task version check][check-go-task-version] and [pipx installer][install-go-task]. @@ [Task]: https://taskfile.dev/ [y-scope/clp-ffi-js#110]: https://github.com/y-scope/clp-ffi-js/issues/110 +[check-go-task-version]: components/core/tools/scripts/lib_install/lib_version_checks/check-go-task-version.sh +[install-go-task]: components/core/tools/scripts/lib_install/pipx_install/install-go-task.sh
83-83: Mirror the canonical-source links in references.Ensure the two new references above are included; see suggested diff.
docs/src/dev-docs/contributing-linting.md (1)
19-21: Add links to the canonical version-check and pipx installer scripts.This PR standardises on pipx; reflect that here so contributors use the same paths/scripts as CI.
Apply:
* [Task] 3.44.0 - * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * Canonical source: see [go-task version check][check-go-task-version]. + * Install via pipx: see [install-go-task][install-go-task]. @@ [Task]: https://taskfile.dev/ [uv]: https://docs.astral.sh/uv/ [y-scope/clp-ffi-js#110]: https://github.com/y-scope/clp-ffi-js/issues/110 [yscope-dev-utils]: https://github.com/y-scope/yscope-dev-utils +[check-go-task-version]: components/core/tools/scripts/lib_install/lib_version_checks/check-go-task-version.sh +[install-go-task]: components/core/tools/scripts/lib_install/pipx_install/install-go-task.shdocs/src/dev-docs/components-core/index.md (1)
16-16: Also link Task’s canonical check/installer here.Keep consistency with other docs; point to the same scripts.
Apply:
* [Task] 3.44.0 * We pin the version to 3.44.0 due to [y-scope/clp-ffi-js#110]. + * Canonical source: see [go-task version check][check-go-task-version] and [pipx installer][install-go-task]. @@ [Task]: https://taskfile.dev/ [y-scope/clp-ffi-js#110]: https://github.com/y-scope/clp-ffi-js/issues/110 [y-scope/clp#795]: https://github.com/y-scope/clp/issues/795 [yscope-dev-utils]: https://github.com/y-scope/yscope-dev-utils +[check-go-task-version]: components/core/tools/scripts/lib_install/lib_version_checks/check-go-task-version.sh +[install-go-task]: components/core/tools/scripts/lib_install/pipx_install/install-go-task.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2-x86_64/Dockerfile (1)
8-9: Guard pipx uninstalls and collapse to one layerUnconditional uninstalls can fail when packages aren’t present; also creates extra layers.
Apply:
-RUN pipx uninstall cmake -RUN pipx uninstall uv +RUN if command -v pipx >/dev/null 2>&1; then \ + pipx uninstall cmake || true; \ + pipx uninstall go-task-bin || true; \ + pipx uninstall uv || true; \ + fi.github/workflows/clp-core-build-macos.yaml (1)
69-74: Harden brew cleanup and cover all pipx-managed toolsAlso handle missing Homebrew, prevent auto-update noise, and remove go-task/uv to avoid conflicts.
Apply:
- - name: "Remove preinstalled programs which will conflict with the pipx-installed packages" - run: | - if brew list --formula cmake >/dev/null 2>&1; then - brew uninstall --force cmake - fi + - name: "Remove preinstalled programs which will conflict with the pipx-installed packages" + run: | + if command -v brew >/dev/null 2>&1; then + export HOMEBREW_NO_AUTO_UPDATE=1 + for f in cmake go-task uv; do + if brew list --formula "$f" >/dev/null 2>&1; then + brew uninstall --force "$f" + fi + done + ficomponents/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile (1)
8-9: Guard pipx uninstalls and collapse to one layerSame risk as musllinux: builds can fail if the packages aren’t installed.
Apply:
-RUN pipx uninstall cmake -RUN pipx uninstall uv +RUN if command -v pipx >/dev/null 2>&1; then \ + pipx uninstall cmake || true; \ + pipx uninstall go-task-bin || true; \ + pipx uninstall uv || true; \ + ficomponents/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/Dockerfile (1)
8-9: Guard pipx calls, include go-task, and collapse to one layer.Uninstalls will fail if pipx isn’t present; also missing go-task cleanup and creates extra layers.
-RUN pipx uninstall cmake -RUN pipx uninstall uv +RUN if command -v pipx >/dev/null 2>&1; then \ + : "${PIPX_HOME:=${PIPX_HOME:-/opt/_internal/pipx}}" "${PIPX_BIN_DIR:=${PIPX_BIN_DIR:-/usr/local/bin}}"; \ + pipx uninstall cmake || true; \ + pipx uninstall go-task-bin || true; \ + pipx uninstall uv || true; \ + ficomponents/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile (1)
8-9: Consolidate and harden uninstalls; cover go-task too.Reduce layers, avoid failures when pipx is absent, and remove any stale go-task.
-RUN pipx uninstall cmake -RUN pipx uninstall uv +RUN if command -v pipx >/dev/null 2>&1; then \ + pipx uninstall cmake || true; \ + pipx uninstall go-task-bin || true; \ + pipx uninstall uv || true; \ + ficomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.sh (1)
3-5: Use project-standard strict mode form.Match existing scripts: separate lines for -eu and pipefail.
-set -o errexit -set -o nounset -set -o pipefail +# Exit on any error, use of undefined variables, and pipeline failures +set -eu +set -o pipefailcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh (1)
3-5: Standardize strict mode across platform scripts.Use the same two-line pattern as elsewhere.
-set -o errexit -set -o nounset -set -o pipefail +set -eu +set -o pipefailcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)
33-34: Export PIPX dirs before invoking pipx installers (for non-Docker runs).Helps when running this script outside images that set ENV.
-# Install remaining packages through pipx -"${script_dir}/../pipx-packages/install-all.sh" +# Install remaining packages through pipx +export PIPX_HOME="${PIPX_HOME:-/opt/pipx}" +export PIPX_BIN_DIR="${PIPX_BIN_DIR:-/usr/local/bin}" +"${script_dir}/../pipx-packages/install-all.sh"components/core/tools/scripts/lib_install/macos/install-all.sh (1)
30-31: Export PIPX_ before delegation for deterministic installs.*
Prevents PATH surprises on self-hosted macOS.# Install remaining packages through pipx -"${script_dir}/../pipx-packages/install-all.sh" +export PIPX_HOME="${PIPX_HOME:-$HOME/.local/pipx}" +export PIPX_BIN_DIR="${PIPX_BIN_DIR:-$HOME/.local/bin}" +"${script_dir}/../pipx-packages/install-all.sh"components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (2)
25-27: pipx bootstrap: add ensurepath and quiet checks.
Use the module form and ensure PATH, else downstream may not find pipx-installed bins.-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 || true
29-30: Standardise pipx dirs before invoking installer.
Keeps parity with container images.# Install remaining packages through pipx -"${script_dir}/../pipx-packages/install-all.sh" +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"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/clp-core-build-macos.yaml(1 hunks)components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86_64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile(1 hunks)components/core/tools/scripts/lib_install/centos-stream-9/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(2 hunks)components/core/tools/scripts/lib_install/check-cmake-version.sh(0 hunks)components/core/tools/scripts/lib_install/install-cmake.sh(0 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-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-all.sh(1 hunks)components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh(2 hunks)components/core/tools/scripts/lib_install/pipx-packages/install-all.sh(1 hunks)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)components/core/tools/scripts/lib_install/pipx-packages/print-go-task-version.yaml(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)docs/README.md(2 hunks)docs/src/dev-docs/building-package.md(2 hunks)docs/src/dev-docs/components-core/index.md(2 hunks)docs/src/dev-docs/contributing-linting.md(2 hunks)taskfiles/lint.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- components/core/tools/scripts/lib_install/install-cmake.sh
- components/core/tools/scripts/lib_install/check-cmake-version.sh
🧰 Additional context used
🧠 Learnings (17)
📓 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.
📚 Learning: 2025-08-04T18:38:33.130Z
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.
Applied to files:
docs/README.mddocs/src/dev-docs/building-package.mddocs/src/dev-docs/contributing-linting.mddocs/src/dev-docs/components-core/index.md
📚 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:
docs/README.mddocs/src/dev-docs/contributing-linting.mddocs/src/dev-docs/components-core/index.mdcomponents/core/tools/scripts/lib_install/ubuntu-jammy/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 causes issues in the clp-ffi-js project (issue #110), so CLP should avoid upgrading to v3.44.1 and stick with v3.44.0 for now.
Applied to files:
docs/README.mddocs/src/dev-docs/building-package.mddocs/src/dev-docs/contributing-linting.mddocs/src/dev-docs/components-core/index.md
📚 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:
.github/workflows/clp-core-build-macos.yamlcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-all.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/Dockerfilecomponents/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/pipx-packages/install-go-task.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.shcomponents/core/tools/scripts/lib_install/macos/install-all.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/manylinux_2_28/install-all.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/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.shcomponents/core/tools/scripts/lib_install/macos/install-all.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/manylinux_2_28/install-all.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-all.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-all.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.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/manylinux_2_28/install-all.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/musllinux_1_2/install-all.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/manylinux_2_28/install-all.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/manylinux_2_28/install-all.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-all.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-all.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.sh
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
components/core/tools/scripts/lib_install/manylinux_2_28/install-all.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-all.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-all.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.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:
docs/src/dev-docs/contributing-linting.mdcomponents/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/manylinux_2_28/install-prebuilt-packages.shdocs/src/dev-docs/components-core/index.mdcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/macos/install-all.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/centos-stream-9/install-prebuilt-packages.shdocs/src/dev-docs/components-core/index.mdcomponents/core/tools/scripts/lib_install/ubuntu-jammy/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/centos-stream-9/install-prebuilt-packages.shdocs/src/dev-docs/components-core/index.mdcomponents/core/tools/scripts/lib_install/ubuntu-jammy/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.shdocs/src/dev-docs/components-core/index.md
📚 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/centos-stream-9/install-prebuilt-packages.shdocs/src/dev-docs/components-core/index.mdcomponents/core/tools/scripts/lib_install/ubuntu-jammy/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.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/install-all.sh
🪛 Shellcheck (0.10.0)
components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh
[warning] 14-14: package_exists appears unused. Verify use (or export if used externally).
(SC2034)
components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh
[warning] 16-16: package_exists appears unused. Verify use (or export if used externally).
(SC2034)
components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh
[warning] 14-14: package_exists appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (10)
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
🔇 Additional comments (22)
taskfiles/lint.yaml (1)
103-103: Extending yamllint coverage to tools dir — LGTMGood addition; this will catch YAML issues in newly added pipx configs.
components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh (1)
3-5: Strict mode + script_dir normalisation — LGTMConsistent with our style guidance and cross-platform scripts. Removing the legacy CMake check aligns with the new pipx flow.
Also applies to: 7-7
components/core/tools/scripts/lib_install/centos-stream-9/install-all.sh (1)
3-5: Strict mode + script_dir normalisation — LGTMMatches ubuntu-jammy; removal of per-OS CMake check is appropriate with pipx installers.
Also applies to: 7-7
components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh (1)
25-31: Version range is correct; aligns with our “range, not exact pin” guidance — LGTMConstraint
cmake>=3.23,<4matches our learnings and avoids v4 incompatibilities.components/core/tools/scripts/lib_install/pipx-packages/install-all.sh (1)
3-5: Installer orchestrator — LGTMStraightforward, strict, and correctly invokes package installers from the same dir.
Also applies to: 7-11
components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile (2)
8-11: PIPX_HOME is already consistent across Dockerfiles
Bothclp-env-base-ubuntu-jammy/Dockerfileandclp-env-base-centos-stream-9/DockerfilesetENV PIPX_HOME=/opt/pipx, so there’s no mismatch to align.Likely an incorrect or invalid review comment.
13-13: Version constraints correctly enforced in pipx installers
The pipx installers now enforce cmake >=3.23,<4, go-task-bin ==3.44.0, and uv >=0.8 as required.components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile (1)
13-13: All pipx installers use the correct version constraints
cmake: “>=3.23,<4.0”; go-task-bin: “==3.44.0”; uv: “>=0.8” – no changes required.components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (2)
11-31: Package list LGTM; pipx inclusion is appropriate.No functional concerns with the updated apt set.
33-34: Installers enforce required versions. install-cmake.sh pins cmake ≥ 3.23,< 4.0; install-go-task.sh pins go-task-bin == 3.44.0; install-uv.sh pins uv ≥ 0.8.components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh (3)
3-4: Strict mode: good hardening.
Enables safer failure semantics.
7-7: Robust script_dir detection: looks good.
Works under Bash; consistent with other platforms.
20-21: Provision pipx before pipx-packages invocation in musllinux_1_2
The Alpine script in components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh doesn’t install pipx. Insert before the pipx-packages call:if ! command -v pipx >/dev/null 2>&1; then apk add --no-cache pipx || python3 -m pip install --upgrade pipx python3 -m pipx ensurepath || true fi 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"Please verify the Alpine package name (‘pipx’ vs ‘py3-pipx’) (pkgs.alpinelinux.org, pipx.pypa.io) and confirm these PIPX_* defaults align with project conventions.
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (2)
3-4: Strict mode: good.
Safer defaults.
7-7: script_dir definition: OK.
Prevents fragile relative paths.components/core/tools/scripts/lib_install/macos/install-all.sh (2)
3-5: Strict mode: good.
Consistent with Linux scripts.
26-26: pkg-config guard: correct and quiet.
Avoids brew conflicts on GitHub runners.components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (2)
3-5: Strict mode: good.
Matches the other platforms.
7-7: script_dir: correct.
Prevents relative-path pitfalls.components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh (2)
9-12: pipx presence check: good.
Early, clear failure.
24-35: Fixpackage_existsconditional and verify Taskfile presence.
Use integer comparison forpackage_exists(e.g.if (( package_exists == 1 )); then) instead of treating it as a command. Confirm that${script_dir}/print-go-task-version.yamlis committed alongside this script.⛔ Skipped due to learnings
Learnt from: jackluo923 PR: y-scope/clp#1054 File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32 Timestamp: 2025-07-07T17:41:15.655Z Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.Learnt from: 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 causes issues in the clp-ffi-js project (issue #110), so CLP should avoid upgrading to v3.44.1 and stick with v3.44.0 for now.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.components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh (1)
7-10: pipx presence check: good.
Early validation is clear.
Comment on lines
+8
to
+11
| # Set pipx environment vars so that the pipx scripts install into the global locations without | ||
| # passing the `--global` flag. | ||
| ENV PIPX_BIN_DIR=/usr/local/bin | ||
| ENV PIPX_HOME=/opt/pipx |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure PIPX_HOME aligns with other images.
Ubuntu sets /opt/pipx; verify many/musl images and CI use the same, or set safe defaults in the installers to avoid cross-image drift.
🏁 Script executed:
#!/usr/bin/env bash
rg -n 'ENV\s+PIPX_HOME' components/core/tools/docker-images -C1Length of output: 581
🏁 Script executed:
#!/usr/bin/env bash
# List all Dockerfiles under the docker-images directory
echo "== Dockerfiles =="
fd --type f -e Dockerfile components/core/tools/docker-images
# Show all occurrences of pipx (including ENV settings) in those files
echo -e "\n== pipx occurrences =="
rg -n 'pipx' -C2 components/core/tools/docker-imagesLength of output: 5055
Consolidate PIPX environment variables across images
musllinux and manylinux Dockerfiles don’t define ENV PIPX_BIN_DIR=/usr/local/bin and ENV PIPX_HOME=/opt/pipx. Add these or enforce the same defaults in your installer scripts to avoid cross-image drift.
🤖 Prompt for AI Agents
In components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile
around lines 8 to 11, the Dockerfile sets PIPX_BIN_DIR and PIPX_HOME but other
images (musllinux, manylinux) lack these SAME ENV settings causing cross-image
drift; ensure consistency by adding ENV PIPX_BIN_DIR=/usr/local/bin and ENV
PIPX_HOME=/opt/pipx to the musllinux and manylinux Dockerfiles (or alternatively
update the shared installer scripts to export these defaults before pipx
installs) so all images use the same pipx paths.
Comment on lines
+11
to
+20
| coreutils \ | ||
| gcc \ | ||
| java11 \ | ||
| libarchive \ | ||
| llvm@16 \ | ||
| lz4 \ | ||
| mariadb-connector-c \ | ||
| pipx \ | ||
| xz \ | ||
| zstd |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify Homebrew formula names; “java11” is likely incorrect.
Homebrew commonly uses openjdk@11 (not java11). Please verify and adjust to avoid CI breakage. Consider ensuring pipx PATH as well.
- java11 \
+ openjdk@11 \
libarchive \
@@
- pipx \
+ pipx \Additionally, add:
brew update
+python3 -m pipx ensurepath || true📝 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
| coreutils \ | |
| gcc \ | |
| java11 \ | |
| libarchive \ | |
| llvm@16 \ | |
| lz4 \ | |
| mariadb-connector-c \ | |
| pipx \ | |
| xz \ | |
| zstd | |
| brew update | |
| python3 -m pipx ensurepath || true | |
| brew install \ | |
| coreutils \ | |
| gcc \ | |
| openjdk@11 \ | |
| libarchive \ | |
| llvm@16 \ | |
| lz4 \ | |
| mariadb-connector-c \ | |
| pipx \ | |
| xz \ | |
| zstd |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/macos/install-all.sh around lines
11 to 20, the Homebrew formula list likely contains an incorrect formula name
("java11") and missing pipx PATH handling; replace "java11" with the correct
Homebrew formula "openjdk@11", verify all other formula names against Homebrew
(e.g., llvm@16, mariadb-connector-c), and ensure pipx is installed and
functional by running the Homebrew install for pipx and then invoking the pipx
path setup (e.g., call pipx ensurepath or add the pipx binary path to PATH) so
CI won't break due to missing/incorrect formula names or pipx not being on PATH.
| set -o pipefail | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm script_dir resolution matches peers.
Align with the prevailing pattern across lib_install to avoid subtle drift.
🤖 Prompt for AI Agents
components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh lines 7:
the script_dir assignment must match the prevailing lib_install pattern used
across other scripts; locate the canonical assignment (e.g. the exact variable
name and resolution expression used in sibling scripts), replace the current
line with that exact expression so behavior and compatibility are identical, and
run a quick shellcheck or small test to confirm the resolved path matches peers.
Comment on lines
19
to
20
| # Install remaining packages through pipx | ||
| "${script_dir}/../pipx-packages/install-all.sh" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provision pipx and export dirs before calling the installer.
On manylinux images pipx may be absent; the call will fail. Exporting PIPX_* stabilises paths.
# Install remaining packages through pipx
-"${script_dir}/../pipx-packages/install-all.sh"
+if ! command -v pipx >/dev/null 2>&1; then
+ dnf install -y pipx python3-pip || python3 -m pip install --upgrade pipx
+ python3 -m pipx ensurepath || true
+fi
+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"📝 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
| # Install remaining packages through pipx | |
| "${script_dir}/../pipx-packages/install-all.sh" | |
| # Install remaining packages through pipx | |
| if ! command -v pipx >/dev/null 2>&1; then | |
| dnf install -y pipx python3-pip || python3 -m pip install --upgrade pipx | |
| python3 -m pipx ensurepath || true | |
| fi | |
| 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" |
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
around lines 19-20, ensure pipx is provisioned and PIPX environment variables
are exported before invoking the installer: check for pipx and install it if
missing (e.g. via python -m pip install --user pipx), export stable PIPX_HOME
and PIPX_BIN_DIR (and update PATH) so the installer can find pipx, and only then
call "${script_dir}/../pipx-packages/install-all.sh".
| set -o pipefail | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Keep script_dir style consistent.
If other platform scripts use the simpler pwd form (no -P), mirror that for consistency; otherwise consider adopting -P everywhere together in a follow-up.
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/musllinux_1_2/install-all.sh around
line 7, the script_dir assignment should follow the simpler pwd style used by
other platform scripts; replace any usage of "pwd -P" with plain "pwd" here so
this file matches the existing simpler form (if other scripts actually use plain
pwd) — if you discover most scripts use "-P" instead, convert this file to use
"pwd -P" and plan a follow-up to standardize all scripts consistently.
Comment on lines
22
to
33
| if (("${uv_major_version}" == 0 && "${uv_minor_version}" < 8)); then | ||
| echo "Error: uv version ${uv_version} is unsupported (require version ≥ 0.8)." | ||
|
|
||
| if package_exists; 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 |
There was a problem hiding this comment.
Fix arithmetic test and conditional bug.
The (( )) expression mixes strings and uses a non-functional “if package_exists”. Use numeric test and proper variable check.
-if (("${uv_major_version}" == 0 && "${uv_minor_version}" < 8)); then
+if (( uv_major_version == 0 && uv_minor_version < 8 )); then
echo "Error: uv version ${uv_version} is unsupported (require version ≥ 0.8)."
- if package_exists; then
+ if (( package_exists == 1 )); 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📝 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 (("${uv_major_version}" == 0 && "${uv_minor_version}" < 8)); then | |
| echo "Error: uv version ${uv_version} is unsupported (require version ≥ 0.8)." | |
| if package_exists; 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 | |
| if (( uv_major_version == 0 && uv_minor_version < 8 )); then | |
| echo "Error: uv version ${uv_version} is unsupported (require version ≥ 0.8)." | |
| if (( package_exists == 1 )); 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 |
🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh around
lines 22 to 33, the arithmetic check mixes quoted strings in (( )) and the
package existence check is non-functional; replace the conditional with a proper
numeric test and ensure the package check invokes the function or tests a
boolean variable. Concretely, change the version guard to use POSIX numeric
comparisons e.g. if [ "$uv_major_version" -eq 0 ] && [ "$uv_minor_version" -lt 8
]; then (or remove the quotes inside a single (( uv_major_version == 0 &&
uv_minor_version < 8 )) expression) and replace the non-functional "if
package_exists" with either a direct function invocation if package_exists is a
function (if package_exists; then) or a boolean variable check (if [
"$package_exists" = "true" ]; then) so the branch correctly detects installed
package state before printing uninstall instructions and exiting.
Comment on lines
+1
to
+6
| version: "3" | ||
|
|
||
| tasks: | ||
| default: | ||
| cmd: >- | ||
| echo {{ .TASK_VERSION | trimPrefix "v" }} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Silence Task output for cleaner version parsing.
Avoids extra noise if the calling script parses stdout.
version: "3"
tasks:
default:
+ silent: true
cmd: >-
echo {{ .TASK_VERSION | trimPrefix "v" }}📝 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
| version: "3" | |
| tasks: | |
| default: | |
| cmd: >- | |
| echo {{ .TASK_VERSION | trimPrefix "v" }} | |
| version: "3" | |
| tasks: | |
| default: | |
| silent: true | |
| cmd: >- | |
| echo {{ .TASK_VERSION | trimPrefix "v" }} |
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/pipx-packages/print-go-task-version.yaml
lines 1-6, the task currently echoes the version to stdout which can add noise
for callers parsing output; make the task silent by adding the Taskfile "silent:
true" flag under the default task (same indentation level as cmd) so the task
won’t print extra runtime info while still emitting the trimmed version on
stdout.
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
Show resolved
Hide resolved
|
|
||
| # Error on undefined variable | ||
| set -u | ||
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Harden script_dir or standardize across scripts.
If other installers use a unified script_dir idiom, mirror it here to reduce drift (and decide together on -P usage).
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
around line 7, the script_dir assignment should be hardened/standardized to
match the project's other installers; replace the current cd/dirname idiom with
the agreed standard (either use cd -P to resolve symlinks before pwd or use
readlink -f on ${BASH_SOURCE[0]}) so script_dir always points to the real script
directory, and update this file to exactly mirror the chosen pattern used by the
other installers.
Comment on lines
+12
to
+14
| * [CMake] >= 3.23.0 and < 4.0.0 | ||
| * Minimum version 3.23.0 is required for [yscope-dev-utils]. | ||
| * We constrain the version to < 4.0.0 due to [y-scope/clp#795]. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good call on CMake bounds; add link to canonical constraint/check (if present).
If the PR added a CMake version-check script, link it as the authority to prevent drift between docs and checks.
Apply (adjust path if different):
* [CMake] >= 3.23.0 and < 4.0.0
* Minimum version 3.23.0 is required for [yscope-dev-utils].
* We constrain the version to < 4.0.0 due to [y-scope/clp#795].
+ * Canonical source: see [CMake version check][check-cmake-version].
@@
[yscope-dev-utils]: https://github.com/y-scope/yscope-dev-utils
+[check-cmake-version]: components/core/tools/scripts/lib_install/lib_version_checks/check-cmake-version.shIf there is no CMake check script, ignore this suggestion.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/src/dev-docs/components-core/index.md around lines 12 to 14, the docs
mention CMake version bounds but don’t link to the authoritative version-check
script; locate the repository's CMake version-check script (e.g.,
scripts/check_cmake_version.(sh|py) or cmake/FindYScope.cmake) and add a link to
it next to the version bullet so the docs reference the canonical check, and if
no such script exists leave the docs unchanged; ensure the link path is
repo-relative and update wording to say "See <path> for the canonical version
check."
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.
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") | ||
|
|
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 |
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})." |
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 |
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 | ||
|
|
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 |
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 |
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 |
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 |
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 |
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.
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
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 |
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.
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