Conversation
There was a problem hiding this comment.
Pull request overview
This PR bumps ostool from version 0.8.12 to 0.8.13 and enhances QEMU UEFI boot support by switching from the -bios approach to a pflash-based UEFI boot flow with proper ESP directory and NVRAM vars setup.
Changes:
- Replaces the simple
-biosOVMF loading with pflash drives (code + vars) and a FAT ESP directory, enabling a more realistic UEFI boot experience. - Refactors architecture detection to be done inline in
run()and selectsq35machine type for x86 architectures instead of always usingvirt. - Adds helper methods (
prepare_uefi_esp,prepare_uefi_vars,uefi_artifact_dir,default_uefi_boot_filename) to construct the UEFI boot environment.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ostool/Cargo.toml | Version bump to 0.8.13 |
| Cargo.lock | Lock file update for version 0.8.13 |
| ostool/src/run/qemu.rs | Replaces -bios with pflash-based UEFI boot, adds ESP/vars preparation, refactors arch detection and machine type selection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let code = prebuilt.get_file(arch, FileType::Code); | ||
| let vars_template = prebuilt.get_file(arch, FileType::Vars); | ||
| let esp_dir = self.prepare_uefi_esp(arch).await?; | ||
| let vars = self.prepare_uefi_vars(&vars_template).await?; | ||
|
|
||
| Ok(Some(UefiBootConfig::Pflash { | ||
| code, | ||
| vars, | ||
| esp_dir, | ||
| })) |
| cmd.arg("-drive").arg(format!( | ||
| "if=pflash,format=raw,unit=0,readonly=on,file={}", | ||
| code.display() | ||
| )); | ||
| cmd.arg("-drive").arg(format!( | ||
| "if=pflash,format=raw,unit=1,file={}", | ||
| vars.display() | ||
| )); | ||
| cmd.arg("-drive") | ||
| .arg(format!("format=raw,file=fat:rw:{}", esp_dir.display())); |
| path::Path, | ||
| path::PathBuf, |
![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: 426874eb31
ℹ️ 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".
| .with_path("failed to create directory", &boot_dir)?; | ||
|
|
||
| let boot_path = boot_dir.join(Self::default_uefi_boot_filename(arch)); | ||
| fs::copy(bin_path, &boot_path).await.with_context(|| { |
There was a problem hiding this comment.
With uefi = true, this path copies ctx.paths.artifacts.bin into EFI/BOOT/BOOT*.EFI, but in this codebase objcopy_output_bin() produces a raw -O binary image, not a PE/COFF EFI executable. Because the commit also disables the -kernel loader in UEFI mode, default workflows (to_bin = true) now hand OVMF a non-EFI file and the VM will fail to boot for existing ELF-based kernels.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn uefi_artifact_dir(&self, bin_path: &Path) -> anyhow::Result<PathBuf> { | ||
| let metadata = self.ctx.metadata()?; |
There was a problem hiding this comment.
This helper unconditionally calls self.ctx.metadata() before computing the artifact directory, so UEFI runs now fail in system = "Custom" projects that do not have a Cargo.toml in manifest (a supported mode). Previously the UEFI preparation path did not depend on Cargo metadata, so this introduces a regression for non-Cargo custom build workflows.
Useful? React with 👍 / 👎.
No description provided.