[v2,2/2] image-bootfiles.bbclass: new class, copy boot files to root filesystem

image-bootfiles class copy files listed in IMAGE_BOOT_FILES
to the IMAGE_BOOTFILES_DIR directory of the root filesystem.

This is useful when there is no explicit boot partition but all boot
files should instead reside inside the root filesystem.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 meta/classes/image-bootfiles.bbclass | 38 ++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 meta/classes/image-bootfiles.bbclass

Hi Marcus,

On 5/25/24 10:50 AM, Marcus Folkesson wrote:
> image-bootfiles class copy files listed in IMAGE_BOOT_FILES
> to the IMAGE_BOOTFILES_DIR directory of the root filesystem.
> 
> This is useful when there is no explicit boot partition but all boot
> files should instead reside inside the root filesystem.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>   meta/classes/image-bootfiles.bbclass | 38 ++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>   create mode 100644 meta/classes/image-bootfiles.bbclass
> 
> diff --git a/meta/classes/image-bootfiles.bbclass b/meta/classes/image-bootfiles.bbclass
> new file mode 100644
> index 0000000000..29a38ac631
> --- /dev/null
> +++ b/meta/classes/image-bootfiles.bbclass
> @@ -0,0 +1,38 @@
> +#
> +# SPDX-License-Identifier: MIT
> +#
> +# Copyright (C) 2024 Marcus Folkesson
> +# Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +#
> +# Writes IMAGE_BOOT_FILES to the IMAGE_BOOTFILES_DIR directory
> +#
> +#
> +# Usage: add INHERIT += "image-bootfiles" to your image
> +#

One is supposed to use inherit image-bootfiles inside recipes. INHERIT 
is for global inherit, from configuration files, e.g. INHERIT += "rm_work".

> +
> +IMAGE_BOOTFILES_DIR ?= "/boot"
> +

I would really suggest to have the exact same prefix as for the 
IMAGE_BOOT_FILES variable since they are related. I would like to avoid 
the DEPLOYDIR, DEPLOY_DIR confusion for example :)

> +def bootfiles_populate(d):
> +    import shutil
> +    from oe.bootfiles import get_boot_files
> +
> +    boot_files = d.getVar("IMAGE_BOOT_FILES")
> +    deploy_image_dir = d.getVar("DEPLOY_DIR_IMAGE")
> +    boot_dir = d.getVar("IMAGE_ROOTFS") + d.getVar("IMAGE_BOOTFILES_DIR")
> +

I would suggest to use
os.path.join(d.getVar("IMAGE_ROOTFS"), d.getVar("IMAGE_BOOTFILES_DIR"))
here to make it a tiny bit more robust to missing/redundant slashes (/) 
for example.

> +    install_files = get_boot_files(deploy_image_dir, boot_files)
> +    if install_files is None:
> +        return
> +

I'm wondering if we shouldn't print a warning or at the very least a 
note if boot_files is not empty but install_files is, this seems like 
there could be an issue no?

> +    os.makedirs(boot_dir, exist_ok=True)
> +    for entry in install_files:
> +        src, dst = entry

Those two lines could be merged into:

for src, dst in install_files:

I believe.

