Conversation
- Replaced instances of AppContext with Tool in menuconfig, qemu, tftp, and uboot modules. - Updated method signatures to accept Tool instead of AppContext. - Introduced Tool struct to encapsulate application context and configuration. - Adjusted related functions to utilize Tool's methods for accessing paths and artifacts. - Enhanced tests to accommodate the new Tool structure and ensure functionality remains intact.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Tool orchestration API and refactors the CLI + runners (QEMU/U-Boot/TFTP) to use it, adding file-based logging and new automation features (shell auto-init + timeouts).
Changes:
- Add
Tool/ToolConfig+ manifest/workspace resolution and migrate build/run flows away from the oldAppContextpath/config responsibilities. - Add a file logger (
{workspace}/target/ostool.ans) and update CLIs to report errors and log locations consistently. - Enhance QEMU/U-Boot runners with shared regex compilation/printing helpers, shell auto-init support, Linux system TFTP staging, and timeout handling.
Reviewed changes
Copilot reviewed 16 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ostool/src/tool.rs | New Tool API (manifest/workspace resolution, artifact handling, build config UI hooks). |
| ostool/src/ctx.rs | Slim down AppContext to runtime state + artifacts only. |
| ostool/src/run/qemu.rs | Refactor to Tool, add shell auto-init + timeout + stdin passthrough/raw-mode guard, adjust config resolution behavior. |
| ostool/src/run/uboot.rs | Refactor to Tool, add shell auto-init + timeout, improve TFTP handling and regex utilities usage. |
| ostool/src/run/tftp.rs | Add Linux tftpd-hpa detection/installation/config staging; keep built-in server for other platforms. |
| ostool/src/run/shell_init.rs | New shared shell auto-init matcher + delayed command sender. |
| ostool/src/run/output_matcher.rs | Factor out regex compilation + match printing + match-to-result conversion. |
| ostool/src/run/mod.rs | Register new shell_init module. |
| ostool/src/sterm/mod.rs | Add optional session timeout + terminal restore helper and propagate timeout errors. |
| ostool/src/logger.rs | New file logger implementation + initializer. |
| ostool/src/main.rs | Switch CLI to Tool, add logger init + improved error reporting. |
| ostool/src/bin/cargo-osrun.rs | Switch runner binary to Tool, add logger init + improved error reporting. |
| ostool/src/menuconfig.rs | Switch menuconfig handler to Tool and updated config path resolution. |
| ostool/src/build/mod.rs | Migrate build/run entrypoints to Tool; enhance QEMU runner kind defaults and config path selection. |
| ostool/src/build/cargo_builder.rs | Migrate to Tool; resolve cargo executable via JSON messages and propagate artifact dirs. |
| ostool/src/lib.rs | Export Tool API and add logger module. |
| ostool/Cargo.toml | Add chrono dependency for timestamped file logs. |
| Cargo.lock | Lockfile updates (chrono bump and windows-sys bumps via dependency resolution). |
| .gitignore | Ignore .sisyphus. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let timeout = self.timeout.unwrap(); | ||
| let label = self.timeout_label.as_deref().unwrap_or("serial terminal"); | ||
| return Err(anyhow!("{label} timed out after {}s", timeout.as_secs())); |
There was a problem hiding this comment.
timed_out can be set whenever stop_deadline expires (e.g., when callers use TermHandle::stop_after(MATCH_DRAIN_DURATION) to drain output), but run_terminal treats any timed_out() as a session timeout and then unconditionally does self.timeout.unwrap(). If no timeout was configured, this will panic; even if a timeout was configured, it can misreport a drain/stop as a timeout. Consider separating “session timeout” from other stop_after uses (e.g., distinct deadlines or a stop reason enum), and only return a timeout error when the configured session timeout expires.
| let timeout = self.timeout.unwrap(); | |
| let label = self.timeout_label.as_deref().unwrap_or("serial terminal"); | |
| return Err(anyhow!("{label} timed out after {}s", timeout.as_secs())); | |
| if let Some(timeout) = self.timeout { | |
| let label = self.timeout_label.as_deref().unwrap_or("serial terminal"); | |
| return Err(anyhow!("{label} timed out after {}s", timeout.as_secs())); | |
| } else { | |
| // Timed-out flag was set, but no session timeout was configured. | |
| // Treat this as a non-session deadline/drain completion rather than an error. | |
| info!("Serial terminal stopped after deadline without configured session timeout"); | |
| return Ok(()); | |
| } |
|
|
||
| pub fn restore_terminal_mode() { | ||
| let _ = disable_raw_mode(); | ||
| let _ = Command::new("stty").arg("echo").arg("icanon").status(); |
There was a problem hiding this comment.
restore_terminal_mode() unconditionally runs stty echo icanon. This is Unix-specific and will fail/no-op on Windows (and can also fail on minimal Linux containers without stty). Consider guarding the stty invocation with #[cfg(unix)] (or a runtime OS check) and relying on crossterm raw mode restoration on non-Unix platforms.
| let _ = Command::new("stty").arg("echo").arg("icanon").status(); | |
| #[cfg(unix)] | |
| { | |
| let _ = Command::new("stty").arg("echo").arg("icanon").status(); | |
| } |
| fn ui_hock_pacage_select(&self) -> ElemHock { | ||
| let path = "system.package"; | ||
| let cargo_toml = self.workspace_dir.join("Cargo.toml"); | ||
|
|
||
| ElemHock { | ||
| path: path.to_string(), | ||
| callback: Arc::new(move |siv: &mut Cursive, path: &str| { | ||
| let mut items = Vec::new(); | ||
| if let Ok(metadata) = cargo_metadata::MetadataCommand::new() | ||
| .manifest_path(&cargo_toml) | ||
| .no_deps() | ||
| .exec() | ||
| { | ||
| for pkg in &metadata.packages { | ||
| items.push(pkg.name.to_string()); | ||
| } | ||
| } | ||
|
|
||
| show_list_select(siv, "Pacage", &items, path, on_package_selected); | ||
| }), |
There was a problem hiding this comment.
Spelling: ui_hock_pacage_select / dialog title "Pacage" look like typos for "package". Renaming the function and fixing the UI label will improve clarity and avoid propagating the misspelling into public-ish identifiers.
No description provided.