build-sys: Rework sealing to be one build step #1898
Conversation
The `cp -a lbi/usr/. /usr` command was preserving local directory permissions which in one case for me have a restrictive mode 0750 which breaks booting. Switch to `install -D -m 0644` which explicitly sets file modes and creates parent directories with correct 755 permissions, while also simplifying the directory structure. Assisted-by: OpenCode (Opus 4.5) Signed-off-by: Colin Walters <[email protected]>
![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 is an excellent refactoring of the build system, unifying the standard and sealed image build processes into a single, multi-stage Docker build. This simplifies the build logic, improves maintainability, and likely enhances build caching. The changes are well-structured, breaking down the complex sealing process into smaller, dedicated scripts and Dockerfile stages. The removal of outdated files and the consolidation of logic into a single, more powerful Dockerfile is a significant cleanup. My review includes a few suggestions to improve the robustness of some of the new shell scripts to better handle cases where file globs might match more than one file.
| kver=$(cd /usr/lib/modules && echo *) | ||
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | ||
| echo "Error: No kernel found" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The command kver=$(cd /usr/lib/modules && echo *) is not robust if multiple kernel version directories exist under /usr/lib/modules. In such a case, kver would contain a space-separated list of versions, which would break subsequent commands. It's safer to ensure exactly one kernel directory is found and fail otherwise.
| kver=$(cd /usr/lib/modules && echo *) | |
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | |
| echo "Error: No kernel found" >&2 | |
| exit 1 | |
| fi | |
| kver_paths=(/usr/lib/modules/*) | |
| if [ "${#kver_paths[@]}" -ne 1 ]; then | |
| echo "Error: Expected 1 kernel version directory, but found ${#kver_paths[@]}" >&2 | |
| ls -l /usr/lib/modules/ | |
| exit 1 | |
| fi | |
| kver=$(basename "${kver_paths[0]}") |
| kver=$(cd "${target}/usr/lib/modules" && echo *) | ||
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | ||
| echo "Error: No kernel found" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The command kver=$(cd "${target}/usr/lib/modules" && echo *) is not robust if multiple kernel version directories exist. In such a case, kver would contain a space-separated list of versions, which would break subsequent commands. It's safer to ensure exactly one kernel directory is found and fail otherwise.
| kver=$(cd "${target}/usr/lib/modules" && echo *) | |
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | |
| echo "Error: No kernel found" >&2 | |
| exit 1 | |
| fi | |
| kver_paths=("${target}/usr/lib/modules"/*) | |
| if [ "${#kver_paths[@]}" -ne 1 ]; then | |
| echo "Error: Expected 1 kernel version directory, but found ${#kver_paths[@]}" >&2 | |
| ls -l "${target}/usr/lib/modules/" | |
| exit 1 | |
| fi | |
| kver=$(basename "${kver_paths[0]}") |
| sdboot_unsigned=$(ls ./usr/lib/systemd/boot/efi/systemd-boot*.efi) | ||
| sdboot_bn=$(basename ${sdboot_unsigned}) |
There was a problem hiding this comment.
The use of ls with a glob to find the systemd-boot EFI binary can be fragile. If the glob matches more than one file, the sdboot_unsigned variable will contain a multi-line string, which could cause sbsign to behave unexpectedly. It's safer to ensure exactly one file is found.
sdboot_unsigned_files=(./usr/lib/systemd/boot/efi/systemd-boot*.efi)
if [ ${#sdboot_unsigned_files[@]} -ne 1 ]; then
echo "Error: Expected 1 systemd-boot EFI file, but found ${#sdboot_unsigned_files[@]}" >&2
ls -l ./usr/lib/systemd/boot/efi/
exit 1
fi
sdboot_unsigned="${sdboot_unsigned_files[0]}"
sdboot_bn=$(basename "${sdboot_unsigned}")
| sdboot=$(ls /usr/lib/systemd/boot/efi/systemd-boot*.efi) | ||
| sdboot_bn=$(basename "${sdboot}") |
There was a problem hiding this comment.
The use of ls with a glob to find the systemd-boot EFI binary can be fragile. If the glob matches more than one file, the sdboot variable will contain a multi-line string. This would cause the install command on line 24 to fail because it would be interpreted as multiple destination arguments. It's safer to ensure exactly one file is found.
| sdboot=$(ls /usr/lib/systemd/boot/efi/systemd-boot*.efi) | |
| sdboot_bn=$(basename "${sdboot}") | |
| sdboot_files=(/usr/lib/systemd/boot/efi/systemd-boot*.efi) | |
| if [ "${#sdboot_files[@]}" -ne 1 ]; then | |
| echo "Error: Expected 1 systemd-boot EFI file, but found ${#sdboot_files[@]}" >&2 | |
| ls -l /usr/lib/systemd/boot/efi/ | |
| exit 1 | |
| fi | |
| sdboot="${sdboot_files[0]}" | |
| sdboot_bn=$(basename "${sdboot}") |
Now that we're doing a "from scratch" build we don't have the mtime issue, and so we can change our build system to do everything in a single step. Assisted-by: OpenCode (Opus 4.5) Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
sealed-scratch-rebase
branch
from
0ba5f48 to
4c7882c
Compare
OK this was working for me at one point but then broke and it took me quite a while to figure out why: containers/storage#743 (comment) |
|
OK this will depend on more composefs-rs work so we need to get back on git main, which is #1791 |
1 participant
Now that we're doing a "from scratch" build we don't
have the mtime issue, and so we can change our build system
to do everything in a single step.
Assisted-by: OpenCode (Opus 4.5)