refactor(tool): 收敛构建配置与运行产物边界#114
Conversation
There was a problem hiding this comment.
审查总结
本 PR 将构建配置加载、menuconfig hooks 和运行产物状态从 tool.rs 大 facade 中拆分到独立模块,是一次干净、结构清晰的重构。整体质量很高,代码组织和封装都有明显改善。
👍 优点
- 模块拆分合理 —
config_loader.rs、config_hooks.rs、artifact/state.rs各自职责单一、边界清晰 - 封装性好 —
OutputArtifacts字段改为私有 + getter 方法,新增的runtime_artifacts()、runtime_arch()、require_bin()、runtime_image()提供了干净的抽象接口 runtime_image()设计巧妙 — 优雅地替代了之前 QEMU runner 中bin.or(elf)的手动 fallback 模式- 测试覆盖充分 —
config_hooks.rs新增 10 个测试,涵盖 docs.rs target 解析、rustup target 解析、默认目标排序、错误路径等 - 向后兼容 —
prepare_elf_artifact作为公开兼容入口保留,ctx.rs通过pub use保持类型可见 - 单次提交、意图清晰 — commit message 详细说明了变更范围和动机
💬 观察和建议(非阻塞)
-
config_hooks.rs已达 584 行 —parse_docs_rs_targets的 JSON 解析+去重+默认目标排序逻辑较为复杂,后续可考虑进一步拆分到子模块 -
collect_feature_options的no_deps()限制 — 由于使用cargo_metadata的no_deps()模式,只有 workspace member 的依赖才能被发现并收集 features。外部 crate 的dep_name/feature格式 feature 不会被列出。这是既有行为,但建议在函数文档中注明这一限制 -
apply_prepared_runtime_artifacts中的set_runtime_artifact_dir仅#[cfg(test)]可见 — 封装做得好,但意味着测试代码使用的接口面与生产代码不同。目前合理,后续如有更多测试需求可再评估 -
parse_docs_rs_targets中去重使用O(n²)线性查找 — 目标数量预期很小所以没问题,但如果未来 target 列表增长可考虑用HashSet
结论
没有发现行为回归或正确性问题。重构保持 CLI/config/runtime 行为不变,公开 API 无新增面,模块拆分边界清晰。建议合入。
Powered by glm-5.1
|
|
||
| fn parse_docs_rs_targets(metadata: &serde_json::Value) -> anyhow::Result<Option<Vec<String>>> { | ||
| let Some(docs) = metadata.get("docs") else { | ||
| return Ok(None); |
There was a problem hiding this comment.
建议在函数文档中注明:由于使用 no_deps() 模式的 cargo_metadata,只有 workspace member 的依赖 feature 才会被收集。外部 crate 的 feature 不会出现在列表中。这是既有限制,但文档化有助于后续维护。
| } | ||
|
|
||
| let option = RustupTargetOption { | ||
| triple, |
There was a problem hiding this comment.
这里用 targets.iter().any(...) 做去重是 O(n²) 的。target 列表通常很短所以没问题,但如果需要处理大量 target 可以考虑用 HashSet 收集。
| workspace_dir: &Path, | ||
| config: &mut BuildConfig, | ||
| enable_someboot_build_config: bool, | ||
| ) -> anyhow::Result<()> { |
There was a problem hiding this comment.
LoadedBuildConfig 的设计很好,将配置路径和配置内容绑定在一起,避免了调用方手动追踪 path。
5059306 to
df4be24
Compare
Move build configuration loading and menuconfig hooks out of the legacy Tool facade while keeping CLI and runtime behavior routed through existing public APIs. Keep runtime artifact preparation behind build orchestration, make runtime artifact state private, and let QEMU/U-Boot runners read Tool state directly instead of storing copied snapshots. Trim helper APIs, unused hook parameters, and tests that only asserted implementation details. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Niu Zhihong <zhihong@nzhnb.com>
df4be24 to
f52a8e3
Compare
There was a problem hiding this comment.
审查总结
本 PR 将构建配置加载(config_loader.rs)、menuconfig hooks(config_hooks.rs)和运行产物状态(artifact/state.rs)从 tool.rs 大 facade 中拆分到独立模块,整体是一次高质量、边界清晰的重构。+910 / -743,14 个文件变更。
👍 优点
- 模块拆分合理、职责单一 —
config_loader负责加载路径解析 + someboot 注入,config_hooks负责 menuconfig 交互钩子,artifact::state封装运行产物状态,各自边界清晰 - 封装性显著提升 —
OutputArtifacts字段全部私有化 + getter 方法(elf()、bin()、runtime_image()、require_bin()),消除了tool.ctx.artifacts.bin.as_ref()这种直接字段访问模式在代码库中的扩散 runtime_image()设计优雅 —self.bin().or_else(|| self.elf())一行替代了 QEMU runner 中之前的if let bin ... else if let elf ...手动 fallback 逻辑runtime_arch()方法统一了架构读取 — 之前 QEMU/U-Boot runner 各自直接读取ctx.arch,现在统一走tool.runtime_arch()方法- 测试覆盖充分 —
config_hooks.rs新增 10 个单元测试,涵盖 docs.rs target 解析、去重、默认目标排序、rustup target 解析、空输入、错误路径等 - 向后兼容做得好 —
prepare_elf_artifact作为公开兼容入口保留;ctx.rs通过pub use crate::artifact::state::OutputArtifacts保持类型可见 main.rs中的清理合理 —apply_cargo_selector不再冗余写回build_config;测试名parse_board_run_defaults_to_no_overrides更贴切;cargo_selector的断言改为.package.is_none() && .bin.is_none()语义更精确
💬 建议(非阻塞)
config_hooks.rs已达 596 行 —parse_docs_rs_targets中 JSON 解析 + 去重 + 默认目标排序逻辑比较复杂(约 70 行),后续可考虑拆分为docs_rs.rs子模块apply_cargo_selector中移除了tool.ctx_mut().build_config = Some(build_config.clone())— 这个写回之前是冗余的(调用方会在后续步骤重新设置),但如果调用链中存在依赖这个副作用的路径,删除后需要确认不会有状态不一致。PR 描述中说 CLI 行为不变,应该是已验证#[cfg(test)]的set_runtime_artifact_dir— 这是一个合理的测试专用 API,但长期来看如果更多地方需要修改产物状态,可能需要一个更通用的 builder 模式collect_feature_options中no_deps()的限制已在文档注释中注明 — 很好,感谢采纳之前的建议
结论
没有发现行为回归或正确性问题。模块拆分边界清晰、封装改善明显、测试充分、向后兼容。建议合入。
Powered by glm-5.1
|
AI,轻而易举啊! |
同步 fork 侧计划文档到 upstream/main 已合入 drivercraft#115 后的状态。 记录 drivercraft#108/drivercraft#111/drivercraft#114/drivercraft#115 已完成 R1 上游友好切片,并明确 InvocationState、ActiveBuildContext、artifact state 与 build config loader/menu hooks 已进入主路径。 保留完整 R1 终态尚未完成的边界:Tool/ctx 仍是兼容 API,runner/board entrypoints 仍依赖 Tool facade;下一步默认先做 R2 artifact/object-tools 窄切片,或另行切 R1g runner/board seam。 Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Niu Zhihong <zhihong@nzhnb.com>
Summary
Toolfacade 中拆出,减少tool.rs的业务逻辑聚集。artifact::state::OutputArtifacts,并通过 getter / crate-private 方法访问,避免公开可变字段继续扩散。Toolstate 读取 runtime artifact 和 architecture,不再维护第二份快照状态。Approach
这次变更保留现有
Tool作为兼容 facade,但把无状态的配置加载、UI hook 构造和运行产物状态处理移到更窄的模块里。CLI、cargo-osrun、QEMU、U-Boot 和 board run 仍然通过已有公开入口运行,内部新增 helper 只保留 crate-private 可见性。Changed Modules
ostool/src/build/config_loader.rs: 集中处理 build config 路径解析、加载和 someboot build config 注入。ostool/src/build/config_hooks.rs: 承载 menuconfig 的 package / feature / target hook 逻辑。ostool/src/artifact/state.rs: 承载 runtime artifact state,并收紧字段访问。ostool/src/run/qemu.rs,ostool/src/run/uboot.rs: runner 直接读取Tool当前运行产物状态,避免复制 artifact / arch 快照。ostool/src/tool.rs: 收缩为兼容 facade,保留现有调用入口。Compatibility
Tool::prepare_elf_artifact仍作为公开兼容入口保留。Verification
在 devbox 容器中运行通过:
cargo fmt --all -- --checkCARGO_BUILD_JOBS=1 cargo test -p ostool -- --nocaptureCARGO_BUILD_JOBS=1 cargo clippy --target x86_64-unknown-linux-gnu --all-features -- -D warningsCARGO_BUILD_JOBS=1 cargo build -p ostool --target x86_64-unknown-linux-gnu --all-featuresgit diff --check未限制并发的
cargo test -p ostool -- --nocapture曾在链接阶段因容器内存峰值报Cannot allocate memory;低并发重跑已完整通过。Risks / Follow-up
Tool仍是兼容 facade,后续切片可以继续把 runner/build orchestration 迁往更窄的输入结构。