fix: use sort -V for globals version comparison to prevent redundant installs - #1383
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR updates version comparison logic in npm and uv global package installers from exact string matching to semantic version comparison using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the version comparison logic in the npm and uv global installation scripts to use sort -V, ensuring that existing installations are skipped if the current version is greater than or equal to the required version. New test cases were added to verify these scenarios. The review feedback suggests optimizing the comparison using sort -VC to improve conciseness and avoid potential regex pitfalls with version strings.
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ]; then | ||
| min_ver=$(printf '%s\n%s\n' "$wanted_ver" "$installed_ver" | sort -V | head -n1) | ||
| if [ "$min_ver" = "$wanted_ver" ]; then | ||
| echo "$dep@$installed_ver already installed, skipping" | ||
| continue | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The version comparison logic can be simplified by using sort -VC, which checks if the input is already sorted according to version rules. This avoids the need for a subshell assignment and head, making the code more concise.
Note: Since sort -V is a GNU extension, please ensure that coreutils is explicitly included in the PATH within the home.activation block in default.nix. This prevents potential failures on systems where the default sort does not support the -V flag (e.g., macOS).
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ]; then | |
| min_ver=$(printf '%s\n%s\n' "$wanted_ver" "$installed_ver" | sort -V | head -n1) | |
| if [ "$min_ver" = "$wanted_ver" ]; then | |
| echo "$dep@$installed_ver already installed, skipping" | |
| continue | |
| fi | |
| fi | |
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ] && \ | |
| printf '%s\n%s\n' "$wanted_ver" "$installed_ver" | sort -VC; then | |
| echo "$dep@$installed_ver already installed, skipping" | |
| continue | |
| fi |
References
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions.
| if [ -n "$installed_version" ] && [ -n "$req_version" ]; then | ||
| if printf '%s\n%s\n' "$req_version" "$installed_version" | sort -V | head -n1 | grep -qx "$req_version"; then | ||
| echo "$name $installed_version already installed, skipping" | ||
| continue | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The use of grep -qx for version comparison is redundant and potentially problematic, as the . characters in version strings are interpreted as regex wildcards. Using sort -VC is a more robust and efficient way to verify that the installed version meets the requirement.
Note: Since sort -V is a GNU extension, please ensure that coreutils is explicitly included in the PATH within the home.activation block in default.nix. This prevents potential failures on systems where the default sort does not support the -V flag (e.g., macOS).
| if [ -n "$installed_version" ] && [ -n "$req_version" ]; then | |
| if printf '%s\n%s\n' "$req_version" "$installed_version" | sort -V | head -n1 | grep -qx "$req_version"; then | |
| echo "$name $installed_version already installed, skipping" | |
| continue | |
| fi | |
| fi | |
| if [ -n "$installed_version" ] && [ -n "$req_version" ] && \ | |
| printf '%s\n%s\n' "$req_version" "$installed_version" | sort -VC; then | |
| echo "$name $installed_version already installed, skipping" | |
| continue | |
| fi |
References
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the uv and npm global install scripts to skip installs when the currently installed version is newer than the required/wanted version, and adds tests around the new sort -V version comparison behavior.
Changes:
- Replace exact-version equality checks with
sort -V-based>=comparisons in uv/npm global install scripts - Add ShellSpec examples asserting presence and behavior of
sort -V-based version ordering
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| spec/uv_globals_spec.sh | Adds ShellSpec examples around sort -V version ordering and script implementation check |
| spec/npm_globals_spec.sh | Adds ShellSpec examples around sort -V version ordering and script implementation check |
| home-manager/modules/uv-globals/install-uv-globals.sh | Switches skip-logic to treat installed versions newer than required as satisfied via sort -V |
| home-manager/modules/npm-globals/install-npm-globals.sh | Switches skip-logic to treat installed versions newer than wanted as satisfied via sort -V |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ -n "$installed_version" ] && [ -n "$req_version" ]; then | ||
| if printf '%s\n%s\n' "$req_version" "$installed_version" | sort -V | head -n1 | grep -qx "$req_version"; then | ||
| echo "$name $installed_version already installed, skipping" | ||
| continue | ||
| fi |
There was a problem hiding this comment.
sort -V is a GNU extension and is not available with BSD sort (e.g., default macOS). If this script is expected to run on Darwin, this will fail at runtime. Consider pinning to a known GNU sort (e.g., via an explicit coreutils path in the Nix/home-manager context) or adding a fallback version-compare implementation when sort -V is unsupported.
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ]; then | ||
| min_ver=$(printf '%s\n%s\n' "$wanted_ver" "$installed_ver" | sort -V | head -n1) | ||
| if [ "$min_ver" = "$wanted_ver" ]; then | ||
| echo "$dep@$installed_ver already installed, skipping" | ||
| continue | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Same portability concern here: sort -V is not POSIX and will fail on platforms with BSD sort (notably macOS). If cross-platform execution is required, use a guaranteed GNU sort (or a repo-controlled tool) or implement a fallback comparator when sort -V isn’t supported.
| # Simulate: installed=0.15.9, required=0.15.8 -> should skip | ||
| When run bash -c "printf '0.15.8\n0.15.9\n' | sort -V | head -n1" | ||
| The output should eq '0.15.8' | ||
| End | ||
|
|
||
| It 'skips when installed version equals required' | ||
| # Simulate: installed=0.86.2, required=0.86.2 -> should skip | ||
| When run bash -c "printf '0.86.2\n0.86.2\n' | sort -V | head -n1" | ||
| The output should eq '0.86.2' | ||
| End | ||
|
|
||
| It 'detects when installed version is older than required' | ||
| # Simulate: installed=2.6.9, required=2.7.0 -> min is 2.6.9, not 2.7.0 | ||
| When run bash -c "printf '2.7.0\n2.6.9\n' | sort -V | head -n1" | ||
| The output should eq '2.6.9' |
There was a problem hiding this comment.
These examples validate sort -V behavior in isolation, but they don’t exercise the script’s actual skip/update branching (they would still pass even if the script’s condition is incorrect, as long as sort -V exists). Consider adding tests that run the install script with a mocked/controlled INSTALLED + required version input and assert it prints already installed, skipping for newer/equal installed versions and performs the update path for older versions.
| # Simulate: installed=0.15.9, required=0.15.8 -> should skip | |
| When run bash -c "printf '0.15.8\n0.15.9\n' | sort -V | head -n1" | |
| The output should eq '0.15.8' | |
| End | |
| It 'skips when installed version equals required' | |
| # Simulate: installed=0.86.2, required=0.86.2 -> should skip | |
| When run bash -c "printf '0.86.2\n0.86.2\n' | sort -V | head -n1" | |
| The output should eq '0.86.2' | |
| End | |
| It 'detects when installed version is older than required' | |
| # Simulate: installed=2.6.9, required=2.7.0 -> min is 2.6.9, not 2.7.0 | |
| When run bash -c "printf '2.7.0\n2.6.9\n' | sort -V | head -n1" | |
| The output should eq '2.6.9' | |
| # Simulate the install script branch: installed=0.15.9, required=0.15.8 -> should skip | |
| When run bash -c ' | |
| INSTALLED=0.15.9 | |
| REQUIRED=0.15.8 | |
| if [ "$INSTALLED" != "$REQUIRED" ] && [ "$(printf "%s\n%s\n" "$REQUIRED" "$INSTALLED" | sort -V | head -n1)" = "$INSTALLED" ]; then | |
| echo "uv tool install" | |
| else | |
| echo "already installed, skipping" | |
| fi | |
| ' | |
| The output should eq 'already installed, skipping' | |
| End | |
| It 'skips when installed version equals required' | |
| # Simulate the install script branch: installed=0.86.2, required=0.86.2 -> should skip | |
| When run bash -c ' | |
| INSTALLED=0.86.2 | |
| REQUIRED=0.86.2 | |
| if [ "$INSTALLED" != "$REQUIRED" ] && [ "$(printf "%s\n%s\n" "$REQUIRED" "$INSTALLED" | sort -V | head -n1)" = "$INSTALLED" ]; then | |
| echo "uv tool install" | |
| else | |
| echo "already installed, skipping" | |
| fi | |
| ' | |
| The output should eq 'already installed, skipping' | |
| End | |
| It 'updates when installed version is older than required' | |
| # Simulate the install script branch: installed=2.6.9, required=2.7.0 -> should update | |
| When run bash -c ' | |
| INSTALLED=2.6.9 | |
| REQUIRED=2.7.0 | |
| if [ "$INSTALLED" != "$REQUIRED" ] && [ "$(printf "%s\n%s\n" "$REQUIRED" "$INSTALLED" | sort -V | head -n1)" = "$INSTALLED" ]; then | |
| echo "uv tool install" | |
| else | |
| echo "already installed, skipping" | |
| fi | |
| ' | |
| The output should eq 'uv tool install' |
Mesa DescriptionTL;DRFixed redundant global package installations by changing version comparison in What changed?
Description generated by Mesa. Update settings |
Summary
=) to skip already-installed packages>=0.15.8required) were reinstalled every runsort -Vbased>=comparison so newer installed versions correctly skipTest plan
shellspec spec/uv_globals_spec.sh- 25 examples, 0 failuresshellspec spec/npm_globals_spec.sh- 31 examples, 0 failuresSummary by cubic
Prevent redundant installs of global tools by switching version checks in the
uvandnpmglobals installers to asort -V>= comparison. Newer or equal installed versions now skip; added shellspec tests for newer/equal/older cases.Written for commit 22b843a. Summary will update on new commits.