[PATCH v2 0/2] image-bootfiles: new class

[PATCH v2 0/2] image-bootfiles: new class

[Marcus Folkesson]

The image-bootfiles class is used to put all files listed in
IMAGE_BOOT_FILES into the root filesystem.

IMAGE_BOOT_FILES is used by the bootimg-partition wic plugin to put the
files into a boot partition. 
Be able to list files as "boot files" in e.g. your .conf or image files
instead of install those in every recipe is a good thing.

It is not always desired to have a separate boot partition for boot
files. Sometimes it could be good to have them as a part of the root
filesystem.

For example, if a double copy strategy is used for update the system,
then you probably want to update both the boot files and root filesystem
at the same time as there may be dependencies.

v2:
    - Removed the documentation from the patch series (will be submitted later)
    - Break out the parts in bootimg-partition that is used by
      image-bootfiles to a common library
    - Make the destination directory in root filesystem configurable

Marcus Folkesson (2):
  bootimg-partition: break out code to a common library.
  image-bootfiles.bbclass: new class, copy boot files to root filesystem

 meta/classes/image-bootfiles.bbclass          | 38 +++++++++++++
 meta/lib/oe/bootfiles.py                      | 56 +++++++++++++++++++
 .../wic/plugins/source/bootimg-partition.py   | 39 +------------
 3 files changed, 97 insertions(+), 36 deletions(-)
 create mode 100644 meta/classes/image-bootfiles.bbclass
 create mode 100644 meta/lib/oe/bootfiles.py

-- 
2.44.0

[PATCH v2 1/2] bootimg-partition: break out code to a common library.

[Marcus Folkesson]

Break out the code that parse IMAGE_BOOT_FILES to a common library.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 meta/lib/oe/bootfiles.py                      | 56 +++++++++++++++++++
 .../wic/plugins/source/bootimg-partition.py   | 39 +------------
 2 files changed, 59 insertions(+), 36 deletions(-)
 create mode 100644 meta/lib/oe/bootfiles.py

diff --git a/meta/lib/oe/bootfiles.py b/meta/lib/oe/bootfiles.py
new file mode 100644
index 0000000000..81e09f138e
--- /dev/null
+++ b/meta/lib/oe/bootfiles.py
@@ -0,0 +1,56 @@
+#
+# SPDX-License-Identifier: MIT
+#
+# Copyright (C) 2024 Marcus Folkesson
+# Author: Marcus Folkesson <marcus.folkesson@gmail.com>
+#
+# Utility functions handling boot files
+
+# Look into deploy_dir and search for boot_files.
+# Returns a list with files to copy.
+#
+# Heavily inspired of bootimg-partition.py
+#
+def get_boot_files(deploy_dir, boot_files):
+    import re
+    import os
+    from glob import glob
+
+    if boot_files is None:
+        return None
+
+    # list of tuples (src_name, dst_name)
+    deploy_files = []
+    for src_entry in re.findall(r'[\w;\-\./\*]+', boot_files):
+        if ';' in src_entry:
+            dst_entry = tuple(src_entry.split(';'))
+            if not dst_entry[0] or not dst_entry[1]:
+                raise ValueError('Malformed boot file entry: %s' % src_entry)
+        else:
+            dst_entry = (src_entry, src_entry)
+
+        deploy_files.append(dst_entry)
+
+    install_files = []
+    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_dir, src))
+
+            for entry in srcs:
+                src = os.path.relpath(entry, deploy_dir)
+                entry_dst_name = entry_name_fn(entry)
+                install_files.append((src, entry_dst_name))
+        else:
+            install_files.append((src, dst))
+
+    return install_files
diff --git a/scripts/lib/wic/plugins/source/bootimg-partition.py b/scripts/lib/wic/plugins/source/bootimg-partition.py
index 1071d1af3f..b22a448b65 100644
--- a/scripts/lib/wic/plugins/source/bootimg-partition.py
+++ b/scripts/lib/wic/plugins/source/bootimg-partition.py
@@ -18,6 +18,8 @@ import re
 
 from glob import glob
 
+from oe.bootfiles import get_boot_files
+
 from wic import WicError
 from wic.engine import get_custom_config
 from wic.pluginbase import SourcePlugin
@@ -66,42 +68,7 @@ class BootimgPartitionPlugin(SourcePlugin):
 
         logger.debug('Boot files: %s', boot_files)
 
-        # list of tuples (src_name, dst_name)
-        deploy_files = []
-        for src_entry in re.findall(r'[\w;\-\./\*]+', boot_files):
-            if ';' in src_entry:
-                dst_entry = tuple(src_entry.split(';'))
-                if not dst_entry[0] or not dst_entry[1]:
-                    raise WicError('Malformed boot file entry: %s' % src_entry)
-            else:
-                dst_entry = (src_entry, src_entry)
-
-            logger.debug('Destination entry: %r', dst_entry)
-            deploy_files.append(dst_entry)
-
-        cls.install_task = [];
-        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(kernel_dir, src))
-
-                logger.debug('Globbed sources: %s', ', '.join(srcs))
-                for entry in srcs:
-                    src = os.path.relpath(entry, kernel_dir)
-                    entry_dst_name = entry_name_fn(entry)
-                    cls.install_task.append((src, entry_dst_name))
-            else:
-                cls.install_task.append((src, dst))
-
+        cls.install_task = get_boot_files(kernel_dir, boot_files)
         if source_params.get('loader') != "u-boot":
             return
 
