feat: allow overriding embedded binary paths via env variables#553
feat: allow overriding embedded binary paths via env variables#553rrauch wants to merge 3 commits intocontainers:mainfrom
Conversation
Summary of ChangesHello @rrauch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the build process by introducing environment variable overrides for embedded binary paths. This change primarily addresses the challenge of using the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to override hardcoded paths for embedded binaries (INIT_BINARY and EDK2_BINARY) using environment variables at build time. This is a valuable improvement for enabling out-of-tree builds. The implementation correctly uses build scripts to set the necessary environment variables for include_bytes!. My review includes suggestions to improve the robustness of the new build scripts by replacing unwrap() calls with expect() to provide more descriptive error messages in case of build failures.
src/devices/build.rs
Outdated
| ) | ||
| }); | ||
| println!("cargo:rustc-env=INIT_BINARY_PATH={init_binary_path}"); | ||
| println!("cargo:rerun-if-changed={init_binary_path}"); |
There was a problem hiding this comment.
Shouldn't it be println!("cargo:rerun-if-env-changed=INIT_BINARY_PATH")?
There was a problem hiding this comment.
According to https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-env-changed:
As of 1.46, using env! and option_env! in source code will automatically detect changes and trigger rebuilds. rerun-if-env-changed is no longer needed for variables already referenced by these macros.
So that case is already covered by the env! macro.
println!("cargo:rerun-if-changed={init_binary_path}") handles the case where the init binary itself changed (e.g. recompiled).
There was a problem hiding this comment.
Actually we should have both. In case the value of the env variable had changed, and in case someone updated the binary file of init.
There was a problem hiding this comment.
Yes, I agree. And I think both cases are already covered. See my previous comment.
There was a problem hiding this comment.
Actually, It seems like include_bytes! seem to already handle that too (but I can't find it documented now)
There was a problem hiding this comment.
I can't find anything in the docs either. I found rust-lang/rust#24423 which states that the included file is tracked, but in the commit itself it says only the file is tracked and not its content - whatever that means in this context.
I would suggest to keep the cargo:rerun-if-changed - I can't see any harm here and it makes it explicit.
However, if you'd prefer to have it removed, I don't mind either.
There was a problem hiding this comment.
If I'm not mistaken, the rerun-if-* stuff specifies when the build script should be rebuilt. It's necessary to include the check for the environment variable (there is no env! in the script) so that the changed path is noticed. rerun-if-changed is superfluous because the contents of the file has no influence on the build script itself; having it included is harmless (at worst the build script is rebuilt unnecessary) but we should understand what's happening exactly.
There was a problem hiding this comment.
@mz-pdm Your comment is spot on! rerun-if-* is really only about when to re-run the build script, nothing else.
I believe your original suggestion is correct then; it should indeed be cargo:rerun-if-env-changed. I stand corrected.
I've tried it & it works reliably. Also the scenario where KRUN_INIT_BINARY_PATH was set to some value previously but has since become unset works correctly. I believe it might have been a bit "flaky" before.
I'll change it to cargo:rerun-if-env-changed then.
Adds `build.rs` to the `devices` and `vmm` crates. The `INIT_BINARY` path in `devices` can now be overridden with `INIT_BINARY_PATH`, and the `EDK2_BINARY` path in `vmm` with `EDK2_BINARY_PATH` respectively. Both fall back to the original paths if not set. Signed-off-by: Roland Rauch <roland@rauch.com>
bc31477 to
26e939d
Compare
To avoid any accidental inclusions, `INIT_BINARY_PATH` is renamed to `KRUN_INIT_BINARY_PATH` and `EDK2_BINARY_PATH` to `KRUN_EDK2_BINARY_PATH`. Signed-off-by: Roland Rauch <roland@rauch.com>
|
I had experimented with requiring the embedding project to supply the init binary as a memory range. This was quite nice as in my use case the coupling was between the embedding program and the init. So in that design, libkrun was only responsible for the hand-off from the host to the guest init instead of also owning init. I think the coupling and complexity of the built-in init suffers because it has to handle every use-case. |
We will likely end up doing something like this for libkrun 2.0. |
This change correctly triggers a build.rs rerun if the KRUN_*_BINARY_PATH environment variable has changed. Signed-off-by: Roland Rauch <roland@rauch.com>
|
@ggoodman I am also using my own I had to make some small changes to However, the |
This PR allows overriding the currently hard-coded paths of
INIT_BINARY(and alsoEDK2_BINARY) at build time.I've had an issue when using the
devicescrate from outside thelibkruntree:Currently, it's perfectly possible to include the crate in my own project (using the
gitcrate location type in myCargo.toml), but the build will fail because theinitbinary included invirtio::fs::<os>::passthroughcannot be found.To fix this, I've replaced
with
Now the
devicescrate can be built even when used out-of-tree. A nice side-effect is that this also allows the use of a custominitwhen desired.If no
INIT_BINARY_PATHenv variable is set, it falls back to the currently used path, so this PR should remain backwards compatible.I've noticed that
EDK2_BINARY(invmm) follows a similar approach, so I have updated that too - even though I am not currently usingaarch64. I have not tested this myself, but believe it should be fine.