fix: venv usage #314
fix: venv usage #314
Conversation
enable verbosity in uv output
dependent on ACTIONS_STEP_DEBUG env var or `inputs.verbosity`
Cpp-Linter Report
|
|
I'm also noticing that the ignore: buildThis ends up running clang tools on files nested in the build/ directory. See bot comment above. I suspect this is related to cpp-linter (py) implementation of |
instead of `curl` on unix or `irm ... iex` on windows because `curl` may not be available
2bndy5
force-pushed
the
fix-venv
branch
5 times, most recently
from
7e10ff1 to
2d780d8
Compare
github-actions
bot
added
the
documentation
|
BTW, Ubuntu 25.10 is transitioning to use rust implementations of And, nushell is already wrapping some of the functions from uutils/coreutils. |
for each platform
WalkthroughReplaces Bash-based setup with Nu-based, cross-platform flows in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner
participant Action
participant OS_PkgMgr as OS Package Manager
participant LLVM_Installer as LLVM Installer
participant Homebrew as Homebrew
participant UV as uv
participant CppLinter as cpp-linter
Runner->>Action: Start action.yml
Action->>Action: Parse inputs, verbosity, cache settings
alt Linux
Action->>OS_PkgMgr: apt-get install clang-format/clang-tidy
alt Tools missing
Action->>LLVM_Installer: Download & run LLVM installer
Action->>OS_PkgMgr: Retry apt-get install
end
else macOS
Action->>Homebrew: brew install llvm@<version>
Action->>Action: Create symlinks for clang-format/tidy
else Windows
Action->>Action: Use provided static binaries
end
Action->>UV: Fetch and run platform-specific installer (install.sh / install.ps1)
Action->>UV: uv run --no-sync --project <action> --directory (pwd) cpp-linter <args>
UV->>CppLinter: Execute cpp-linter
CppLinter-->>Runner: Results / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
README.md (3)
161-168: Tighten phrasing and fix minor grammar around PATH behaviorThe sentences read a bit clunky with the semicolon and “now uses” construction. Suggested edits improve clarity and flow.
-As of v2.16.0, this action now uses +As of v2.16.0, this action uses -This action installs [nushell] and [uv] automatically, but only [nushell] is added to the PATH environment variable; -[uv], and any standalone Python distribution it downloads, are not added to the PATH environment variable. +This action installs [nushell] and [uv] automatically. Only [nushell] is added to the PATH environment variable. +[uv], and any standalone Python distribution it downloads, are not added to the PATH environment variable.
171-186: Subject-verb agreement and stronger guidance for Debian-based containers
- Use “are installed” for the list.
- Consider including an apt-get update in the example, as many minimal images need it before install.
-We only support Linux runners using a Debian-based Linux OS (like Ubuntu and many others). -This is because we first try to use the `apt` package manager to install clang tools. +We only support Linux runners using a Debian-based Linux OS (like Ubuntu and many others). +This is because we first try to use the `apt` package manager to install clang tools. -Linux workflows that use a specific [`container`][gh-container-syntax] should ensure that -the following is installed: +Linux workflows that use a specific [`container`][gh-container-syntax] should ensure that +the following are installed: -```shell -apt-get install -y libc6 wget lsb-release software-properties-common gnupg -``` +```shell +apt-get update && apt-get install -y libc6 wget curl lsb-release software-properties-common gnupg +```
189-199: Use “macOS” capitalization and align with Apple’s styleThis is purely cosmetic, but standardizes terminology across docs.
-### On MacOS runners +### On macOS runnersaction.yml (2)
324-333: macOS symlink creation may fail or be non-idempotent
- Use
-sfto replace existing symlinks on reruns.- On some environments,
/usr/local/binmay require elevated permissions. GitHub macOS runners typically allow it, but being idempotent helps.- ^ln -s $"($brew_prefix)/bin/clang-format" "/usr/local/bin/clang-format-${{ inputs.version }}" - ^ln -s $"($brew_prefix)/bin/clang-tidy" "/usr/local/bin/clang-tidy-${{ inputs.version }}" + ^ln -sf $"($brew_prefix)/bin/clang-format" "/usr/local/bin/clang-format-${{ inputs.version }}" + ^ln -sf $"($brew_prefix)/bin/clang-tidy" "/usr/local/bin/clang-tidy-${{ inputs.version }}"
430-438: Minor: simplify PATH augmentation and remove unused variable
tmp_pathis unused; directly mutate PATH within the conditional.- let local_bin = '~/.local/bin' | path expand - let tmp_path = if ( + let local_bin = '~/.local/bin' | path expand + if ( ('${{ runner.os }}' == 'Linux') and ($local_bin | path exists) and (not ($env.PATH | any {$in == $local_bin})) ) { # add ~/.local/bin to PATH (temporarily) $env.PATH = $env.PATH | append $local_bin }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(2 hunks)action.yml(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~163-~163: There might be a mistake here.
Context: ... for cross-platform compatible scripting - [uv] for driving a Python virtual environ...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ... added to the PATH environment variable; [uv], and any standalone Python distribu...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...[gh-container-syntax] should ensure that the following is installed: - GLIBC (v2...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (ubuntu-latest, 19)
- GitHub Check: test (macos-latest, 16)
- GitHub Check: test (macos-latest, 15)
- GitHub Check: test (macos-latest, 11)
- GitHub Check: test (macos-latest, 8)
- GitHub Check: test (macos-latest, 14)
🔇 Additional comments (6)
README.md (2)
36-38: Good clarification via anchored warningPointing to a concrete “Required tools” section is clearer than the previous generic warning. This improves discoverability for container users.
204-209: Link references look goodThe added references are accurate and helpful.
action.yml (4)
220-227: Cache description improvements are clearGood callout that clang tools themselves aren’t cached and where assets live. No issues.
372-385: LGTM on uv args construction and clang-tools invocationThe
--project+--directory (pwd)workaround addresses uv’s project environment edge cases. Verbosity handling is clean.
249-271: Cache key derivation and storage path look goodUsing uv.lock hash or action.yml hash is a sensible cache key. Storing in runner.temp avoids polluting workspace.
273-321: Request quick validation on Linux/macOS runners after these Nu changesGiven the shell migration and the critical Windows uv fix above, a quick smoke run across platforms will guard against regressions (apt script/save semantics, brew symlinks, uv flags, PATH augmentation).
I can generate a matrix workflow snippet you can paste into a temporary branch to validate Linux/macOS end-to-end if you’d like. -->
Also applies to: 324-333, 339-361, 372-385, 387-442
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
action.yml (1)
395-416: About passing empty --extra-arg: acceptable per downstream parserYou previously noted that your downstream parser in cpp-linter/cpp-linter tolerates
--extra-arg="". Keeping it as-is is fine; conditional appending can be explored later if desired.
🧹 Nitpick comments (4)
README.md (2)
159-169: Tighten phrasing; minor grammar tweaks for clarityA few wording improvements make this section scan better and avoid hyphenation ambiguity.
Apply this diff:
-## Required tools installed +## Required tools installed @@ -As of v2.16.0, this action uses +As of v2.16.0, this action uses: @@ -- [nushell] for cross-platform compatible scripting -- [uv] for driving a Python virtual environment +- [nushell] for cross-platform scripting +- [uv] to manage a Python virtual environment @@ -This action installs [nushell] and [uv] automatically. -Only [nushell] is added to the PATH environment variable. -[uv], and any standalone Python distribution it downloads, are not added to the PATH environment variable. +This action installs both [nushell] and [uv] automatically. +Only [nushell] is added to PATH. +[uv], and any standalone Python distribution it downloads, are not added to PATH.
171-189: Linux prerequisites: include CA certs and make wget/curl guidance explicitLLVM’s installer fetches over HTTPS and GPG-validates keys; CA bundles are often missing in minimal images. Also, the bulleted list says “wget or curl” but the command installs only wget. Consider installing both or making the choice explicit.
Proposed diff:
Linux workflows that use a specific [container][gh-container-syntax] should ensure that the following are installed: @@ -- GLIBC (v2.32 or later) -- `wget` or `curl` +- GLIBC (v2.32 or later) +- `wget` or `curl` (at least one) - `lsb-release` (required by LLVM-provided install script) - `software-properties-common` (required by LLVM-provided install script) - `gnupg` (required by LLVM-provided install script) +- `ca-certificates` (required for HTTPS downloads) @@ -apt-get update -apt-get install -y libc6 wget lsb-release software-properties-common gnupg +# choose either wget or curl; installing both is also fine +apt-get update +apt-get install -y libc6 wget curl lsb-release software-properties-common gnupg ca-certificatesaction.yml (2)
273-321: Linux: minor nits + optional hardening when fetching llvm.sh
- There’s a harmless double space before
...$apt_install_argson Line 290.- Optional: consider
save --forceor removing the file before saving for idempotency across re-runs (even though action path usually starts clean).- Optional: when executing remote installers, you could pin or checksum the script for extra supply-chain safety.
Apply this diff to fix the spacing nit and make save idempotent:
- ^apt-get ...$apt_install_args + ^apt-get ...$apt_install_args @@ - ( - http get --raw --redirect-mode follow https://apt.llvm.org/llvm.sh - | save $"($action_path)/llvm_install.sh" - ) + ( + http get --raw --redirect-mode follow https://apt.llvm.org/llvm.sh + | save --force $"($action_path)/llvm_install.sh" + )If you want checksum verification, I can draft a Nu snippet that computes and validates a known SHA256 before execution.
349-361: Windows uv installer: add -NoProfile to avoid profile side-effectsPiping to PowerShell with
-Command -is correct. Add-NoProfileto prevent user profiles from mutating the session.Apply this diff:
- if $is_windows { - $installer | ^powershell -NonInteractive -ExecutionPolicy ByPass -Command - + if $is_windows { + $installer | ^powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command - } else { $installer | ^sh }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(2 hunks)action.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-17T12:34:25.438Z
Learnt from: 2bndy5
PR: cpp-linter/cpp-linter-action#314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.438Z
Learning: The cpp-linter/cpp-linter-action repository manages the downstream parsing of command-line arguments through their own cpp-linter/cpp-linter project, so passing empty string values for flags like --extra-arg="" is acceptable and known to work in their implementation.
Applied to files:
action.yml
📚 Learning: 2025-08-17T12:31:25.296Z
Learnt from: 2bndy5
PR: cpp-linter/cpp-linter-action#314
File: action.yml:273-321
Timestamp: 2025-08-17T12:31:25.296Z
Learning: In Nu shell, the pattern `(...) | complete | $in.exit_code == 0` is valid syntax for checking command exit codes, where `complete` captures the command result and `$in` refers to the piped input.
Applied to files:
action.yml
📚 Learning: 2025-08-17T12:50:57.018Z
Learnt from: 2bndy5
PR: cpp-linter/cpp-linter-action#314
File: action.yml:273-321
Timestamp: 2025-08-17T12:50:57.018Z
Learning: In Nu shell, `$in` is shorthand for a closure that takes 1 argument, so `(...) | complete | $in.exit_code == 0` is equivalent to `(...) | complete | {|in| $in.exit_code == 0}`.
Applied to files:
action.yml
📚 Learning: 2025-08-17T12:34:25.438Z
Learnt from: 2bndy5
PR: cpp-linter/cpp-linter-action#314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.438Z
Learning: In the cpp-linter-action project, technical improvements can be deferred to separate PRs when they're not critical to the current changes, especially when the current implementation is known to work correctly.
Applied to files:
action.yml
📚 Learning: 2025-08-17T12:50:57.018Z
Learnt from: 2bndy5
PR: cpp-linter/cpp-linter-action#314
File: action.yml:273-321
Timestamp: 2025-08-17T12:50:57.018Z
Learning: In Nu shell, the `.` operator on records is equivalent to `| get`, so `$rec.exit_code` is the same as `$rec | get 'exit_code'`.
Applied to files:
action.yml
🪛 LanguageTool
README.md
[grammar] ~163-~163: There might be a mistake here.
Context: ... for cross-platform compatible scripting - [uv] for driving a Python virtual environ...
(QB_NEW_EN)
[grammar] ~175-~175: There might be a mistake here.
Context: ...[gh-container-syntax] should ensure that the following are installed: - GLIBC (v...
(QB_NEW_EN)
⏰ 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). (16)
- GitHub Check: test (windows-latest, 15)
- GitHub Check: test (windows-latest, 17)
- GitHub Check: test (windows-latest, 14)
- GitHub Check: test (windows-latest, 18)
- GitHub Check: test (windows-latest, 12)
- GitHub Check: test (macos-latest, 17)
- GitHub Check: test (windows-latest, 10)
- GitHub Check: test (macos-latest, 12)
- GitHub Check: test (windows-latest, 8)
- GitHub Check: test (ubuntu-latest, 18)
- GitHub Check: test (macos-latest, 14)
- GitHub Check: test (ubuntu-latest, 15)
- GitHub Check: test (ubuntu-latest, 12)
- GitHub Check: test (macos-latest, 19)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (ubuntu-latest, 19)
🔇 Additional comments (9)
README.md (2)
37-37: Good change: warning now points to concrete requirementsLinking to the new “Required tools installed” section is clearer and more actionable than the previous generic warning.
191-201: macOS/Windows notes align with installer flowThe documented fallback to static binaries matches the action’s later uv-driven clang-tools ensure step. Looks consistent.
action.yml (7)
220-227: Cache description reads well and sets correct expectationsClear that tool binaries aren’t cached and where cache lives. No issues found.
324-333: macOS: symlink approach is fine; consider Homebrew prefix varianceOn Apple Silicon, Homebrew’s prefix is commonly
/opt/homebrew; you’re already deriving it viabrew --prefix llvm@<ver>, which is good. The hard-coded/usr/local/binsymlink target generally works on GitHub macOS runners; if self-hosted runners are in scope, note that write permissions may vary.If you support self-hosted macOS runners, confirm
/usr/local/binis writable; otherwise, consider symlinking into the detected Homebrew bin dir or adjusting PATH instead.
372-377: uv sync flow is clean and respects verbosityUsing
--group actionwith conditional-vis tidy and debuggable. Looks good.
379-385: Ensuring clang-tools via uv is aligned with README fallbackThis provides the promised cross-platform fallback regardless of OS-specific package managers. LGTM.
417-429: Verbosity gating is consistent with earlier stepsReusing the same detection for ACTIONS_STEP_DEBUG and user input keeps behavior predictable. No issues.
430-438: Linux PATH augmentation for ~/.local/bin is a nice touchTemporary PATH extension avoids surprises when user-installed tools live under
~/.local/bin. Looks correct and safe.
441-441: Final uv invocation is correct and mirrors setup pattern
uv run --no-sync --project <action_path> --directory (pwd)with args expansion is consistent and robust across shells. LGTM.
The drawback of using nushell is that it requires some learning and pre-installation (since GitHub Actions doesn’t ship it by default, unlike other shells or Python). I’ve been wondering whether it’s worth adopting, especially given that PowerShell and Python are also available on Windows, Linux, and macOS runners. After trying some commands, nushell seems quite interesting—which might be the most important. |
I've been using nushell on all my machines for almost 2 years now. The more I use the nushell syntax, the more I love it. Some concepts were borrowed from bash (commands like There's a task runner out there called nur (nushell runner) which ships with nushell builtin. So, authors don't have to worry about what OS/shell another team member might use. |
|
I don't know PowerShell either. : )
It's true. Good to know you’re familiar with Nushell. I tried it a bit myself, and it’s interesting and I have no concerns about it now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
README.md (7)
156-166: Polish wording and quote PATH for consistency/readabilityTighten phrasing and standardize PATH formatting.
-As of v2.16.0, this action uses - -- [nushell] for cross-platform compatible scripting -- [uv] for driving a Python virtual environment +As of v2.16.0, this action uses: + +- [nushell] for cross-platform scripting +- [uv] for managing Python virtual environments @@ -This action installs [nushell] and [uv] automatically. -Only [nushell] is added to the PATH environment variable. -[uv], and any standalone Python distribution it downloads, are not added to the PATH environment variable. +This action installs [nushell] and [uv] automatically. +Only [nushell] is added to the `PATH`. +[uv], and any standalone Python distribution it downloads, are not added to the `PATH`.
169-171: Clarify Linux support scopeMake the constraint and rationale concise.
-We only support Linux runners using a Debian-based Linux OS (like Ubuntu and many others). -This is because we first try to use the `apt` package manager to install clang tools. +We currently support Debian-based Linux runners (e.g., Ubuntu) because the action installs clang tools via `apt`.
172-180: Name what’s being installed and include ca-certificates (often required)Explicitly say “packages” and consider adding ca-certificates which is commonly needed for HTTPS downloads/repos.
-Linux workflows that use a specific [`container`][gh-container-syntax] should ensure that -the following are installed: +Linux workflows that use a specific [`container`][gh-container-syntax] should ensure that +the following packages are installed: @@ - `software-properties-common` (required by LLVM-provided install script) - `gnupg` (required by LLVM-provided install script) +- `ca-certificates` (required for HTTPS downloads and APT repositories)
181-184: APT example: add ca-certificates and avoid pulling recommendsKeeps container images leaner and reduces surprises.
apt-get update -apt-get install -y libc6 wget lsb-release software-properties-common gnupg +apt-get install -y --no-install-recommends libc6 wget lsb-release software-properties-common gnupg ca-certificates
186-186: Tighten wordingMinor phrasing improvement.
-Otherwise, [nushell] and/or the LLVM-provided bash script will fail to run. +Otherwise, [nushell] and/or the LLVM-provided install script will fail to run.
190-193: Minor style: “details” vs “detail”Nit on phrasing.
-see [cpp-linter/clang-tools-pip] and [cpp-linter/clang-tools-static-binaries] projects for more detail. +see [cpp-linter/clang-tools-pip] and [cpp-linter/clang-tools-static-binaries] projects for more details.
196-198: Minor style: mirror macOS phrasingSame nit for consistency.
-See [cpp-linter/clang-tools-pip] and [cpp-linter/clang-tools-static-binaries] projects for more detail. +See [cpp-linter/clang-tools-pip] and [cpp-linter/clang-tools-static-binaries] projects for more details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-17T13:20:51.010Z
Learnt from: 2bndy5
PR: cpp-linter/cpp-linter-action#314
File: README.md:206-211
Timestamp: 2025-08-17T13:20:51.010Z
Learning: For GitHub Actions documentation references in cpp-linter-action, prefer stable workflow syntax documentation links over "canonical" specialized pages that may redirect, as the comprehensive workflow syntax page provides better context and examples for container configuration.
Applied to files:
README.md
🪛 LanguageTool
README.md
[grammar] ~160-~160: There might be a mistake here.
Context: ... for cross-platform compatible scripting - [uv] for driving a Python virtual environ...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...[gh-container-syntax] should ensure that the following are installed: - GLIBC (v...
(QB_NEW_EN)
⏰ 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). (18)
- GitHub Check: test (windows-latest, 10)
- GitHub Check: test (ubuntu-latest, 15)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (ubuntu-latest, 19)
- GitHub Check: test (windows-latest, 13)
- GitHub Check: test (macos-latest, 14)
- GitHub Check: test (ubuntu-latest, 14)
- GitHub Check: test (windows-latest, 19)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (windows-latest, 17)
- GitHub Check: test (windows-latest, 16)
- GitHub Check: test (windows-latest, 12)
- GitHub Check: test (windows-latest, 8)
- GitHub Check: test (macos-latest, 18)
- GitHub Check: test (windows-latest, 11)
- GitHub Check: test (macos-latest, 13)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 9)
🔇 Additional comments (1)
README.md (1)
203-208: LGTM on link references (keeps workflow syntax doc as preferred target)References look good and align with the preference to use the comprehensive workflow syntax page for container configuration.
|
The default immutability of variables in nushell is the only feature that causes me trouble. But that just makes me rethink my approach, in which case the script ends up being more robust anyway. |
|
I've also made some important changes to the test repo (test-uv branch). I'll submit a PR for that repo soon. |
related to #305
resolves #313
Python venv fix
Apparently,
uvrespects theUV_PROJECT_ENVIRONMENTenv var for only certainuvcommands.This workaround involves a combination of
--projectand--directory:Where
(pwd)is the nushell builtin command similar to bash's$(pwd)and should point to the working directory.Tested in a container
I ran this in the test repo using the
ubuntu:latestdocker image (for Linux runners only).While it works now (after switching from bash to nushell), I found that the LLVM script requires certain tools that are not present in docker images.
So after a lot of trial and error, I revised the README to describe the required tools for each platform. It turns out nushell's builtin
httpcommand usescurlorwgetunderneath (on Linux).sudois no longer required if the docker container runs as root. 🎉Summary by CodeRabbit
Documentation
cache-enabledescription to explain caching behavior, defaults, and cache location.Refactor
Chores