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.folkes...@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.folkes...@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
signature.asc
Description: PGP signature
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#199953): https://lists.openembedded.org/g/openembedded-core/message/199953 Mute This Topic: https://lists.openembedded.org/mt/106295832/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
