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

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

[Quentin Schulz]

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?

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

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

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

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

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

Cheers,
Quentin

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

[Alexander Kanavin]

I also have to repeat what I said in docs@ list: why do we need a
special mechanism for placing files into /boot? If /boot is not a
special partition, then it's just a folder in / and can be populated
from packages like any other folder.

Alex

On Thu, 23 May 2024 at 15:26, Quentin Schulz via
lists.yoctoproject.org
<quentin.schulz=cherry.de@lists.yoctoproject.org> 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?
>
> > +
> > +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:
>
>  >>> 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.
>
> > +        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?
>
> 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 ;)
>
> > +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?
> 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?
>
> Cheers,
> Quentin
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#5236): https://lists.yoctoproject.org/g/docs/message/5236
> Mute This Topic: https://lists.yoctoproject.org/mt/106262122/1686489
> Group Owner: docs+owner@lists.yoctoproject.org
> Unsubscribe: https://lists.yoctoproject.org/g/docs/unsub [alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

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

[Quentin Schulz]

Hi Alex,

On 5/23/24 3:38 PM, Alexander Kanavin wrote:
> I also have to repeat what I said in docs@ list: why do we need a
> special mechanism for placing files into /boot? If /boot is not a
> special partition, then it's just a folder in / and can be populated
> from packages like any other folder.
> 

There's renaming logic here. Which provides much more flexibility than a 
package would. You could have a different name for each image recipe for 
example, instead of having to use a distro, packageconfig, bbappend to 
modify it. Is it a valid use-case we want to support /me shrugs, be we 
already do support it in wic, so maybe that ship has sailed already.

Cheers,
Quentin

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

[Marcus Folkesson]

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

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

[Quentin Schulz]

Hi Marcus,

(First, sorry for the typo I made in your name)

On 5/23/24 3:49 PM, Marcus Folkesson wrote:
> 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.
> 

Would be nice to add this to the comment as well then, so people know 
their options?

[...]

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

The code already exists, it's probably robust thanks to the test of 
time, so maybe not worth refactoring until someone complains about it. I 
assume we shouldn't have hundreds/thousands of entries in that variable 
so the cost is probably fine.

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

No, I should rather learn to not skip whatever looks like a license to 
me at the top of the file :) Sorry for the misplaced complaint.

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

This probably should be another python lib or something like that, maybe 
in meta/lib/ ? It's a bit trickier since the wic plugins are not really 
any part of a recipe, rather pure python stuff, so a bit different than 
the usual way of plugging stuff in it the proepr way?

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

Sounds good.

Cheers,
Quentin

Re: [OE-core] [PATCH 2/2] ref-manual: classes: add new image-bootfiles class

[Quentin Schulz]

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 ]
> 
> Describe the newly introduced image-bootfiles class.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>   documentation/ref-manual/classes.rst | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/documentation/ref-manual/classes.rst b/documentation/ref-manual/classes.rst
> index 9520d0bf7c..53b3697bee 100644
> --- a/documentation/ref-manual/classes.rst
> +++ b/documentation/ref-manual/classes.rst
> @@ -1169,6 +1169,20 @@ Yocto Project Overview and Concepts Manual.
> 
>   .. _ref-classes-image-buildinfo:
> 

This isn't the appropriate name for the reflink, we should ave

.. _ref-classes-image-bootfiles:

instead, to match the actual name of the class.

> +``image-bootfiles``
> +===================
> +
> +The :ref:`ref-classes-image-buildinfo` class copies over files listed

That's not the appropriate name, it should be 
ref-classes-image-bootfiles (it currently renders properly because the 
reflink is incorrect).

> +in :ref:`IMAGE_BOOT_FILES` to the /boot directory of the root filesystem.
> +

Tick quote /boot:
`/boot`
it;'s not meant to be understood as an English word.

> +This can be useful if no separate boot partition is used but all boot files

Replace "all" with "some", it depends on the content of 
IMAGE_BOOT_FILES, so we cannot say "all" here, it is misleading.

> +should be included into the rootfs image.
> +
> +:ref:`IMAGE_BOOT_FILES` is the same space-separated list of files used
> +by the ``bootimg-partition`` source plugin to populate the boot partition.
> +

I would not mention this here, rather edit the variables glossary entry 
for IMAGE_BOOT_FILES to mention that this is also used by image-bootfiles.
Also, it seems bootimg-efi plugin uses it, according to the glossary entry.

> +.. _ref-classes-image_types:

This isn't the appropriate name for the reflink, we should have

.. _ref-classes-image-buildinfo:

instead.

Cheers,
Quentin

Re: [OE-core] [PATCH 2/2] ref-manual: classes: add new image-bootfiles class

[Marcus Folkesson]

Hi Quentin,

On Thu, May 23, 2024 at 03:33:00PM +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 ]
> > 
> > Describe the newly introduced image-bootfiles class.
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >   documentation/ref-manual/classes.rst | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/documentation/ref-manual/classes.rst b/documentation/ref-manual/classes.rst
> > index 9520d0bf7c..53b3697bee 100644
> > --- a/documentation/ref-manual/classes.rst
> > +++ b/documentation/ref-manual/classes.rst
> > @@ -1169,6 +1169,20 @@ Yocto Project Overview and Concepts Manual.
> > 
> >   .. _ref-classes-image-buildinfo:
> > 
> 
> This isn't the appropriate name for the reflink, we should ave
> 
> .. _ref-classes-image-bootfiles:
> 
> instead, to match the actual name of the class.
> 
> > +``image-bootfiles``
> > +===================
> > +
> > +The :ref:`ref-classes-image-buildinfo` class copies over files listed
> 
> That's not the appropriate name, it should be ref-classes-image-bootfiles
> (it currently renders properly because the reflink is incorrect).
> 
> > +in :ref:`IMAGE_BOOT_FILES` to the /boot directory of the root filesystem.
> > +
> 
> Tick quote /boot:
> `/boot`
> it;'s not meant to be understood as an English word.
> 
> > +This can be useful if no separate boot partition is used but all boot files
> 
> Replace "all" with "some", it depends on the content of IMAGE_BOOT_FILES, so
> we cannot say "all" here, it is misleading.
> 
> > +should be included into the rootfs image.
> > +
> > +:ref:`IMAGE_BOOT_FILES` is the same space-separated list of files used
> > +by the ``bootimg-partition`` source plugin to populate the boot partition.
> > +
> 
> I would not mention this here, rather edit the variables glossary entry for
> IMAGE_BOOT_FILES to mention that this is also used by image-bootfiles.
> Also, it seems bootimg-efi plugin uses it, according to the glossary entry.
> 
> > +.. _ref-classes-image_types:
> 
> This isn't the appropriate name for the reflink, we should have
> 
> .. _ref-classes-image-buildinfo:
> 
> instead.
> 
> Cheers,
> Quentin

Thank you for all the comments, I agree with them all and will update
the patch accordingly.

FYI, I will drop the documentation patch from this series and resend it
once the class is ready for merging.
It was suggested by the doc mailing list (had to resend it as I was not
subscribed, so it is not part of this thread).


Best regards,
Marcus Folkesson