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

[PATCH v3 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
v3:
    - See changelog in patches

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.45.1

[PATCH v3 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>
Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
---

Notes:
    v3:
       - Removed unnecessary "import glob"
       - Explicitely tell that bootfiles_populate() returns a tuple

 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..666141df4e
--- /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 of tuples with (original filepath relative to
+# deploy_dir, desired filepath renaming)
+#
+# Heavily inspired of bootimg-partition.py
+#
+def get_boot_files(deploy_dir, boot_files):
+    import re
+    import os
+
+    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.45.1

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

[Quentin Schulz]

Hi Marcus,

On 5/28/24 11:08 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>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
> 
> Notes:
>      v3:
>         - Removed unnecessary "import glob"

Ah, there was a misunderstanding here, the "wrong" import glob got 
removed :)

>         - Explicitely tell that bootfiles_populate() returns a tuple
> 
>   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..666141df4e
> --- /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 of tuples with (original filepath relative to
> +# deploy_dir, desired filepath renaming)
> +#
> +# Heavily inspired of bootimg-partition.py
> +#
> +def get_boot_files(deploy_dir, boot_files):
> +    import re
> +    import os
> +

We need from glob import glob here........

> +    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))
> +

.... otherwise this won't work (it'll I believe but only if the python 
module that calls it imports glob itself).

> +            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
>   

It's this glob we don't need anymore, because it would then be imported 
by the python lib added in this very patch.

Cheers,
Quentin

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

[Marcus Folkesson]

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

Hi Quentin,

On Tue, May 28, 2024 at 01:01:29PM +0200, Quentin Schulz wrote:
> Hi Marcus,
> 
> On 5/28/24 11:08 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>
> > Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> > ---
> > 
> > Notes:
> >      v3:
> >         - Removed unnecessary "import glob"
> 
> Ah, there was a misunderstanding here, the "wrong" import glob got removed
> :)

Ah, of course.

> 
> >         - Explicitely tell that bootfiles_populate() returns a tuple
> > 
> >   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..666141df4e
> > --- /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 of tuples with (original filepath relative to
> > +# deploy_dir, desired filepath renaming)
> > +#
> > +# Heavily inspired of bootimg-partition.py
> > +#
> > +def get_boot_files(deploy_dir, boot_files):
> > +    import re
> > +    import os
> > +
> 
> We need from glob import glob here........
> 
> > +    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))
> > +
> 
> .... otherwise this won't work (it'll I believe but only if the python
> module that calls it imports glob itself).

Yep, it did, that was why my testing did not catch it.

> 
> > +            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
> 
> It's this glob we don't need anymore, because it would then be imported by
> the python lib added in this very patch.
> 
> Cheers,
> Quentin


Thanks again Quentin.
I will delay v4 a day or two to catch more comments.


Best regards,
Marcus Folkesson

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

[PATCH v3 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_BOOT_FILES_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>
---

Notes:
    v3:
            - Skip the intermediate bootfiles() function
            - Rename variable names to be consistent
            - Various python optimizations

 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..79c58f414a
--- /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_BOOT_FILES_DIR directory
+#
+#
+# Usage: add inherit += "image-bootfiles" to your image
+#
+
+IMAGE_BOOT_FILES_DIR ?= "boot"
+
+python bootfiles_populate() {
+    import shutil
+    from oe.bootfiles import get_boot_files
+
+    deploy_image_dir = d.getVar("DEPLOY_DIR_IMAGE")
+    boot_dir = os.path.join(d.getVar("IMAGE_ROOTFS"), d.getVar("IMAGE_BOOT_FILES_DIR"))
+
+    boot_files = d.getVar("IMAGE_BOOT_FILES")
+    if boot_files is None:
+        return
+
+    install_files = get_boot_files(deploy_image_dir, boot_files)
+    if install_files is None:
+        bb.warn("Could not find any boot files to install even though IMAGE_BOOT_FILES is not empty")
+        return
+
+    os.makedirs(boot_dir, exist_ok=True)
+    for src, dst  in install_files:
+        image_src = os.path.join(deploy_image_dir, src)
+        image_dst = os.path.join(boot_dir, dst)
+        shutil.copyfile(image_src, image_dst)
+}
+
+IMAGE_PREPROCESS_COMMAND += "bootfiles_populate;"
-- 
2.45.1

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

[Quentin Schulz]

Hi Marcus,

On 5/28/24 11:08 AM, Marcus Folkesson wrote:
> image-bootfiles class copy files listed in IMAGE_BOOT_FILES
> to the IMAGE_BOOT_FILES_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>
> ---
> 
> Notes:
>      v3:
>              - Skip the intermediate bootfiles() function
>              - Rename variable names to be consistent
>              - Various python optimizations
> 
>   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..79c58f414a
> --- /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_BOOT_FILES_DIR directory
> +#
> +#
> +# Usage: add inherit += "image-bootfiles" to your image
> +#

Simply

inherit image-bootfiles

c.f. 
https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#inherit-directive

since you put this class into meta/classes/ and not 
meta/classes-recipes/, it could be included both with

INHERIT += "image-bootfiles"

and

inherit image-bootfiles

The former is to be included from configuration files (e.g. a distro, 
local.conf, machine conf file, etc.). The latter inside the image recipe 
directly.

I am wondering if we shouldn't make this a recipes-only bbclass? so 
moving it to meta/classes-recipes/? This means that we won't be able to 
use the INHERIT += "image-bootfiles" anymore but.... should we? On one 
side, we have something that only applies to images, on the other hand, 
it's probably something close to a policy, so rather part of a distro. 
So just wondering, not asking for a change :)