-- 
2.44.0

Re: [PATCH v2 1/2] bootimg-partition: break out code to a common library.

[Quentin Schulz]

Hi Marcus,

On 5/25/24 10:50 AM, Marcus Folkesson wrote:
> Break out the code that parse IMAGE_BOOT_FILES to a common library.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>   meta/lib/oe/bootfiles.py                      | 56 +++++++++++++++++++
>   .../wic/plugins/source/bootimg-partition.py   | 39 +------------
>   2 files changed, 59 insertions(+), 36 deletions(-)
>   create mode 100644 meta/lib/oe/bootfiles.py
> 
> diff --git a/meta/lib/oe/bootfiles.py b/meta/lib/oe/bootfiles.py
> new file mode 100644
> index 0000000000..81e09f138e
> --- /dev/null
> +++ b/meta/lib/oe/bootfiles.py
> @@ -0,0 +1,56 @@
> +#
> +# SPDX-License-Identifier: MIT
> +#
> +# Copyright (C) 2024 Marcus Folkesson
> +# Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +#
> +# Utility functions handling boot files
> +
> +# Look into deploy_dir and search for boot_files.
> +# Returns a list with files to copy.
> +#

It returns a list of tuples with (original filepath relative to 
deploy_dir, desired filepath renaming) if I read the code properly, I 
think this is really important information.

> +# Heavily inspired of bootimg-partition.py
> +#
> +def get_boot_files(deploy_dir, boot_files):
> +    import re
> +    import os
> +    from glob import glob
> +
> +    if boot_files is None:
> +        return None
> +
> +    # list of tuples (src_name, dst_name)
> +    deploy_files = []
> +    for src_entry in re.findall(r'[\w;\-\./\*]+', boot_files):
> +        if ';' in src_entry:
> +            dst_entry = tuple(src_entry.split(';'))
> +            if not dst_entry[0] or not dst_entry[1]:
> +                raise ValueError('Malformed boot file entry: %s' % src_entry)
> +        else:
> +            dst_entry = (src_entry, src_entry)
> +
> +        deploy_files.append(dst_entry)
> +
> +    install_files = []
> +    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_dir, src))
> +
> +            for entry in srcs:
> +                src = os.path.relpath(entry, deploy_dir)
> +                entry_dst_name = entry_name_fn(entry)
> +                install_files.append((src, entry_dst_name))
> +        else:
> +            install_files.append((src, dst))
> +
> +    return install_files
> diff --git a/scripts/lib/wic/plugins/source/bootimg-partition.py b/scripts/lib/wic/plugins/source/bootimg-partition.py
> index 1071d1af3f..b22a448b65 100644
> --- a/scripts/lib/wic/plugins/source/bootimg-partition.py
> +++ b/scripts/lib/wic/plugins/source/bootimg-partition.py
> @@ -18,6 +18,8 @@ import re
>   
>   from glob import glob
>   

You should be able to remove this import as it was only used in the code 
that is now moved to the lib.

Otherwise looks good to me, so

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks,
Quentin

Re: [PATCH v2 1/2] bootimg-partition: break out code to a common library.

[Marcus Folkesson]

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

Hi Quentin,

On Mon, May 27, 2024 at 05:20:53PM +0200, Quentin Schulz wrote:
> Hi Marcus,

[...]

> 
> > --- /dev/null
> > +++ b/meta/lib/oe/bootfiles.py
> > @@ -0,0 +1,56 @@
> > +#
> > +# SPDX-License-Identifier: MIT
> > +#
> > +# Copyright (C) 2024 Marcus Folkesson
> > +# Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +#
> > +# Utility functions handling boot files
> > +
> > +# Look into deploy_dir and search for boot_files.
> > +# Returns a list with files to copy.
> > +#
> 
> It returns a list of tuples with (original filepath relative to deploy_dir,
> desired filepath renaming) if I read the code properly, I think this is
> really important information.

Agree. I will add that to the comment.


[...]

> >   from glob import glob
> 
> You should be able to remove this import as it was only used in the code
> that is now moved to the lib.

True, I will remove the import.

> 
> Otherwise looks good to me, so
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks,
> Quentin

Best regards,
Marcus Folkesson

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

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

[Marcus Folkesson]

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
+#
+
+IMAGE_BOOTFILES_DIR ?= "/boot"
+
+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")
+
+    install_files = get_boot_files(deploy_image_dir, boot_files)
+    if install_files is None:
+        return
+
+    os.makedirs(boot_dir, exist_ok=True)
+    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)
+
+python bootfiles () {
+   bootfiles_populate(d),
+}
+
+IMAGE_PREPROCESS_COMMAND += "bootfiles;"
-- 
2.44.0

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

[Quentin Schulz]

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

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

[Marcus Folkesson]

[-- 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 --]