Install FastLED probe-rs for LPC SWD deploy#1001
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes 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 Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds a ChangesNo-probe-rs flag and pinned probe-rs install
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
51809c2 to
567c849
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fbuild-cli/src/mcp/tools.rs (1)
151-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
no_probe_rsis missing from the MCPtrigger_deploytool. Add it to the tool schema and forward it intoDeployRequest; right now MCP callers can only sendskip_build, so the lpc21isp fallback stays unreachable from this path.🤖 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 `@crates/fbuild-cli/src/mcp/tools.rs` around lines 151 - 185, The MCP trigger_deploy path is not forwarding the no_probe_rs option, so the deploy request always leaves the lpc21isp fallback disabled. Update the trigger_deploy tool schema in tools.rs to accept no_probe_rs alongside skip_build, then thread that parsed value into crate::daemon_client::DeployRequest in the same request construction used by trigger_deploy.
🧹 Nitpick comments (1)
crates/fbuild-deploy/src/probe_rs.rs (1)
186-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStaging directory is never cleaned up.
Each install creates a unique
probe-rs-install/<tag>-<pid>-<millis>dir holding the downloaded archive plus extracted tree and leaves it behind. Consider removing the staging dir after a successful install (or on the error paths) to avoid accumulating archives in the temp root over repeated installs.🤖 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 `@crates/fbuild-deploy/src/probe_rs.rs` around lines 186 - 203, The probe-rs install flow leaves the per-run staging directory behind, causing downloaded archives and extracted files to accumulate. Update the install path in probe_rs.rs around probe_rs_staging_dir, download_file, verify_checksum_async, and extract_and_install_probe_rs so the temporary staging dir is removed after a successful install and also cleaned up on failure paths. Keep the cleanup tied to the existing install workflow and ensure any errors from cleanup do not hide the اصلی install error.
🤖 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.
Inline comments:
In `@crates/fbuild-deploy/src/probe_rs.rs`:
- Around line 218-253: The install flow in extract_and_install_probe_rs writes
directly to dest_path, which can race across concurrent deploys and leave a
partial probe-rs binary on interruption. Change the function to copy the found
binary into a sibling temporary file under the destination directory, apply the
Unix permissions to that temp file, and then atomically rename it over
dest_path. Keep the existing error handling around extract_and_install_probe_rs,
std::fs::copy, and permission setting, but ensure the final swap to dest_path is
the only write to the target.
---
Outside diff comments:
In `@crates/fbuild-cli/src/mcp/tools.rs`:
- Around line 151-185: The MCP trigger_deploy path is not forwarding the
no_probe_rs option, so the deploy request always leaves the lpc21isp fallback
disabled. Update the trigger_deploy tool schema in tools.rs to accept
no_probe_rs alongside skip_build, then thread that parsed value into
crate::daemon_client::DeployRequest in the same request construction used by
trigger_deploy.
---
Nitpick comments:
In `@crates/fbuild-deploy/src/probe_rs.rs`:
- Around line 186-203: The probe-rs install flow leaves the per-run staging
directory behind, causing downloaded archives and extracted files to accumulate.
Update the install path in probe_rs.rs around probe_rs_staging_dir,
download_file, verify_checksum_async, and extract_and_install_probe_rs so the
temporary staging dir is removed after a successful install and also cleaned up
on failure paths. Keep the cleanup tied to the existing install workflow and
ensure any errors from cleanup do not hide the اصلی install error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c06f1110-4d94-4fdd-b1ad-39e8f5d4f828
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/fbuild-cli/src/cli/args.rscrates/fbuild-cli/src/cli/deploy.rscrates/fbuild-cli/src/cli/dispatch.rscrates/fbuild-cli/src/daemon_client/types.rscrates/fbuild-cli/src/mcp/tools.rscrates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-daemon/src/models.rscrates/fbuild-deploy/Cargo.tomlcrates/fbuild-deploy/src/lpc.rscrates/fbuild-deploy/src/probe_rs.rs
| fn extract_and_install_probe_rs( | ||
| archive: &Path, | ||
| staging_dir: &Path, | ||
| dest_path: &Path, | ||
| ) -> Result<PathBuf> { | ||
| let extract_dir = staging_dir.join("extract"); | ||
| std::fs::create_dir_all(&extract_dir)?; | ||
| fbuild_packages::extractor::extract(archive, &extract_dir)?; | ||
|
|
||
| let found = find_extracted_probe_rs_binary(&extract_dir)?; | ||
| let dest_dir = dest_path.parent().ok_or_else(|| { | ||
| FbuildError::PackageError(format!( | ||
| "managed probe-rs path has no parent: {}", | ||
| dest_path.display() | ||
| )) | ||
| })?; | ||
| std::fs::create_dir_all(dest_dir)?; | ||
| std::fs::copy(&found, dest_path).map_err(|e| { | ||
| FbuildError::PackageError(format!( | ||
| "failed to install probe-rs from {} to {}: {}", | ||
| found.display(), | ||
| dest_path.display(), | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| let mut perms = std::fs::metadata(dest_path)?.permissions(); | ||
| perms.set_mode(perms.mode() | 0o755); | ||
| std::fs::set_permissions(dest_path, perms)?; | ||
| } | ||
|
|
||
| Ok(dest_path.to_path_buf()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby helpers to see how installs are coordinated.
git ls-files crates/fbuild-deploy/src/probe_rs.rs
wc -l crates/fbuild-deploy/src/probe_rs.rs
sed -n '1,340p' crates/fbuild-deploy/src/probe_rs.rs
# Look for any locking, temp-file, or atomic-install helpers used elsewhere in the repo.
rg -n "rename\\(|create_new\\(|tempfile|atomic|lock|Mutex|flock|probe-rs" crates/fbuild-deploy crates -g '!**/target/**'Repository: FastLED/fbuild
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read just the install helper and the surrounding resolver functions.
sed -n '170,290p' crates/fbuild-deploy/src/probe_rs.rs
# Check for any existing atomic-install helper in the workspace that this code could reuse.
rg -n "write_atomic|atomic rename|rename\\(|temp file|tempfile::NamedTempFile|create_new\\(" crates/fbuild-* crates/fbuild-core -g '!**/target/**'Repository: FastLED/fbuild
Length of output: 15101
Install via a temp file and atomic rename. crates/fbuild-deploy/src/probe_rs.rs:218-253 writes straight to dest_path, so concurrent deploys can race on the same file and an interrupted copy can leave a partial executable. Copy to a sibling temp file, set permissions there, then rename onto dest_path so the swap is atomic.
🤖 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 `@crates/fbuild-deploy/src/probe_rs.rs` around lines 218 - 253, The install
flow in extract_and_install_probe_rs writes directly to dest_path, which can
race across concurrent deploys and leave a partial probe-rs binary on
interruption. Change the function to copy the found binary into a sibling
temporary file under the destination directory, apply the Unix permissions to
that temp file, and then atomically rename it over dest_path. Keep the existing
error handling around extract_and_install_probe_rs, std::fs::copy, and
permission setting, but ensure the final swap to dest_path is the only write to
the target.
Summary
fastled-v0.31.2-nusb-v1-transport) and map all six host artifacts with GitHub release SHA256 digests.~/.fbuild/{prod|dev}/tools/probe-rs/probe-rs[.exe]using the existing fbuild package downloader, checksum verifier, and extractor.LpcDeployerprefer probe-rs SWD when an LPC-Link2 CMSIS-DAP probe is present, while keeping clear fallback tolpc21ispand addingfbuild deploy --no-probe-rs.Closes #933
Closes #934
Refs #1000
Validation
gh release list --repo FastLED/framework-arduino-lpc8xx --limit 10showsfastled-v0.31.2-nusb-v1-transportas Latest.gh release view fastled-v0.31.2-nusb-v1-transport --repo FastLED/framework-arduino-lpc8xx --json tagName,name,publishedAt,body,assets,urlconfirms the six artifact digests used in fbuild.soldr cargo fmt --allgit diff --checksoldr --no-cache cargo test -p fbuild-deploy probe_rssoldr --no-cache cargo test -p fbuild-deploy lpcsoldr --no-cache cargo test -p fbuild-daemon deploy_requestsoldr --no-cache cargo check -p fbuild-deploy -p fbuild-daemon -p fbuild-cliFBUILD_DEV_MODE=1 .\target\x86_64-pc-windows-msvc\debug\fbuild.exe deploy tests\platform\lpc845brk -e lpc845brk --port COM10 --verboseinstalled the managed dev probe-rs artifact, flashedfirmware.elfviaprobe-rs download --chip LPC845M301JBD48 --probe 1fc9:0132, and COM10 reopened successfully afterward.Note: the normal cached
soldr cargo test ...path wedged locally during this run.soldr --helpdocuments--no-cacheas the recovery path for a wedged cache, so the validation above usedsoldr --no-cache.Summary by CodeRabbit
New Features
Bug Fixes