fix(deps-dev): Ensure pipx-installed tools are discoverable during platform dependency script execution.#1320
fix(deps-dev): Ensure pipx-installed tools are discoverable during platform dependency script execution.#1320Bill-hbrhbr wants to merge 52 commits into
Conversation
WalkthroughCentralizes pipx installation/discovery, adds sudo-aware re-exec in pipx orchestration, prepends pipx bin dir to PATH for installers, and changes tool installers to uninstall then reinstall with simplified version checks and unified messaging. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bill-hbrhbr
changed the title
sudo before pipx installs to ensure user-scoped pipx packages.sudo before pipx installs to ensure user-scoped pipx packages.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
-
components/core/tools/scripts/lib_install/pipx-packages/install-all.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
components/core/tools/scripts/lib_install/pipx-packages/install-all.sh
[style] 7-7:
(SC2004)
⏰ 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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
components/core/tools/scripts/lib_install/pipx-packages/install-all.sh (1)
7-11: Confirm intended behaviour when run as root directly (not via sudo).If invoked as root without sudo, the guard doesn’t trigger and installs proceed as root. Is that desired for CI/containers, or should we warn/exit?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/tools/scripts/lib_install/pipx-packages/install-all.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: package-image
🔇 Additional comments (3)
components/core/tools/scripts/lib_install/pipx-packages/install-all.sh (3)
7-7: LGTM: arithmetic EUID check with nounset-safe default.
Matches ShellCheck guidance; good fix.
10-11: LGTM: robust re-exec using env bash and BASH_SOURCE; PATH preserved.
Prevents$0pitfalls and keeps PATH intact for user tools.
7-12: No remaining sudo-prefixed pipx invocations found — resolved.
Searched repository with rg -nP --hidden --no-ignore-vcs -S '\bsudo\b[^\n]*\bpipx\b' — no matches.
Bill-hbrhbr
changed the title
sudo before pipx installs to ensure user-scoped pipx packages.sudo before pipx installs to ensure user-scoped pipx packages; Allow pipx package changes to trigger artifact building CI.
… version checking
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh(2 hunks)components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh(2 hunks)components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/pipx-packages/install-uv.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/pipx-packages/install-cmake.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). (4)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh (1)
15-25: Consistent Task binary handling reads well.Using
go_task_binto drive both discovery and logging keeps preinstalled-versus-pipx-installed cases aligned. Nice touch.components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh (1)
32-34: Thanks for reusing the discoveredcmakebinary here.Parsing the capabilities JSON off the resolved path eliminates
$PATHdependence and keeps the version gate reliable.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/core/tools/scripts/lib_install/pipx-packages/find-pipx-bin.sh (1)
25-29: Fix jq query against pipx JSON and fail fast if not found.pipx’s JSON exposes main_package.app_paths as a map of app->path, not an array with Path. The current jq returns nothing. Resolve by querying the map (with a safe fallback) and exit non‑zero if no path is found.
Apply:
-pipx list --json | jq --raw-output --arg pkg "$pkg" --arg app "$app" ' - .venvs[$pkg].metadata.main_package.app_paths[]?.__Path__ - | select((split("/") | last) == $app) -' | head -n1 +bin_path="$( + pipx list --json \ + | jq -re --arg pkg "$pkg" --arg app "$app" ' + .venvs[$pkg].metadata.main_package.app_paths[$app] + // (.venvs[$pkg].metadata.main_package.app_paths[]? | select(endswith("/"+$app))) + ' +)" || { + echo "Error: failed to locate binary '$app' for package '$pkg' via pipx." >&2 + exit 3 +} +echo "${bin_path}"components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh (1)
7-10: Add a jq dependency check.This script uses jq to parse CMake capabilities. Without jq, set -e will fail without a clear message.
if ! command -v pipx >/dev/null 2>&1; then echo "Error: pipx not found." exit 1 fi + +if ! command -v jq >/dev/null 2>&1; then + echo "Error: jq not found." + exit 1 +ficomponents/core/tools/scripts/lib_install/pipx-packages/install-uv.sh (2)
7-10: Add a jq dependency check.jq is required to parse uv’s JSON version output.
if ! command -v pipx >/dev/null 2>&1; then echo "Error: pipx not found." exit 1 fi + +if ! command -v jq >/dev/null 2>&1; then + echo "Error: jq not found." + exit 1 +fi
29-33: Quote uv_bin and fix version variable typo.Quote the command path; align variable names for clarity.
-installed_version=$(${uv_bin} 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=$("${uv_bin}" self version --output-format json | jq --raw-output ".version") +IFS=. read -r installed_version_major installed_version_minor _ <<<"${installed_version}" + +if (("${installed_version_major}" == "${required_version_major_min}" && \ "${installed_version_minor}" < "${required_version_minor_min}")); then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/clp-artifact-build.yaml(1 hunks).github/workflows/clp-core-build-macos.yaml(2 hunks)components/core/tools/scripts/lib_install/pipx-packages/find-pipx-bin.sh(1 hunks)components/core/tools/scripts/lib_install/pipx-packages/install-cmake.sh(2 hunks)components/core/tools/scripts/lib_install/pipx-packages/install-go-task.sh(2 hunks)components/core/tools/scripts/lib_install/pipx-packages/install-uv.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/pipx-packages/install-go-task.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/pipx-packages/install-cmake.sh
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
PR: y-scope/clp#1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.
Applied to files:
.github/workflows/clp-core-build-macos.yaml
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 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/pipx-packages/install-uv.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). (5)
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
.github/workflows/clp-core-build-macos.yaml (1)
12-12: LGTM: ensure pipx-packages changes trigger macOS builds.The added path filter looks correct and consistent with existing patterns.
Please confirm no other workflows rely on these scripts without similar path filters.
Also applies to: 27-27
.github/workflows/clp-artifact-build.yaml (1)
65-66: LGTM: include pipx-packages in image delta filters.Consistent additions across all image filters; matches intent to rebuild when pipx installers change.
Also applies to: 72-73, 79-80, 86-87
Bill-hbrhbr
changed the title
sudo before pipx installs to ensure user-scoped pipx packages; Allow pipx package changes to trigger artifact building CI.pipx-packages scripts:
Bill-hbrhbr
changed the title
pipx-packages scripts:pipx installs and reliable version checks for pipx packages.
There was a problem hiding this comment.
while getting the exact binary path cmake_bin from pipx's output is good for explicitness and guarantees the one installed by pipx is picked for execution, i think the approach is not portable and hard to maintain - see we need to update all our install-xxx.sh scripts and get exact binary paths for each binary installed for further setups.
instead, can we simply append the original user's local bin path before we need to use them?
e.g., we can
- create helper
components/core/tools/scripts/lib_install/pipx-packages/set-up-path.sh#!/usr/bin/env bash # Adds the pipx bin directory to PATH. When running via sudo, uses the original user's home # directory since pipx packages are installed there (see install-all.sh). if [[ -n "${SUDO_USER:-}" ]]; then user_home=$(getent passwd "${SUDO_USER}" | cut -d: -f6) export PATH="${user_home}/.local/bin:${PATH}" else export PATH="${HOME}/.local/bin:${PATH}" fi
- in
components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh:# Add pipx bin directory to PATH for source builds that need cmake source "${script_dir}/../pipx-packages/setup-path.sh" "${script_dir}/install-packages-from-source.sh"
- similarly, update the other
lib_install/xxx/install-all.shscripts
Bill-hbrhbr
changed the title
pipx installs and reliable version checks for pipx packages.
| # Prepend the pipx bin directory to PATH so pipx-installed build tools take precedence. | ||
| # Required by install-packages-from-source.sh, which installs tools into the pre-baked deps images. | ||
| # This can be removed once all library installs switch to go-task, which runs outside this script. | ||
| # The same rationale applies to all other platforms (e.g., centos9, manylinux, musllinux, macos), | ||
| # though the comment is omitted in those scripts. |
There was a problem hiding this comment.
would something like this be better placed in a dev doc?
There was a problem hiding this comment.
This is very technical so I think placing it here serves well for anyone who will be working on the code in our stead in the future.
But you've made a good point. I'll also write some documentation for the dev guide.
| pipx_bin_dir_cmd_args+=(pipx environment --value PIPX_BIN_DIR) | ||
|
|
||
| "${pipx_bin_dir_cmd_args[@]}" || { | ||
| echo "Failed to determine pipx bin directory via pipx environment (require version ≥ 1.7.0)." |
There was a problem hiding this comment.
errors should better be printed to stderr?
| echo "Failed to determine pipx bin directory via pipx environment (require version ≥ 1.7.0)." | |
| echo >&2 "Failed to determine pipx bin directory via pipx environment (require version ≥ 1.7.0)." |
There was a problem hiding this comment.
Yes, I was planning on change all error echo outputs to stderr in a separate PR.
| pipx install --force "cmake>=${required_version_min},<${required_version_major_max_plus_1}" | ||
| pipx ensurepath | ||
| fi | ||
| pipx uninstall cmake >/dev/null 2>&1 || true |
There was a problem hiding this comment.
why we want to reinstall cmake, even if it may be already at acceptable versions?
There was a problem hiding this comment.
The main motivation is simplicity. Previously, the script checked whether CMake was already installed and would fail partway through if the existing version didn’t meet our requirements.
With the current approach, we can safely install a known good CMake version via pipx regardless of what’s already present on the system. The pipx installation cleanly takes precedence and is also easy to uninstall if the user wants to revert to their original CMake.
I’ll add this to the dev docs so the intent is documented more clearly.
Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/src/dev-docs/components-core/ubuntu-jammy-deps-install.md`:
- Line 31: The [src-install-script] link currently points to the CentOS Stream 9
script; update the link target to the Ubuntu Jammy
install-packages-from-source.sh path so the documentation references the correct
script. Locate the [src-install-script] reference in
docs/src/dev-docs/components-core/ubuntu-jammy-deps-install.md and replace the
CentOS URL
(https://github.com/y-scope/clp/.../centos-stream-9/install-packages-from-source.sh)
with the corresponding Ubuntu Jammy script URL (the
install-packages-from-source.sh under the Ubuntu Jammy directory in the same
repo) so the [src-install-script] label resolves to the Ubuntu Jammy script.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/src/dev-docs/components-core/ubuntu-jammy-deps-install.md`:
- Around line 29-31: Update the two markdown link targets [pipx-install-script]
and [src-install-script] to use relative repository paths instead of the
absolute GitHub URLs; replace the full commit-pinned URLs with
repository-relative paths like
components/core/tools/scripts/lib_install/pipx-packages/install-all.sh and
components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
so the docs reference files in the same commit.
| [CMake]: https://cmake.org/ | ||
| [pipx-install-script]: https://github.com/y-scope/clp/blob/DOCS_VAR_CLP_GIT_REF/components/core/tools/scripts/lib_install/pipx-packages/install-all.sh | ||
| [src-install-script]: https://github.com/y-scope/clp/blob/DOCS_VAR_CLP_GIT_REF/components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh |
There was a problem hiding this comment.
Use relative links for file references in docs.
Both [pipx-install-script] and [src-install-script] point to individual files and should be relative paths so docs stay synced with the same commit.
✏️ Proposed fix
-[pipx-install-script]: https://github.com/y-scope/clp/blob/DOCS_VAR_CLP_GIT_REF/components/core/tools/scripts/lib_install/pipx-packages/install-all.sh
-[src-install-script]: https://github.com/y-scope/clp/blob/DOCS_VAR_CLP_GIT_REF/components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
+[pipx-install-script]: components/core/tools/scripts/lib_install/pipx-packages/install-all.sh
+[src-install-script]: components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.shBased on learnings, “In the CLP project documentation, when linking to scripts or other files within the repository, use relative paths (e.g., components/core/tools/scripts/...) rather than commit-pinned GitHub URLs to ensure docs and referenced files always belong to the same commit and stay synchronized.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [CMake]: https://cmake.org/ | |
| [pipx-install-script]: https://github.com/y-scope/clp/blob/DOCS_VAR_CLP_GIT_REF/components/core/tools/scripts/lib_install/pipx-packages/install-all.sh | |
| [src-install-script]: https://github.com/y-scope/clp/blob/DOCS_VAR_CLP_GIT_REF/components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh | |
| [CMake]: https://cmake.org/ | |
| [pipx-install-script]: components/core/tools/scripts/lib_install/pipx-packages/install-all.sh | |
| [src-install-script]: components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh |
🤖 Prompt for AI Agents
In `@docs/src/dev-docs/components-core/ubuntu-jammy-deps-install.md` around lines
29 - 31, Update the two markdown link targets [pipx-install-script] and
[src-install-script] to use relative repository paths instead of the absolute
GitHub URLs; replace the full commit-pinned URLs with repository-relative paths
like components/core/tools/scripts/lib_install/pipx-packages/install-all.sh and
components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
so the docs reference files in the same commit.
2 participants
Description
Background
The PR resolves the issue that tools installed via
pipxscripts (e.g.cmake) were not immediately available after their installation (#1360). This happens becausepipxmay install binaries into a directory that is not guaranteed to be on$PATHduring script runtime, causing subsequent steps to fail.$PATH handling
There are two reliable ways to make pipx installed tools available:
pipx ensurepath, which updates shell rc files to persistently add the pipx bin directory.PIPX_BIN_DIRfrompipx environmentand explicitly modifying PATH at runtime.While the first option is useful for persistent configuration, the second option is safer and more deterministic within scripts, since it guarantees immediate availability without relying on shell initialization behavior.
Pipx version
The
pipx environmentcommand requires pipx version 1.7 or newer. However, the pipx versions shipped by default on Ubuntu and CentOS are outdated (usually1.0.0). To support the safer $PATH handling approach (option 2), this PR also revamps howpipxitself is installed and upgraded to ensure a sufficiently recent version is always used.User mode installs for tool isolation
The scripts now consistently prefer user mode installations whenever possible. This ensures that
pipxmanaged tools are installed into the user environment, typically under the$HOME/.local/bindirectory, and avoids conflicts with system provided tools.This provides a level of isolation that makes tool installation, removal, updates, and version pinning cleaner and more predictable.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Chores
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.