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 > > [email protected]. 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 <[email protected]> > > --- > > 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 <[email protected]> > > +# > > +# 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
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#199797): https://lists.openembedded.org/g/openembedded-core/message/199797 Mute This Topic: https://lists.openembedded.org/mt/106227725/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
