fix(install): defer rknpu auto-install until after repo clone + add wheel-build deps (closes redkjuegos #362)#405
Conversation
…heel-build deps User reported (discussion #362) two related issues on a fresh Pi install: 1. install-server.sh detects the RKNPU and tries to auto-install rkllama, but the install-rknpu.sh script lives inside the repo that hasn't been cloned yet at that point in the flow. Result: warns 'install-rknpu.sh not found locally yet' and skips. User then runs the script manually later, which is exactly what the auto path was meant to avoid. Fix: split the rknpu logic into detection (early, sets a deferred flag) and install (after `git clone $INSTALL_DIR`). detect_and_advise_ accelerators sets RKNPU_PENDING_INSTALL=1; install_rknpu_if_pending runs after the cd into the cloned repo and chains into the now-present install-rknpu.sh. 2. install-rknpu.sh's rkllama venv install fails on a clean Pi with 'Failed building wheel for webrtcvad' because rkllama's transitive deps need a C toolchain (gcc + python3-dev + libffi-dev) and Pi images often ship without these. Fix: install-rknpu.sh now apts the build deps (only the missing ones) right before the venv pip install. Failures are warn-only since the pip install will surface a clearer error if the actual issue was something else. Both reported on discussion #362.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR restructures the RKNPU backend installation from immediate chaining during hardware detection to deferred execution after repository clone. Detection sets a flag; a new handler function executes the installation post-clone with error resilience. Build tool prerequisites are installed before pip compilation to support native wheel builds. ChangesRKNPU Backend Installation Deferral
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by grok-code-fast-1:optimized:free · 178,697 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/install-server.sh (1)
344-347: ⚡ Quick winAvoid
-xgate here; require file presence/readability instead.You execute via
bash "$rknpu_script", so requiring executable bit can incorrectly skip a valid script. Also, the current warning says “not found” even when it exists but is non-executable.Proposed patch
- if [[ ! -x "$rknpu_script" ]]; then - warn "install-rknpu.sh not found at $rknpu_script — skipping rkllama auto-install" + if [[ ! -f "$rknpu_script" || ! -r "$rknpu_script" ]]; then + warn "install-rknpu.sh missing/unreadable at $rknpu_script — skipping rkllama auto-install" warn " to set up rkllama later: sudo bash $INSTALL_DIR/scripts/install-rknpu.sh" return 0 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-server.sh` around lines 344 - 347, The guard using [[ -x "$rknpu_script" ]] is incorrect because the script is invoked with bash and may be readable but not executable; change the check to test for presence/readability (e.g., [[ -r "$rknpu_script" ]] or [[ -f "$rknpu_script" ]]) and update the warn message so it doesn't claim "not found" when the file exists but lacks exec bits; target the rknpu_script check and the warning text that references install-rknpu.sh and INSTALL_DIR to ensure we only skip when the file is truly absent/unreadable and the message accurately describes the reason.scripts/install-rknpu.sh (1)
330-338: ⚡ Quick winAdd a lightweight
apt-get updatebefore installing missing build deps.On fresh/minimal images,
apt-get installcan fail with outdated or missing package indexes even when packages are valid. Running a quietapt-get updateright before this install makes the new dependency step much more reliable.Proposed patch
if command -v apt-get >/dev/null 2>&1; then local _need=() command -v gcc >/dev/null 2>&1 || _need+=("build-essential") dpkg-query -W python3-dev >/dev/null 2>&1 || _need+=("python3-dev") dpkg-query -W libffi-dev >/dev/null 2>&1 || _need+=("libffi-dev") if (( ${`#_need`[@]} )); then log "installing build deps for rkllama wheel compilation: ${_need[*]}" + sudo DEBIAN_FRONTEND=noninteractive apt-get update -qq \ + || warn "apt-get update failed — package install may fail" sudo DEBIAN_FRONTEND=noninteractive apt-get install -y -qq "${_need[@]}" \ || warn "apt-get install of build deps failed — wheel builds may still fail" fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-rknpu.sh` around lines 330 - 338, Add a quiet apt-get update right before installing build deps when using apt-get: inside the apt-get branch, after you determine _need is non-empty (the block that logs "installing build deps for rkllama wheel compilation: ${_need[*]}"), run a sudo DEBIAN_FRONTEND=noninteractive apt-get update -qq (or similar) before the existing sudo DEBIAN_FRONTEND=noninteractive apt-get install -y -qq "${_need[@]}" so installs won't fail on fresh/minimal images; keep the update scoped to run only when _need has items and keep the existing warn fallback for install failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/install-rknpu.sh`:
- Around line 330-338: Add a quiet apt-get update right before installing build
deps when using apt-get: inside the apt-get branch, after you determine _need is
non-empty (the block that logs "installing build deps for rkllama wheel
compilation: ${_need[*]}"), run a sudo DEBIAN_FRONTEND=noninteractive apt-get
update -qq (or similar) before the existing sudo DEBIAN_FRONTEND=noninteractive
apt-get install -y -qq "${_need[@]}" so installs won't fail on fresh/minimal
images; keep the update scoped to run only when _need has items and keep the
existing warn fallback for install failures.
In `@scripts/install-server.sh`:
- Around line 344-347: The guard using [[ -x "$rknpu_script" ]] is incorrect
because the script is invoked with bash and may be readable but not executable;
change the check to test for presence/readability (e.g., [[ -r "$rknpu_script"
]] or [[ -f "$rknpu_script" ]]) and update the warn message so it doesn't claim
"not found" when the file exists but lacks exec bits; target the rknpu_script
check and the warning text that references install-rknpu.sh and INSTALL_DIR to
ensure we only skip when the file is truly absent/unreadable and the message
accurately describes the reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7fe4bb6-5d01-434a-ac62-948a61f76df3
📒 Files selected for processing (2)
scripts/install-rknpu.shscripts/install-server.sh
Two follow-ups while testing PR #405 on the Pi: 1. Cold boot on Pi 5 / Orange Pi 5 lands around 55-65s (issue #337) so the existing 60s timeout was racing the actual ready state and printing a false 'controller did not respond' warning even on successful installs. Validated the new install ran fine then timed out. 120s keeps the safety net while removing the false alarm. 2. CR nit on the install_rknpu_if_pending guard: -x requires the executable bit but we invoke via `bash $rknpu_script` which only needs readable. Switched to -f && -r and updated the warning.
1 participant
Summary
User redkjuegos reported on discussion #362 that the install-server.sh "auto-install rkllama" path doesn't actually work on a fresh Pi. Two related bugs:
1. Ordering: install-rknpu.sh referenced before the repo is cloned
`detect_and_advise_accelerators` runs at the top of install-server.sh and tries to `bash $INSTALL_DIR/scripts/install-rknpu.sh` — but $INSTALL_DIR doesn't exist yet because git clone happens later in the script. The user sees:
```
[server-install] Rockchip NPU detected — auto-installing the jaylfc/rkllama backend
[server-install] install-rknpu.sh not found locally yet — it will be after the repo is cloned
```
So the auto-path always fails on the first run.
Fix: split detection and install. `detect_and_advise_accelerators` now sets a `RKNPU_PENDING_INSTALL=1` flag instead of trying to install. After the `git clone`, a new `install_rknpu_if_pending` chains into the now-present script.
2. webrtcvad wheel build fails on Pi
```
ERROR: Failed building wheel for webrtcvad
```
webrtcvad is a transitive rkllama dep that compiles from source. Pi images often ship without `gcc` / `python3-dev` / `libffi-dev`.
Fix: install-rknpu.sh apts the missing build deps right before the venv pip install. Only installs what's missing.
Test plan
Closes redkjuegos's findings #1 + #3 from discussion #362. Issues #2 (librknnrt empty version on first run) and #4 (Store install button no feedback) tracked separately.
Summary by CodeRabbit