Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
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 <walters@verbum.org>
cgwalters
force-pushed
the
initramfs-bit
branch
from
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 |
2 participants
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
Makefiletarget, so existing build systemusers won't pick it up. Our development-only
Dockerfilegains 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.