fix(installer): rknpu version check SIGPIPEs strings, aborting the first clean install (#1560)#1561
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe librknnrt version-check logic in ChangesInstaller version-check fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 The previous review's outstanding suggestion at Files Reviewed (2 files)
Previous Review Summaries (4 snapshots, latest commit 62f8775)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 62f8775)Status: 1 Issue Found | Recommendation: Address before merge (minor) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit aa6d532)Status: No Issues Found | Recommendation: Merge The previous review's outstanding suggestion (the Files Reviewed (2 files)
Previous review (commit bc57d76)Status: 1 Issue Found | Recommendation: Address before merge (minor) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 744e1eb)Status: 1 Issue Found | Recommendation: Address before merge (minor) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 47.6K · Output: 2.7K · Cached: 439.7K |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # #1543). The second run only worked because librknnrt was already at | ||
| # 2.3.0 and this whole pin block was skipped. Read to EOF (no early | ||
| # exit) and guard the assignment so a strings/awk hiccup is never fatal. | ||
| verified="$(strings "$tmp" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }')" || true |
There was a problem hiding this comment.
SUGGESTION: The || true on this command substitution combined with 2>/dev/null on strings means a genuine strings/awk failure on the downloaded runtime is now indistinguishable from a missing version string. The fallback at line 353 reads the same binary content from $LIBRKNNRT_DEST, so if strings truly can't read this file it'll likely fail there too, and the only operator-visible signal becomes the "skipping version-string re-check" log — which previously meant "binutils missing" and now also means "strings/awk failed silently". Consider logging a warn when the primary substitution returns empty so future first-run failures aren't masked during diagnostics.
| verified="$(strings "$tmp" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }')" || true | |
| if verified_output="$(strings "$tmp" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }')"; then | |
| verified="$verified_output" | |
| else | |
| warn "strings/awk version probe failed on $tmp; falling back to installed path" | |
| fi |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install-rknpu.sh (1)
105-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating the version-parsing awk pattern.
The same
strings | awk '/librknnrt version: / && !seen {...}'logic now exists in two places. Since this exact duplication is what let the original early-exit bug slip into only one of the two nearly-identical pipelines historically, extracting a small shared helper (e.g.,_librknnrt_parse_version <file>) would reduce the chance of future fixes only being applied to one site.♻️ Example consolidation
+# Reads librknnrt version string from a binary via `strings`. Reads to EOF +# (no early awk exit) to avoid SIGPIPE under `set -o pipefail`. +_librknnrt_parse_version() { + strings "$1" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }' +} + librknnrt_current_version() { if [[ -f "$LIBRKNNRT_DEST" ]] && command -v strings >/dev/null 2>&1; then - strings "$LIBRKNNRT_DEST" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }' + _librknnrt_parse_version "$LIBRKNNRT_DEST" fi }- verified="$(strings "$tmp" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }')" || true + verified="$(_librknnrt_parse_version "$tmp")" || trueAlso applies to: 350-350
🤖 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` at line 105, The version-parsing logic in the install script is duplicated, so update the two `strings | awk '/librknnrt version: / && !seen { ... }'` call sites to use a shared helper such as `_librknnrt_parse_version`. Keep the helper focused on extracting the first `librknnrt version` match from a given file, and have both existing version checks call it instead of repeating the awk pipeline directly.
🤖 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`:
- Line 105: The version-parsing logic in the install script is duplicated, so
update the two `strings | awk '/librknnrt version: / && !seen { ... }'` call
sites to use a shared helper such as `_librknnrt_parse_version`. Keep the helper
focused on extracting the first `librknnrt version` match from a given file, and
have both existing version checks call it instead of repeating the awk pipeline
directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71df4a6a-a9b8-4066-b9ce-2e16774f7c08
📒 Files selected for processing (2)
CHANGELOG.mdscripts/install-rknpu.sh
| if verified="$(strings "$tmp" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }')"; then | ||
| : | ||
| else | ||
| warn "strings/awk version probe failed on the downloaded runtime; falling back to the installed path" |
There was a problem hiding this comment.
SUGGESTION: The warn here is a step up from silent failure, but it still loses the root cause: strings "$tmp" 2>/dev/null discards strings's own stderr, so when the probe genuinely fails (truncated download, perms, corrupted ELF, etc.) the operator only sees "strings/awk version probe failed on the downloaded runtime" with no clue why. Either drop the 2>/dev/null on strings so its real error surfaces, or capture and include it in the warn, e.g. if ! err="$(strings "$tmp" 2>&1 >/dev/null)"; then warn "strings failed: $err"; fi paired with a separate stdout-only capture. Also, verified="" on the next line is redundant — when the if-condition fails, verified is already empty from the failed substitution.
| warn "strings/awk version probe failed on the downloaded runtime; falling back to the installed path" | |
| if verified="$(strings "$tmp" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }')"; then | |
| : | |
| else | |
| warn "strings/awk version probe failed on the downloaded runtime; falling back to the installed path" | |
| fi |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
jaylfc
force-pushed
the
fix/rknpu-strings-sigpipe
branch
from
aa6d532 to
62f8775
Compare
| # No early awk exit: it SIGPIPEs strings and trips pipefail (see the | ||
| # note at the version-verify site). Read to EOF and print the first | ||
| # match only. | ||
| strings "$LIBRKNNRT_DEST" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }' |
There was a problem hiding this comment.
SUGGESTION: The newly-added 2>/dev/null on strings "$LIBRKNNRT_DEST" is inconsistent with the rationale in commit 3/3 at line 358, which explicitly removed 2>/dev/null so strings's real diagnostic surfaces on a failed probe ("if the probe fails (truncated download, bad perms, corrupted ELF) its real error is the diagnostic"). The same logic applies here: if strings fails on the installed library (vendor image with unusual perms, corrupted ELF, etc.), the env banner that consumes this output will silently show no NPU runtime line with no operator-visible cause. Either drop the 2>/dev/null to match the line 358 policy, or capture and surface the error like the pin-site does. Note: under set -o pipefail + set -e, a failed strings here would also abort the whole script if the caller didn't guard it, which is the same class of bug the rest of this PR is fixing.
| strings "$LIBRKNNRT_DEST" 2>/dev/null | awk '/librknnrt version: / && !seen { print $3; seen=1 }' | |
| strings "$LIBRKNNRT_DEST" | awk '/librknnrt version: / && !seen { print $3; seen=1 }' |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…irst install The deterministic first-run abort of install-rknpu.sh (#1560, #1543) was NOT ldconfig. The librknnrt version check ran: verified="$(strings "$tmp" | awk '/librknnrt version: / { print $3; exit }')" awk exits on the first match, closing the pipe while strings still has ~7MB to write, so strings dies with SIGPIPE. Under set -o pipefail the substitution exits 141, and with set -e that aborted the entire install. It only mattered where binutils (strings) is present, and only on the FIRST run: the second run found librknnrt already pinned to 2.3.0 and skipped this whole block, which is exactly the deterministic-first-run / working-second-run pattern reported. Fix: awk reads to EOF (prints the first match without exiting) and the assignment is guarded, so a strings/awk hiccup can never be fatal. Same early-exit pattern removed from librknnrt_current_version for consistency. Proven with a set -e -o pipefail repro: old pattern exits 141, new returns the version and exits 0. Fixes #1560.
…robe fails Folds Kilo: distinguishing a genuine strings/awk failure from a missing version string, so a future first-run problem is not masked as the benign binutils-absent case. The pipeline status is captured via the if-condition (still no early awk exit, so no SIGPIPE abort).
…; drop redundant reset - remove 2>/dev/null from the strings probe so a genuine failure (truncated download, bad perms, corrupted ELF) shows its real diagnostic in the log instead of only the ambiguous warn - drop the redundant verified="" reset: a failed command substitution already leaves the variable empty
… probe too Fold Kilo nit: librknnrt_current_version() kept 2>/dev/null, inconsistent with the version-verify site that dropped it so a real strings failure (vendor perms, corrupted ELF) surfaces in the log instead of the banner silently dropping the runtime line. The awk already reads to EOF and both callers guard with || true, so a probe failure still cannot abort the install.
jaylfc
force-pushed
the
fix/rknpu-strings-sigpipe
branch
from
62f8775 to
e06f4f7
Compare
1 participant
Root-causes the deterministic first-run abort of install-rknpu.sh that @mandresve reported across two clean Bookworm installs (#1560, formerly #1543). It was not ldconfig.
The librknnrt version check ran
strings $tmp | awk '/librknnrt version: / { print $3; exit }'. awk exits on the first match and closes the pipe whilestringsstill has ~7MB of the binary left to write, sostringstakes SIGPIPE. Underset -o pipefailthe substitution exits 141, and withset -ethat aborts the whole install. It only triggers where binutils is present (mandresve's vendor image has it) and only on the first run: the second run finds librknnrt already at 2.3.0 and skips the entire pin block, which is exactly the reported deterministic-first / working-second pattern.Fix: awk reads to EOF (first match, no early exit) and the assignment is guarded so a strings/awk hiccup is never fatal; the same early-exit pattern is removed from
librknnrt_current_version. Proven with aset -e -o pipefailrepro (old pattern exits 141, new returns the version, exits 0).Fixes #1560.
Summary by CodeRabbit
binutilsinstalled.