fix: restore OpenOCD scripts/ search path for bundled releases#11
Conversation
The xpack-bundled OpenOCD release archive ships its standard scripts under <install>/scripts/ (interface/, target/, ...). Our .cfg files reference these via `source [find interface/stlink.cfg]` etc., which resolves only through OpenOCD's search path. A previous cleanup commit dropped the `-s scripts` argument, thinking it was a leftover from the old script-dir layout — it wasn't. Without that path, the bundled OpenOCD has to fall back to its compiled-in install prefix, which doesn't match the flat layout the release archive produces. Restore the search path with an absolute resolution against exe_dir, guarded by an `is_dir()` check so a system-OpenOCD install (where `<exe_dir>/scripts/` doesn't exist) keeps using OpenOCD's defaults. Closes #8.
There was a problem hiding this comment.
Pull request overview
Restores OpenOCD’s ability to resolve standard interface//target/ scripts when using the xpack-bundled OpenOCD layout by re-adding an OpenOCD -s search path entry that points at <exe_dir>/scripts when present.
Changes:
- Adds an existence-guarded
-s <exe_dir>/scriptsargument to OpenOCD invocations (for bundled releases). - Simplifies the
cmd.args(...)usage by removing an unnecessary re-binding.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .into_string() | ||
| .map_err(|_| "Failed to convert scripts path to string")?; | ||
| cmd.args(&["-s", &scripts_str]); |
There was a problem hiding this comment.
The new error string "Failed to convert scripts path to string" is missing a trailing period and is slightly inconsistent with nearby messages (e.g., tmp_dir/config path conversions). Consider standardizing the punctuation/style so user-facing errors are consistent.
There was a problem hiding this comment.
Made moot by 568e386 — the conversion (and its error message) are gone now that Command::arg accepts the PathBuf directly. Resolved by following the sibling suggestion.
| let scripts_str = scripts_folder | ||
| .into_os_string() | ||
| .into_string() | ||
| .map_err(|_| "Failed to convert scripts path to string")?; | ||
| cmd.args(&["-s", &scripts_str]); |
There was a problem hiding this comment.
Command can accept OsStr/Path arguments directly, so converting scripts_folder to a UTF-8 String is unnecessary and can fail on non-UTF8 paths. Consider passing the PathBuf (or OsStr) directly to avoid the extra conversion and keep this consistent with Rust’s usual path handling.
| let scripts_str = scripts_folder | |
| .into_os_string() | |
| .into_string() | |
| .map_err(|_| "Failed to convert scripts path to string")?; | |
| cmd.args(&["-s", &scripts_str]); | |
| cmd.arg("-s").arg(&scripts_folder); |
There was a problem hiding this comment.
Fixed in 568e386 — applied your suggestion as-is. cmd.arg("-s").arg(&scripts_folder) removes the panic vector on non-UTF-8 paths and is cleaner. Thanks!
Command::arg accepts OsStr/Path directly, so the into_os_string() + into_string() conversion was both unnecessary and a panic vector on non-UTF-8 paths. Per Copilot review on PR #11.
Summary
Hotfix for #8.
A cleanup commit during the wireless-stack PR (#5) dropped the
-s scriptsargument passed to OpenOCD, on the assumption that it was a leftover from the old script-dir layout. It wasn't.The xpack-bundled release archive places OpenOCD's standard scripts (interface/, target/, ...) at
<install>/scripts/next to the binary. Our .cfg files load these withsource [find interface/stlink.cfg]etc., which only resolves through OpenOCD's search path. Without-s <scripts-path>, the bundled OpenOCD falls back to its compiled-in install prefix, which doesn't match the flat layout — first flash on a fresh release archive fails withCan't find interface/stlink.cfg.This restores the search path, but with two improvements over the original:
dirs::get_exe_dir()instead of the bare relative"scripts", so it works regardless of the user's CWD.is_dir(), so a system-OpenOCD setup (where<exe_dir>/scripts/doesn't exist) keeps using OpenOCD's compiled-in defaults instead of getting a useless-sarg.Test plan
cargo runfrom the repo root: DapLink unlock/erase/flash still resolves via the system OpenOCD (no<exe_dir>/scripts/present,-sarg skipped).cargo build --release+ manually mirror the workflow's archive layout into a tmpdir withconfigs/,wireless_stack/,scripts/,openocd); run the binary; confirm the unlock step findsinterface/stlink.cfg.wb5x.cfgwhichsource [find interface/cmsis-dap.cfg]) succeeds with the bundled scripts path.