> +        image_src = os.path.join(deploy_image_dir, src)
> +        image_dst = os.path.join(boot_dir, dst)
> +        shutil.copyfile(image_src, image_dst)
> +
> +python bootfiles () {
> +   bootfiles_populate(d),

I'm surprised by the comma at the end of the line, is this required 
somehow? What does this do?

I'm also not entirely sure we need to have this additional indirection? Is
"""
python bootfiles_populate() {
     <code from def bootfiles_populate(d):>
}
IMAGE_PREPROCESS_COMMAND += "bootfiles_populate;"
"""

not working somehow (I don't know if it should, so just asking)? 
tinyinitrd in meta/recipes-core/images/core-image-tiny-initramfs.bb 
seems to be doing it, so should/could be possible?

Cheers,
Quentin

Hi Quentin,

Thank you for your in-depth review.
It is probably obvious that Python is not my mother tounge, so all
feedback is appreciated :-)

On Mon, May 27, 2024 at 05:29:40PM +0200, Quentin Schulz wrote:
> Hi Marcus,
> 
> On 5/25/24 10:50 AM, Marcus Folkesson wrote:
> > image-bootfiles class copy files listed in IMAGE_BOOT_FILES
> > to the IMAGE_BOOTFILES_DIR directory of the root filesystem.
> > 
> > This is useful when there is no explicit boot partition but all boot
> > files should instead reside inside the root filesystem.
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >   meta/classes/image-bootfiles.bbclass | 38 ++++++++++++++++++++++++++++
> >   1 file changed, 38 insertions(+)
> >   create mode 100644 meta/classes/image-bootfiles.bbclass
> > 
> > diff --git a/meta/classes/image-bootfiles.bbclass b/meta/classes/image-bootfiles.bbclass
> > new file mode 100644
> > index 0000000000..29a38ac631
> > --- /dev/null
> > +++ b/meta/classes/image-bootfiles.bbclass
> > @@ -0,0 +1,38 @@
> > +#
> > +# SPDX-License-Identifier: MIT
> > +#
> > +# Copyright (C) 2024 Marcus Folkesson
> > +# Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +#
> > +# Writes IMAGE_BOOT_FILES to the IMAGE_BOOTFILES_DIR directory
> > +#
> > +#
> > +# Usage: add INHERIT += "image-bootfiles" to your image
> > +#
> 
> One is supposed to use inherit image-bootfiles inside recipes. INHERIT is
> for global inherit, from configuration files, e.g. INHERIT += "rm_work".

Got it, thanks.

> 
> > +
> > +IMAGE_BOOTFILES_DIR ?= "/boot"
> > +
> 
> I would really suggest to have the exact same prefix as for the
> IMAGE_BOOT_FILES variable since they are related. I would like to avoid the
> DEPLOYDIR, DEPLOY_DIR confusion for example :)

Good point.

> 
> > +def bootfiles_populate(d):
> > +    import shutil
> > +    from oe.bootfiles import get_boot_files
> > +
> > +    boot_files = d.getVar("IMAGE_BOOT_FILES")
> > +    deploy_image_dir = d.getVar("DEPLOY_DIR_IMAGE")
> > +    boot_dir = d.getVar("IMAGE_ROOTFS") + d.getVar("IMAGE_BOOTFILES_DIR")
> > +
> 
> I would suggest to use
> os.path.join(d.getVar("IMAGE_ROOTFS"), d.getVar("IMAGE_BOOTFILES_DIR"))
> here to make it a tiny bit more robust to missing/redundant slashes (/) for
> example.

That is better.

> 
> > +    install_files = get_boot_files(deploy_image_dir, boot_files)
> > +    if install_files is None:
> > +        return
> > +
> 
> I'm wondering if we shouldn't print a warning or at the very least a note if
> boot_files is not empty but install_files is, this seems like there could be
> an issue no?

Sounds good, I will add a print.

> 
> > +    os.makedirs(boot_dir, exist_ok=True)
> > +    for entry in install_files:
> > +        src, dst = entry
> 
> Those two lines could be merged into:
> 
> for src, dst in install_files:
> 
> I believe.

Yep, it worked.

> 
> > +        image_src = os.path.join(deploy_image_dir, src)
> > +        image_dst = os.path.join(boot_dir, dst)
> > +        shutil.copyfile(image_src, image_dst)
> > +
> > +python bootfiles () {
> > +   bootfiles_populate(d),
> 
> I'm surprised by the comma at the end of the line, is this required somehow?
> What does this do?

No it is not. It is a remnant from my debugging. Good catch though.

> 
> I'm also not entirely sure we need to have this additional indirection? Is
> """
> python bootfiles_populate() {
>     <code from def bootfiles_populate(d):>
> }
> IMAGE_PREPROCESS_COMMAND += "bootfiles_populate;"
> """
> 
> not working somehow (I don't know if it should, so just asking)? tinyinitrd
> in meta/recipes-core/images/core-image-tiny-initramfs.bb seems to be doing
> it, so should/could be possible?

I change it as suggested. bootfiles() did more things before I cleaned
it up, so it is most of a remnant.

> 
> Cheers,
> Quentin


Best regards,
Marcus Folkesson