-
Notifications
You must be signed in to change notification settings - Fork 129
Add initramfs infrastructure #1500
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
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.
Code Review
This pull request adds the basic infrastructure for an initramfs binary. The changes look good overall, introducing a new crate, systemd service, dracut module, and build system support. I've identified a few issues in the Dockerfile scripting and Rust argument parsing that could lead to incorrect behavior. My comments include suggestions to make the logic more robust and correct.
Dockerfile
Outdated
if test -n "${initramfs:-}"; then | ||
make install-initramfs-dracut DESTDIR=/out | ||
fi |
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 condition test -n "${initramfs:-}"
is likely not what you intend. It evaluates to true for any non-empty value of initramfs
, including the default 0
. This means the initramfs code will be installed even when initramfs
is 0
(the default). To disable it, one would have to pass --build-arg initramfs=
, which is not intuitive given the comment "Flip this on to enable initramfs code".
A more explicit check would be to test if the value is not 0
.
if [ "${initramfs:-0}" -ne 0 ]; then
make install-initramfs-dracut DESTDIR=/out
fi
This adds scaffolding to install a stub binary which can optionally be added into the initramfs; prep for us doing real work during setup as we aim to move to the native composefs backend. The binary is *built* but is only installed by a new `Makefile` target, so existing build system users won't pick it up. Our development-only `Dockerfile` gains a build option to use it (and also ensures the initramfs is regenerated). However previously we also discussed moving the fstab logic into the initramfs: bootc-dev#1113 I might try doing that once this lands. One notable thing is that even this trivial nearly-no-op binary is still 4MB which I think is mostly due to linking in a whole copy of prebuilt rust `std`. In theory we could try going to `#[no_std]` but I don't think it'll be viable once we start doing more here. Probably most practical thing re size is `-Z build-std` + LTO. Signed-off-by: Colin Walters <[email protected]>
6d0a45d
to
f61ba60
Compare
FWIW on 1.89 with release build, Edit: and after stripping, it's down to 378K |
With build-std and LTO it only drops to 354K but takes waaaaaay longer to build. |
Oh duh of course I forgot we had |
This adds scaffolding to install a stub binary which can optionally be added into the initramfs;
prep for us doing real work during setup as we aim to move to the native composefs backend.
The binary is built but is only installed by a
new
Makefile
target, so existing build systemusers won't pick it up. Our development-only
Dockerfile
gains a build option to use it(and also ensures the initramfs is regenerated).
However previously we also discussed moving the fstab logic into the initramfs:
#1113
I might try doing that once this lands.
One notable thing is that even this trivial nearly-no-op binary is still 4MB which I think is mostly due
to linking in a whole copy of prebuilt rust
std
. In theory we could try going to#[no_std]
but Idon't think it'll be viable once we start doing more here. Probably most practical thing re size is
-Z build-std
+ LTO.