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

[Marcus Folkesson <marcus.folkesson@gmail.com>]

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


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

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