Re: [PATCH v3 2/2] Move UKI creation to a separate derivation

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 6511 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> It will be used by the update code later.
>
> No functional change intended, other than a trivial shell script
> refactoring.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  host/efi.nix             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  pkgs/default.nix         |  1 +
>  release/live/Makefile    | 15 ++-------------
>  release/live/default.nix | 19 +++++--------------
>  4 files changed, 54 insertions(+), 27 deletions(-)
>
> diff --git a/host/efi.nix b/host/efi.nix
> new file mode 100644
> index 0000000000000000000000000000000000000000..a2b47fd050fbf00050473a0d5a1373eb96c341b5
> --- /dev/null
> +++ b/host/efi.nix
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: EUPL-1.2+

MIT for Nix files please.  (Fine to take my stuff from the EUPL-1.2+
Makefile and use it in a MIT-licensed Nix file.)

> +# SPDX-FileCopyrightText: 2021-2024 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +import ../lib/call-package.nix (
> +{ bash, callSpectrumPackage, cryptsetup, runCommand
> +, stdenv, systemdUkify, rootfs
> +}:
> +let
> +  initramfs = callSpectrumPackage ./initramfs {};
> +  kernel = "${rootfs.kernel}/${stdenv.hostPlatform.linux-kernel.target}";
> +  systemd = systemdUkify.overrideAttrs ({ mesonFlags ? [], ... }: {
> +    # The default limit is too low to build a generic aarch64 distro image:
> +    # https://github.com/systemd/systemd/pull/37417
> +    mesonFlags = mesonFlags ++ [ "-Defi-stub-extra-sections=3000" ];
> +  });
> +in
> +
> +runCommand "spectrum-efi" {
> +  nativeBuildInputs = [ cryptsetup systemd bash ];

bash?

> +  __structuredAttrs = true;
> +  unsafeDiscardReferences = { out = true; };
> +  dontFixup = true;
> +  passthru = { inherit systemd; };
> +  env = {
> +    DTBS = "${rootfs.kernel}/dtbs";
> +    KERNEL = kernel;
> +    INITRAMFS = initramfs;
> +    ROOTFS = rootfs;
> +  };

Usually we'd just inline these via string interpolation, rather than
passing them through as environment variables.

> diff --git a/pkgs/default.nix b/pkgs/default.nix
> index cc60228a10cddcb70e5ab9faa1bab7d74f3ebb35..c9f6dcfad9369567468b30d1c5697e3551a7b236 100644
> --- a/pkgs/default.nix
> +++ b/pkgs/default.nix
> @@ -36,6 +36,7 @@ let
>        path: (import path { inherit (self) callPackage; }).override;
>  
>      rootfs = self.callSpectrumPackage ../host/rootfs {};
> +    efi = self.callSpectrumPackage ../host/efi.nix {};
>      spectrum-build-tools = self.callSpectrumPackage ../tools {
>        appSupport = false;
>        buildSupport = true;

Generally images don't need entries here, and can just be loaded by
callSpectrumPackage.  There was a specific reason to make an exception
for rootfs (which I've now forgotten).

> diff --git a/release/live/Makefile b/release/live/Makefile
> index 191b44944af0adf965e1d5f2785719b236bfd99c..4de8743f42dec65aa863c3020cd70124316a6118 100644
> --- a/release/live/Makefile
> +++ b/release/live/Makefile
> @@ -19,19 +19,8 @@ $(dest): ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sf
>  build/empty:
>  	mkdir -p $@
>  
> -build/spectrum.efi: $(DTBS) $(KERNEL) $(INITRAMFS) $(ROOT_FS_VERITY_ROOTHASH)
> -	{ \
> -	    printf "[UKI]\nDeviceTreeAuto=" && \
> -	    find $(DTBS) -name '*.dtb' -print0 | tr '\0' ' ' ;\
> -	} | $(UKIFY) build \
> -	    --output $@ \
> -	    --config /dev/stdin \
> -	    --linux $(KERNEL) \
> -	    --initrd $(INITRAMFS) \
> -	    --os-release $$'NAME="Spectrum"\n' \
> -	    --cmdline "ro intel_iommu=on roothash=$$(cat "$$ROOT_FS_VERITY_ROOTHASH")"
> -
> -build/boot.fat: $(SYSTEMD_BOOT_EFI) build/spectrum.efi
> +build/boot.fat: $(SYSTEMD_BOOT_EFI) $(EFI_IMAGE) build/empty
> +	ln -sf -- "$$EFI_IMAGE" build/spectrum.efi
>  	$(TRUNCATE) -s 440401920 $@
>  	$(MKFS_FAT) $@
>  	$(MMD) -i $@ ::/EFI ::/EFI/BOOT ::/EFI/Linux

Why a symlink?  Why not just replace the path we copy from?

> diff --git a/release/live/default.nix b/release/live/default.nix
> index 9a62d4da9cfea11d94d2a1d5764d41587efd5ad5..c234d87e62cc9ae65ba60f94bab6e58b43beddbc 100644
> --- a/release/live/default.nix
> +++ b/release/live/default.nix
> @@ -6,7 +6,7 @@ import ../../lib/call-package.nix (
>  { callSpectrumPackage, spectrum-build-tools, rootfs, src
>  , lib, pkgsStatic, stdenvNoCC
>  , cryptsetup, dosfstools, jq, mtools, util-linux
> -, systemdUkify
> +, systemdUkify, efi
>  }:
>  
>  let
> @@ -14,13 +14,6 @@ let
>  
>    stdenv = stdenvNoCC;
>  
> -  systemd = systemdUkify.overrideAttrs ({ mesonFlags ? [], ... }: {
> -    # The default limit is too low to build a generic aarch64 distro image:
> -    # https://github.com/systemd/systemd/pull/37417
> -    mesonFlags = mesonFlags ++ [ "-Defi-stub-extra-sections=3000" ];
> -  });
> -
> -  initramfs = callSpectrumPackage ../../host/initramfs {};
>    efiArch = stdenv.hostPlatform.efiArch;
>  in
>  
> @@ -40,19 +33,17 @@ stdenv.mkDerivation {
>    sourceRoot = "source/release/live";
>  
>    nativeBuildInputs = [
> -    cryptsetup dosfstools jq spectrum-build-tools mtools systemd util-linux
> +    cryptsetup dosfstools jq spectrum-build-tools mtools util-linux
>    ];
>  
>    env = {
> -    INITRAMFS = initramfs;
>      KERNEL = "${rootfs.kernel}/${stdenv.hostPlatform.linux-kernel.target}";
>      ROOT_FS = "${rootfs}/rootfs";
>      ROOT_FS_VERITY = "${rootfs}/rootfs.verity.superblock";
>      ROOT_FS_VERITY_ROOTHASH = "${rootfs}/rootfs.verity.roothash";

Since efi is tied to a specific rootfs, maybe it would be nice to use
efi.rootfs here?

> -    SYSTEMD_BOOT_EFI = "${systemd}/lib/systemd/boot/efi/systemd-boot${efiArch}.efi";
> +    SYSTEMD_BOOT_EFI = "${efi.systemd}/lib/systemd/boot/efi/systemd-boot${efiArch}.efi";

We can just get this from the default systemd package.  Doesn't need to
be efi's special overridden one.

> +    EFI_IMAGE = efi;
>      EFINAME = "BOOT${toUpper efiArch}.EFI";
> -  } // lib.optionalAttrs stdenv.hostPlatform.linux-kernel.DTB or false {
> -    DTBS = "${rootfs.kernel}/dtbs";
>    };
>  
>    buildFlags = [ "dest=$(out)" ];
> @@ -65,6 +56,6 @@ stdenv.mkDerivation {
>    unsafeDiscardReferences = { out = true; };
>    dontFixup = true;
>  
> -  passthru = { inherit initramfs rootfs; };
> +  passthru = { inherit rootfs; };
>  }
>  ) (_: {})
>
> -- 
> 2.51.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]