Re: [OE-core] [PATCH 1/2] image-bootfiles.bbclass: new class, copy boot files to /boot on rfs

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

Hello Quentin,

First, thank you for your feedback.

On Thu, May 23, 2024 at 03:26:36PM +0200, Quentin Schulz wrote:
> Hi Markus,
> 
> On 5/21/24 7:33 PM, Marcus Folkesson via lists.openembedded.org wrote:
> > [You don't often get email from marcus.folkesson=gmail.com@lists.openembedded.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > image-bootfiles class copy files listed IMAGE_BOOT_FILES
> > to the /boot 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 | 69 ++++++++++++++++++++++++++++
> >   1 file changed, 69 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..850e14e4bb
> > --- /dev/null
> > +++ b/meta/classes/image-bootfiles.bbclass
> > @@ -0,0 +1,69 @@
> > +#
> > +# Writes IMAGE_BOOT_FILES to the /boot directory
> > +#
> > +# Copyright (C) 2024 Marcus Folkesson
> > +# Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +#
> > +# Licensed under the MIT license, see COPYING.MIT for details
> > +# Inspired of bootimg-partition.py
> > +#
> > +# Usage: add INHERIT += "image-bootfiles" to your conf file
> > +#
> 
> Since it's in meta/classes, I assume we can also inherit image-bootfiles in
> an image recipe?

Yes, that is how I do it right now.

> 
> > +
> > +def bootfiles_populate(d):
> > +    import re
> > +    from glob import glob
> > +    import shutil
> > +
> > +    boot_files = d.getVar("IMAGE_BOOT_FILES")
> > +    deploy_image_dir = d.getVar("DEPLOY_DIR_IMAGE")
> > +    boot_dir = d.getVar("IMAGE_ROOTFS") + "/boot"
> > +    install_files = []
> > +
> > +    if boot_files is None:
> > +        return
> > +
> > +    # list of tuples (src_name, dst_name)
> > +    deploy_files = []
> > +    for src_entry in re.findall(r'[\w;\-\./\*]+', boot_files):
> 
> Isn't this a more complex boot_files.split()? What do we gain from using
> this regex? If we have malformed IMAGE_BOOT_FILES, this would actually
> introduce some logic error:

Probably it is. The code is heavily based on the bootimg_partition [1]
wic plugin, and this regex comes from there.
The rationale to keep it was that it is probably well-tested already and
I want to keep the same behavior as bootimg_partition as I want it to be
interchangable.

> 
> >>> s="plop\\plip;plep plop;plap"
> >>> re.findall(r'[\w;\-\./\*]+', s)
> ['plop', 'plip;plep', 'plop;plap']
> 
> I'm pretty sure we don't want that. Instead, I would suggest to use split()
> and iterate over that, split(';'), check for each if there's 1 or 2 entries,
> add one if it's one.

Looks better to me. Thanks

> 
> > +        if ';' in src_entry:
> > +            dst_entry = tuple(src_entry.split(';'))
> > +            if not dst_entry[0] or not dst_entry[1]:
> > +                raise bb.parse.SkipRecipe('Malformed boot file entry: %s' % src_entry)
> > +        else:
> > +            dst_entry = (src_entry, src_entry)
> > +
> > +        deploy_files.append(dst_entry)
> > +
> > +    for deploy_entry in deploy_files:
> > +        src, dst = deploy_entry
> > +        if '*' in src:
> > +            # by default install files under their basename
> > +            entry_name_fn = os.path.basename
> > +            if dst != src:
> > +                # unless a target name was given, then treat name
> > +                # as a directory and append a basename
> > +                entry_name_fn = lambda name: \
> > +                                os.path.join(dst,
> > +                                             os.path.basename(name))
> > +
> > +
> > +                srcs = glob(os.path.join(deploy_image_dir, src))
> > +                for entry in srcs:
> > +                    src = os.path.relpath(entry, deploy_mage_dir)
> > +                    entry_dst_name = entry_name_fn(entry)
> > +                    install_files.append((src, entry_dst_name))
> > +            else:
> > +                install_files.append((src, dst))
> > +
> > +    for entry in install_files:
> > +        src, dst = entry
> > +        image_src = os.path.join(deploy_image_dir, src)
> > +        image_dst = os.path.join(boot_dir, dst)
> > +        shutil.copyfile(image_src, image_dst)
> > +
> 
> We literally iterate three times over the same files, I don't see a reason
> for not merging those three for loops into one?

Yes, that is probably better.

> 
> Also, I now realize that this was heavily inspired from what's in
> scripts/lib/wic/plugins/source/bootimg-partition.py, would have been nice to
> say so ;)

Yes it is, I have this text in the header

+#
+# Licensed under the MIT license, see COPYING.MIT for details
+# Inspired of bootimg-partition.py

Maybe I should make it more clear?

> 
> > +python bootfiles () {
> > +   bootfiles_populate(d),
> 
> Not entirely convinced we should have a comma after bootfiles_populate here?
> 
> More general question:
> 1) how does this work if one has a boot partition created by wic (via the
> plugin bootimg-partition?

The images will be copied to both the partition and /boot.

> 2) shouldn't we factor out this code here and bootimg-partition's? or make
> bootimg-partition somehow call bootfiles IMAGE_PREPROCESS_COMMAND to avoid
> code duplication?

I would love that. Not sure where to but it though, I'm still learning
the structure of poky.

I'm preparing a v2 with an indentation error and that the installation directory is configurable so
that it does not *have* to be /boot.
But I will probably wait one or two more daysto give people a chance to
give their comments.

> 
> Cheers,
> Quentin

[1] https://github.com/yoctoproject/poky/blob/master/scripts/lib/wic/plugins/source/bootimg-partition.py#L71

Best regards,
Marcus Folkesson