Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8490435a42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if config_path.exists() { | ||
| let config_content = fs::read_to_string(&config_path) | ||
| .await | ||
| .with_path("failed to read file", &config_path)?; | ||
| let config: QemuConfig = toml::from_str(&config_content) | ||
| .with_context(|| format!("failed to parse QEMU config: {}", config_path.display()))?; | ||
| config | ||
| } else { | ||
| let mut config = QemuConfig { | ||
| to_bin: true, | ||
| ..Default::default() | ||
| }; | ||
| config.args.push("-nographic".to_string()); | ||
| if let Some(arch) = ctx.arch { | ||
| match arch { | ||
| Architecture::Aarch64 => { | ||
| config.args.push("-cpu".to_string()); | ||
| config.args.push("cortex-a53".to_string()); | ||
| } | ||
| Architecture::Riscv64 => { | ||
| config.args.push("-cpu".to_string()); | ||
| config.args.push("rv64".to_string()); | ||
| } | ||
| _ => {} | ||
| return Ok(config); |
There was a problem hiding this comment.
When run_qemu_with_more_default_args() is used in a workspace that already has a QEMU config file (the normal case, since find_qemu_config() and menuconfig both prefer .qemu.toml), this early return drops the supplied args, success_regex, and fail_regex entirely. In that scenario the new helper behaves exactly like run_qemu(), so callers cannot actually add the advertised extra defaults unless the config file is absent.
Useful? React with 👍 / 👎.
| let config = build_default_qemu_config(ctx.arch, overrides); | ||
| fs::write(&config_path, toml::to_string_pretty(&config)?) | ||
| .await | ||
| .with_path("failed to write file", &config_path)?; |
There was a problem hiding this comment.
.qemu.toml
These values now come from a per-invocation helper, but this branch serializes them into the auto-created config file. If a caller passes temporary QEMU flags or a bad regex while no config exists, the first run leaves a sticky or even invalid .qemu.toml behind, and later plain run_qemu() calls keep inheriting or re-failing on those persisted overrides. This makes behavior depend on whether the file existed before the run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
该 PR 在 ostool 的 QEMU 运行逻辑中引入“默认配置覆盖”机制，用于在首次生成 .qemu.toml 默认配置时追加自定义 QEMU 参数及成功/失败正则，从而支持更灵活的测试/运行场景配置。
Changes:
- 新增
QemuDefaultOverrides，并将默认配置生成逻辑抽取为build_default_qemu_config - 将
run_qemu重构为通过run_qemu_with_defaults/load_or_create_qemu_config/run_qemu_with_config组织流程 - 增加单元测试覆盖默认配置生成与覆盖追加行为
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub async fn run_qemu_with_more_default_args( | ||
| ctx: AppContext, | ||
| run_args: RunQemuArgs, | ||
| args: Vec<String>, | ||
| success_regex: Vec<String>, | ||
| fail_regex: Vec<String>, | ||
| ) -> anyhow::Result<()> { |
| run_qemu_with_defaults( | ||
| ctx, | ||
| run_args, | ||
| QemuDefaultOverrides { | ||
| args, | ||
| success_regex, | ||
| fail_regex, | ||
| }, | ||
| ) | ||
| .await |
| pub async fn run_qemu_with_more_default_args( | ||
| ctx: AppContext, | ||
| run_args: RunQemuArgs, | ||
| args: Vec<String>, | ||
| success_regex: Vec<String>, | ||
| fail_regex: Vec<String>, |
ZR233
changed the title
There was a problem hiding this comment.
Pull request overview
This PR enhances ostool’s QEMU configuration workflow by (1) allowing callers to add extra default QEMU args/output matchers without fully replacing generated defaults, and (2) improving QEMU/build config discovery via shared path resolution with architecture-aware filename preference.
Changes:
- Add “default overrides” support when generating a new QEMU config, plus unit tests for override merging.
- Introduce shared QEMU config path resolution with arch-specific filename priority and optional
config_search_dir, and update menuconfig to save back to the resolved path. - Update build config path resolution to optionally prefer
config_search_dir/.build.tomloverworkspace/.build.toml, and expand path-resolution test coverage.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ostool/src/run/qemu.rs |
Adds default override plumbing, QEMU config path resolver, and unit tests for override/path resolution. |
ostool/src/menuconfig.rs |
Uses the shared QEMU config resolver and saves edits back to the resolved config path. |
ostool/src/main.rs |
Updates CLI help text for QEMU config default behavior. |
ostool/src/ctx.rs |
Adds config_search_dir and introduces build config path resolution helper. |
ostool/Cargo.toml |
Adds tempfile dev-dep and updates dependency versions/features. |
Cargo.lock |
Lockfile updates from dependency changes. |
.gitignore |
Ignores .qemu*.toml to avoid local arch-specific QEMU configs showing as untracked. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| success_regex: overrides.success_regex, | ||
| fail_regex: overrides.fail_regex, | ||
| ..Default::default() | ||
| }; |
There was a problem hiding this comment.
build_default_qemu_config currently replaces success_regex/fail_regex with the override vectors (success_regex: overrides.success_regex, fail_regex: overrides.fail_regex). The PR description/API naming suggests these should be appended on top of any generated defaults (like how args are extended). Consider initializing from defaults and extend-ing with the override patterns so overrides are additive rather than replacing.
| success_regex: overrides.success_regex, | |
| fail_regex: overrides.fail_regex, | |
| ..Default::default() | |
| }; | |
| ..Default::default() | |
| }; | |
| // Make override patterns additive: append to any default regexes | |
| config.success_regex.extend(overrides.success_regex); | |
| config.fail_regex.extend(overrides.fail_regex); |
| pub build_config: Option<BuildConfig>, | ||
| /// Path to the build configuration file. | ||
| pub build_config_path: Option<PathBuf>, | ||
| /// 可选的配置文件搜索目录，设置后优先于 workspace/manifest 进行配置发现 | ||
| pub config_search_dir: Option<PathBuf>, | ||
| } |
There was a problem hiding this comment.
config_search_dir is introduced and used by config resolution, but it doesn’t appear to be set from any CLI flag, env var, or config loading path (e.g., ostool/src/main.rs and src/bin/cargo-osrun.rs construct AppContext without it). As-is, end users can’t actually take advantage of the new search-dir behavior described in the PR. Please wire this field up to a user-facing option (or remove it until it’s supported).
| schemars = {workspace = true, features = ["derive"]} | ||
| serde = {workspace = true, features = ["derive"]} | ||
| serde_json = {workspace = true} | ||
| reqwest = { version = "0.13" } |
There was a problem hiding this comment.
The reqwest dependency is changed from an explicit default-features = false + rustls-tls setup to reqwest = { version = "0.13" }, which implicitly enables reqwest’s default TLS/backend feature set. That can materially change binary size, platform support, and TLS/root-store behavior, and it isn’t mentioned in the PR description (which focuses on QEMU/config discovery). If this bump isn’t required for the QEMU work/tests, consider reverting to the previous feature selection or documenting the intended TLS/backend change.
| reqwest = { version = "0.13" } | |
| reqwest = { version = "0.13", default-features = false, features = ["rustls-tls"] } |
| #[test] | ||
| fn build_config_explicit_path_wins() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let workspace = tmp.path().to_path_buf(); | ||
| let manifest = workspace.join("manifest"); | ||
| std::fs::create_dir(&manifest).unwrap(); | ||
| let search_dir = workspace.join("config"); | ||
| std::fs::create_dir(&search_dir).unwrap(); | ||
|
|
||
| // 创建多个配置文件 | ||
| std::fs::write(search_dir.join(".build.toml"), "").unwrap(); | ||
| std::fs::write(workspace.join(".build.toml"), "").unwrap(); | ||
|
|
||
| let ctx = AppContext { | ||
| paths: PathConfig { | ||
| workspace: workspace.clone(), | ||
| manifest, | ||
| ..Default::default() | ||
| }, | ||
| config_search_dir: Some(search_dir), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| // 显式路径应该优先 | ||
| let explicit = workspace.join("custom.build.toml"); | ||
| let result = ctx.resolve_build_config_path(Some(explicit.clone())); | ||
| assert_eq!(result, explicit); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_config_search_dir_beats_workspace() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let workspace = tmp.path().to_path_buf(); | ||
| let manifest = workspace.join("manifest"); | ||
| std::fs::create_dir(&manifest).unwrap(); | ||
| let search_dir = workspace.join("config"); | ||
| std::fs::create_dir(&search_dir).unwrap(); | ||
|
|
||
| // 创建多个配置文件 | ||
| std::fs::write(search_dir.join(".build.toml"), "[system]").unwrap(); | ||
| std::fs::write(workspace.join(".build.toml"), "[system]").unwrap(); | ||
|
|
||
| let ctx = AppContext { | ||
| paths: PathConfig { | ||
| workspace: workspace.clone(), | ||
| manifest, | ||
| ..Default::default() | ||
| }, | ||
| config_search_dir: Some(search_dir.clone()), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let result = ctx.resolve_build_config_path(None); | ||
| assert_eq!(result, search_dir.join(".build.toml")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_config_fallback_to_workspace() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let workspace = tmp.path().to_path_buf(); | ||
| let manifest = workspace.join("manifest"); | ||
| std::fs::create_dir(&manifest).unwrap(); | ||
|
|
||
| let ctx = AppContext { | ||
| paths: PathConfig { | ||
| workspace: workspace.clone(), | ||
| manifest, | ||
| ..Default::default() | ||
| }, | ||
| config_search_dir: None, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let result = ctx.resolve_build_config_path(None); | ||
| assert_eq!(result, workspace.join(".build.toml")); | ||
| } |
There was a problem hiding this comment.
The tests under this module include build-config path resolution cases (resolve_build_config_path), which live in ctx.rs. Keeping these tests colocated with the code under test (e.g., in ctx.rs’s test module or an integration test) would make future maintenance easier and avoid mixing unrelated concerns into run/qemu.rs.
| #[test] | |
| fn build_config_explicit_path_wins() { | |
| let tmp = TempDir::new().unwrap(); | |
| let workspace = tmp.path().to_path_buf(); | |
| let manifest = workspace.join("manifest"); | |
| std::fs::create_dir(&manifest).unwrap(); | |
| let search_dir = workspace.join("config"); | |
| std::fs::create_dir(&search_dir).unwrap(); | |
| // 创建多个配置文件 | |
| std::fs::write(search_dir.join(".build.toml"), "").unwrap(); | |
| std::fs::write(workspace.join(".build.toml"), "").unwrap(); | |
| let ctx = AppContext { | |
| paths: PathConfig { | |
| workspace: workspace.clone(), | |
| manifest, | |
| ..Default::default() | |
| }, | |
| config_search_dir: Some(search_dir), | |
| ..Default::default() | |
| }; | |
| // 显式路径应该优先 | |
| let explicit = workspace.join("custom.build.toml"); | |
| let result = ctx.resolve_build_config_path(Some(explicit.clone())); | |
| assert_eq!(result, explicit); | |
| } | |
| #[test] | |
| fn build_config_search_dir_beats_workspace() { | |
| let tmp = TempDir::new().unwrap(); | |
| let workspace = tmp.path().to_path_buf(); | |
| let manifest = workspace.join("manifest"); | |
| std::fs::create_dir(&manifest).unwrap(); | |
| let search_dir = workspace.join("config"); | |
| std::fs::create_dir(&search_dir).unwrap(); | |
| // 创建多个配置文件 | |
| std::fs::write(search_dir.join(".build.toml"), "[system]").unwrap(); | |
| std::fs::write(workspace.join(".build.toml"), "[system]").unwrap(); | |
| let ctx = AppContext { | |
| paths: PathConfig { | |
| workspace: workspace.clone(), | |
| manifest, | |
| ..Default::default() | |
| }, | |
| config_search_dir: Some(search_dir.clone()), | |
| ..Default::default() | |
| }; | |
| let result = ctx.resolve_build_config_path(None); | |
| assert_eq!(result, search_dir.join(".build.toml")); | |
| } | |
| #[test] | |
| fn build_config_fallback_to_workspace() { | |
| let tmp = TempDir::new().unwrap(); | |
| let workspace = tmp.path().to_path_buf(); | |
| let manifest = workspace.join("manifest"); | |
| std::fs::create_dir(&manifest).unwrap(); | |
| let ctx = AppContext { | |
| paths: PathConfig { | |
| workspace: workspace.clone(), | |
| manifest, | |
| ..Default::default() | |
| }, | |
| config_search_dir: None, | |
| ..Default::default() | |
| }; | |
| let result = ctx.resolve_build_config_path(None); | |
| assert_eq!(result, workspace.join(".build.toml")); | |
| } |
Summary
This PR improves ostool's QEMU configuration workflow in two connected areas:
First, it adds support for default QEMU overrides so callers can append extra QEMU arguments and expected output regex patterns without replacing the rest of the generated defaults.
Second, it refactors build and QEMU config discovery so configuration can be resolved from an explicit search directory and can prefer architecture-specific filenames before falling back to generic ones.
User Impact
Users running different targets can now keep per-architecture QEMU configs such as
qemu-aarch64.tomlor.qemu-aarch64.tomland let ostool discover them automatically. Users can also place config files in a dedicatedconfig_search_dirinstead of being forced into a single manifest or workspace location.For runtime behavior, the new default override structure makes it easier to extend generated QEMU defaults with additional command-line arguments and output matchers while preserving the existing defaults.
Root Cause
Before these changes:
.qemu.tomlpath instead of the resolved config pathThat combination made multi-architecture setups less predictable and made small runtime customizations more invasive than necessary.
Fix
This branch introduces the following behavior updates:
config_search_dir, then manifest directory, checking architecture-specific names before generic namesconfig_search_dir/.build.toml, thenworkspace/.build.toml.qemu*.tomlso local architecture-specific variants do not appear as untracked noiseValidation
Validated with targeted Rust tests:
cargo test -p ostool qemu_config_ -- --nocapturecargo test -p ostool build_config_ -- --nocapture