Cheers,
Quentin

> +
> +IMAGE_BOOT_FILES_DIR ?= "boot"
> +
> +python bootfiles_populate() {
> +    import shutil
> +    from oe.bootfiles import get_boot_files
> +
> +    deploy_image_dir = d.getVar("DEPLOY_DIR_IMAGE")
> +    boot_dir = os.path.join(d.getVar("IMAGE_ROOTFS"), d.getVar("IMAGE_BOOT_FILES_DIR"))
> +
> +    boot_files = d.getVar("IMAGE_BOOT_FILES")
> +    if boot_files is None:
> +        return
> +
> +    install_files = get_boot_files(deploy_image_dir, boot_files)
> +    if install_files is None:
> +        bb.warn("Could not find any boot files to install even though IMAGE_BOOT_FILES is not empty")
> +        return
> +
> +    os.makedirs(boot_dir, exist_ok=True)
> +    for src, dst  in install_files:
> +        image_src = os.path.join(deploy_image_dir, src)
> +        image_dst = os.path.join(boot_dir, dst)
> +        shutil.copyfile(image_src, image_dst)

This won't work if the parent directory of image_dst doesn't exist yet.

"""
 >>> import shutil
 >>> shutil.copyfile('plep', 'thisisatest/alsothis/test')
Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
   File "/usr/lib64/python3.12/shutil.py", line 262, in copyfile
     with open(dst, 'wb') as fdst:
          ^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 
'thisisatest/alsothis/test'
"""

So I think we need to run:

os.makedirs(os.path.dirname(image_dst), exist_ok=True)

before calling shutil.copyfile?

In the bootimg-partition wic plugin, the following is done to install files:

install -m 0644 -D <src> <dst>

which creates all directories leading to <dst>. Additionally it makes it 
rw for owner, r for group and others. It seems that shutil.copyfile does 
this automatically (doesn't seem documented but it did this on a test in 
the python interpreter).

Cheers,
Quentin

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

[Marcus Folkesson]

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

Hi Quentin,

On Tue, May 28, 2024 at 01:23:20PM +0200, Quentin Schulz wrote:
> Hi Marcus,
> 

[...]

> > +#
> > +#
> > +# Usage: add inherit += "image-bootfiles" to your image
> > +#
> 
> Simply
> 
> inherit image-bootfiles

Noted

> 
> c.f. https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#inherit-directive
> 
> since you put this class into meta/classes/ and not meta/classes-recipes/,
> it could be included both with
> 
> INHERIT += "image-bootfiles"
> 
> and
> 
> inherit image-bootfiles
> 
> The former is to be included from configuration files (e.g. a distro,
> local.conf, machine conf file, etc.). The latter inside the image recipe
> directly.
> 
> I am wondering if we shouldn't make this a recipes-only bbclass? so moving
> it to meta/classes-recipes/? This means that we won't be able to use the
> INHERIT += "image-bootfiles" anymore but.... should we? On one side, we have
> something that only applies to images, on the other hand, it's probably
> something close to a policy, so rather part of a distro. So just wondering,
> not asking for a change :)

I don't have a strong opinion, I let this be to what people think is
the best.
If I must choose, I think it is more of a distro thing.

> 
> Cheers,
> Quentin
> 
> > +
> > +IMAGE_BOOT_FILES_DIR ?= "boot"
> > +
> > +python bootfiles_populate() {
> > +    import shutil
> > +    from oe.bootfiles import get_boot_files
> > +
> > +    deploy_image_dir = d.getVar("DEPLOY_DIR_IMAGE")
> > +    boot_dir = os.path.join(d.getVar("IMAGE_ROOTFS"), d.getVar("IMAGE_BOOT_FILES_DIR"))
> > +
> > +    boot_files = d.getVar("IMAGE_BOOT_FILES")
> > +    if boot_files is None:
> > +        return
> > +
> > +    install_files = get_boot_files(deploy_image_dir, boot_files)
> > +    if install_files is None:
> > +        bb.warn("Could not find any boot files to install even though IMAGE_BOOT_FILES is not empty")
> > +        return
> > +
> > +    os.makedirs(boot_dir, exist_ok=True)
> > +    for src, dst  in install_files:
> > +        image_src = os.path.join(deploy_image_dir, src)
> > +        image_dst = os.path.join(boot_dir, dst)
> > +        shutil.copyfile(image_src, image_dst)
> 
> This won't work if the parent directory of image_dst doesn't exist yet.

Good catch, I will do as you suggest.

[...]
> 
> Cheers,
> Quentin


Best regards,
Marcus Folkesson

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