-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Embed bios and uefi binaries #395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR! It took me a while to understand your use case and the actual issue, but looking through https://github.com/mysteriouslyseeing/bootloader_linker/ cleared things up. Sorry about the weird crate design, I can imagine that this issue must be very difficult to debug!
I like the idea of having a self-contained library without any external file dependencies at runtime. So I think we can just always do the include_bytes
without any feature gates. Or do you see any disadvantage in that (aside from a slightly larger library size)?
src/lib.rs
Outdated
#[cfg(all(feature = "embedded_binaries", feature = "uefi"))] | ||
static UEFI_BOOTLOADER: &'static [u8] = include_bytes!(env!("UEFI_BOOTLOADER_PATH")); | ||
|
||
#[cfg(all(feature = "embedded_binaries", feature = "bios"))] | ||
static BIOS_BOOT_SECTOR: &'static [u8] = include_bytes!(env!("BIOS_BOOT_SECTOR_PATH")); | ||
#[cfg(all(feature = "embedded_binaries", feature = "bios"))] | ||
static BIOS_STAGE_2: &'static [u8] = include_bytes!(env!("BIOS_STAGE_2_PATH")); | ||
#[cfg(all(feature = "embedded_binaries", feature = "bios"))] | ||
static BIOS_STAGE_3: &'static [u8] = include_bytes!(env!("BIOS_STAGE_3_PATH")); | ||
#[cfg(all(feature = "embedded_binaries", feature = "bios"))] | ||
static BIOS_STAGE_4: &'static [u8] = include_bytes!(env!("BIOS_STAGE_4_PATH")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of statics, I think we can use consts here. E.g. const BIOS_BOOT_SECTOR: &'static [u8] = ...
.
src/lib.rs
Outdated
pub fn create_bios_image(&self, image_path: &Path) -> anyhow::Result<()> { | ||
const BIOS_STAGE_3_NAME: &str = "boot-stage-3"; | ||
const BIOS_STAGE_4_NAME: &str = "boot-stage-4"; | ||
let stage_3 = FileDataSource::Data(BIOS_STAGE_3.to_vec()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The to_vec
call copies the data to the heap whenever this function is invoked. I think we could avoid this copy by introducing a new FileDataSource::Bytes(&'static [u8])
variant, which could be used with statics and const data.
Added new variant for FileDataSource and inlined now unnecessary functions
Removed an unnecessary comma
Re-added multipl #[cfg(feature = "bios")] and #[cfg(feature = "uefi")]
I've made the changes you suggested, but it's now failing a check, specifically the Check docs.rs build. The reason why is that the build.rs script passes in blank PathBufs into the UEFI_BOOTLOADER_PATH (and co) environment variables when running under the docsrs_dummy_build cfg directive. #[cfg(not(docsrs_dummy_build))]
let uefi_path = build_uefi_bootloader(&out_dir).await;
// dummy implementation because docsrs builds have no network access
#[cfg(docsrs_dummy_build)]
let uefi_path = PathBuf::new(); The fact that the paths don't point to a file didn't matter for the original implementation without the This can be fixed changing the build script, so that rather than simply doing nothing if docsrs_dummy_build is set, it will instead populate the |
…rs_dummy_build" cfg directive Changed build.rs to place dummy files in out_dir when under the "docsrs_dummy_build" cfg directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the updates! Looks good!
This crate embeds links to bios and uefi binaries created in the build.rs script, and that means you can't depend on it with a crate from crates.io, as the binaries created are deleted after the final binary (with the code that you actually wrote) is moved to .cargo/bin/ and it throws a file not found error.
This was kind of annoying to pin down, because when I manually built it, it worked, until it stopped randomly. This was caused (although I didn't know this at the time) by the files being cleaned from the directory they were built in, no matter where the final binary was at that point.
So here is my solution; feature-gated binary embedding using include_bytes!()
I had to rewrite part of mbr but the api is exactly the same when the feature is disabled.
The main reason I wanted to use it as a dependency was to make a command-line interface to replace bootimage, which doesn't work with 10.x or 11.x (the resulting command-line crate is here) and I liked the crate structure that bootimage encouraged (main.rs, entry_point!(), lib.rs etc.) as opposed to the crate structure currently recommended (a build.rs that links bootloader to a subpackage containing the kernel and a main.rs that calls qemu).
Anyway, if this is all a terrible idea, please let me know.