Re: [docs] [PATCH v3] classes.rst variables.rst: add documentation for uki.bbclass

[Mikko Rapeli <mikko.rapeli@linaro.org>]

Hi,

On Wed, Nov 27, 2024 at 11:26:56AM +0100, Antonin Godard wrote:
> Hi Mikko,
> 
> Thanks for taking the time to document your class. In general it looks very good
> to me.
> 
> A nitpick from the documentation/standards.md of the doc is that we prefer
> wrapping the lines to 80 chars, as is done in the rest of the document. Could
> you do this?

Done

> Otherwise my comments are below.
> 
> On Tue Nov 26, 2024 at 12:02 PM CET, Mikko Rapeli via lists.yoctoproject.org wrote:
> > Documentation for the new class.
> >
> > [YOCTO #15650]
> > https://bugzilla.yoctoproject.org/show_bug.cgi?id=15650
> >
> > Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> > ---
> >  documentation/ref-manual/classes.rst   | 34 ++++++++++++++++++++++++++
> >  documentation/ref-manual/variables.rst | 32 ++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/documentation/ref-manual/classes.rst b/documentation/ref-manual/classes.rst
> > index b92f4e4f20..d2b2469552 100644
> > --- a/documentation/ref-manual/classes.rst
> > +++ b/documentation/ref-manual/classes.rst
> > @@ -3345,6 +3345,40 @@ and the `signature process
> >  See also the description of :ref:`ref-classes-kernel-fitimage` class, which this class
> >  imitates.
> >  
> > +.. _ref-classes-uki:
> > +
> > +``uki``
> > +=======
> > +
> > +The :ref:`ref-classes-uki` class provides support for `Unified Kernel Image (UKI) <https://uapi-group.org/specifications/specs/unified_kernel_image/>`__
> > +format. UKIs combine kernel, :term:`Initramfs`, signatures, metadata etc to a single UEFI firmware
> > +compatible binary. The class is intended to be inherited by rootfs image recipes. The build configuration
> > +should also use an initramfs, ``systemd-boot`` as boot menu provider and have UEFI support on target
> > +HW. Using ``systemd`` as init is recommended. Image builds should create an ESP partition
> 
> s/HW/hardware/

Done

> > +for UEFI firmware and copy ``systemd-boot`` and UKI files there. Sample configuration for Wic
> > +images is provided in :oe_git:`scripts/lib/wic/canned-wks/efi-uki-bootdisk.wks.in <openembedded-core/tree/scripts/lib/wic/canned-wks/efi-uki-bootdisk.wks.in>`.
> > +UKIs are generated using ``systemd`` reference implementation `ukify <https://www.freedesktop.org/software/systemd/man/latest/ukify.html>`__.
> > +This class uses a number of variables but tries to find sensible defaults for them.
> > +
> > +The variables used by this class are:
> > +
> > +-  :term:`EFI_ARCH`: architecture name within EFI standard, set in :oe_git:`meta/conf/image-uefi.conf <openembedded-core/tree/meta/conf/image-uefi.conf>`
> 
> We are missing `EFI_ARCH` from variables.rst, so this :term: won't work. Is that
> something you could add to variables.rst? Since you briefly document it here it
> should be a small addition.

Done

> > +-  :term:`IMAGE_EFI_BOOT_FILES`: files to install to EFI boot partition created by ``bootimg-efi`` Wic tool
> 
> s/by ``bootimg-efi``/by the ``bootimg-efi``/
> 
> It is a plugin rather than a tool, I think? s/Wic tool/Wic plugin/

Done

> > +-  :term:`INITRAMFS_IMAGE`: initramfs recipe name
> > +-  :term:`INITRD_ARCHIVE`: initramfs image file name
> 
> Same for this variable, :term: doesn't work here. Since this variable is local
> to this file, I'm not sure it should be part of variables.rst, unless overriding
> it makes sense (I see ?= is used for it).

Done, removed. It can be overwritten if needed but is internal to the file.

> > +-  :term:`KERNEL_DEVICETREE`: optional devicetree files to embed into UKI
> > +-  :term:`UKIFY_CMD`: `ukify <https://www.freedesktop.org/software/systemd/man/latest/ukify.html>`__ command to build UKI image
> 
> s/build/build the/

Done

> > +-  :term:`UKI_CMDLINE`: kernel command line to use with UKI
> > +-  :term:`UKI_CONFIG_FILE`: optional config file for `ukify <https://www.freedesktop.org/software/systemd/man/latest/ukify.html>`__
> > +-  :term:`UKI_FILENAME`: output file name for UKI image
> 
> s/for/for the/

Done

> > +-  :term:`UKI_KERNEL_FILENAME`: kernel image file name
> > +-  :term:`UKI_SB_CERT`: optional UEFI secureboot certificate mathing the private key
> 
> s/mathing/matching/

Done

> > +-  :term:`UKI_SB_KEY`: optional UEFI secureboot private key to sign UKI with
> > +
> > +For examples on how to use this class see oeqa selftest :oe_git:`meta/lib/oeqa/selftest/cases/uki.py <openembedded-core/tree/meta/lib/oeqa/selftest/cases/uki.py>`.
> > +Also an oeqa runtime test ``uki`` is provided which verifies that the target system
> 
> Add a link to meta/lib/oeqa/runtime/cases/uki.py, I guess that's the one?

Done

> > +booted the same UKI binary as was set at buildtime via :term:`UKI_FILENAME`.
> > +
> >  .. _ref-classes-uninative:
> >  
> >  ``uninative``
> > diff --git a/documentation/ref-manual/variables.rst b/documentation/ref-manual/variables.rst
> > index ec4d7ab73f..65fea43a1e 100644
> > --- a/documentation/ref-manual/variables.rst
> > +++ b/documentation/ref-manual/variables.rst
> > @@ -9846,6 +9846,38 @@ system and gives an overview of their function and contents.
> >        passes and uses "all" for the target during the U-Boot building
> >        process.
> >  
> > +   :term:`UKIFY_CMD`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      `ukify <https://www.freedesktop.org/software/systemd/man/latest/ukify.html>`__ command to build
> > +      `Unified Kernel Image (UKI) <https://uapi-group.org/specifications/specs/unified_kernel_image/>`__.
> > +      Defaults to ``ukify build``.
> > +
> > +   :term:`UKI_CMDLINE`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      the kernel command line to use when booting the `Unified Kernel Image (UKI) <https://uapi-group.org/specifications/specs/unified_kernel_image/>`__.
> > +      Defaults to ``rootwait root=LABEL=root console=${KERNEL_CONSOLE}``.
> > +
> > +   :term:`UKI_CONFIG_FILE`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      an optional config file for `ukify <https://www.freedesktop.org/software/systemd/man/latest/ukify.html>`__ command.
> 
> s/for/for the/

Done

> > +   :term:`UKI_FILENAME`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      the output file name for generated `Unified Kernel Image (UKI) <https://uapi-group.org/specifications/specs/unified_kernel_image/>`__.
> 
> s/for/for the/

Done

> > +      Defaults to ``uki.efi``.
> > +
> > +   :term:`UKI_KERNEL_FILENAME`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      the kernel image file name to use as input. Defaults to :term:`KERNEL_IMAGETYPE`.
> > +
> > +   :term:`UKI_SB_CERT`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      optional UEFI secureboot certificate matching the private key in :term:`UKI_SB_KEY`.
> > +
> > +   :term:`UKI_SB_KEY`
> > +      When inheriting the :ref:`ref-classes-uki` class,
> > +      optional UEFI secureboot private key to sign `Unified Kernel Image (UKI) <https://uapi-group.org/specifications/specs/unified_kernel_image/>`__.
> 
> s/sign/sign the/

Done

Will send a v4.

Cheers,

-Mikko