Re: [docs] [PATCH] ref-manual: add documentation for the barebox class

["Antonin Godard" <antonin.godard@bootlin.com>]

Hi Enrico,

On Fri Jan 17, 2025 at 2:36 PM CET, Enrico Jörns wrote:
> Am Freitag, dem 17.01.2025 um 14:12 +0100 schrieb Antonin Godard:
>> Hi Enrico,
>> 
>> On Thu Jan 16, 2025 at 12:14 PM CET, Enrico Jörns wrote:
>> > From: Enrico Joerns <ejo@pengutronix.de>
>> > 
>> > This adds the initial documentation for the newly added barebox.bbclass
>> > to the Reference Manual's class list.
>> > It also adds the two most notable variables to the variable list.
>> > 
>> > Signed-off-by: Enrico Joerns <ejo@pengutronix.de>
>> > ---
>> >  documentation/ref-manual/classes.rst   | 36 ++++++++++++++++++++++++++
>> >  documentation/ref-manual/variables.rst | 13 ++++++++++
>> >  2 files changed, 49 insertions(+)
>> > 
>> > diff --git a/documentation/ref-manual/classes.rst b/documentation/ref-manual/classes.rst
>> > index 22c4301a4..c11c0a529 100644
>> > --- a/documentation/ref-manual/classes.rst
>> > +++ b/documentation/ref-manual/classes.rst
>> > @@ -128,6 +128,42 @@ It's useful to have some idea of how the tasks defined by the
>> >  -  :ref:`ref-tasks-install` --- runs ``make install`` and
>> >     passes in ``${``\ :term:`D`\ ``}`` as ``DESTDIR``.
>> >  
>> > +.. _ref-classes-barebox:
>> > +
>> > +``barebox``
>> > +===========
>> > +
>> > +The :ref:`ref-classes-barebox` class manages building the barebox bootloader.
>> 
>> s/barebox/BareBox/? or Barebox
>> We try to capitalize project names.
>
> I can change it, but the project understands itself as "barebox" [1].
> Like 'systemd', which I don't find as "Systemd" in the docs either.

You're right, my bad I wasn't aware of that - you can leave it as "barebox"
then.

>> > +
>> > +If a file named ``defconfig`` is included in the :term:`SRC_URI`, it will be
>> > +copied to ``.config`` in the build directory and used as the barebox
>> > +configuration.
>> > +Instead of providing a ``defconfig`` file, you can set :term:`BAREBOX_CONFIG`
>> > +to a defconfig provided by the barebox source tree.
>> > +If neither ``defconfig`` nor :term:`BAREBOX_CONFIG` is specified, the class
>> > +will raise an error.
>> > +
>> > +The :ref:`ref-classes-barebox` class supports config fragments and internally
>> > +includes the :ref:`ref-classes-cml1` class to provide kconfig support for
>> 
>> Maybe link to https://docs.kernel.org/kbuild/kconfig-language.html here? Some
>> people might not know what this is referring to. This would be done like so:
>> 
>>   ...provide `Kconfig <https://docs.kernel.org/kbuild/kconfig-language.html>`__
>>   support...
>
> Ok, I'll add that.
>
>> > +barebox, enabling tasks such as :ref:`ref-tasks-menuconfig` and
>> > +:ref:`ref-tasks-diffconfig`.
>> > +
>> > +The generated barebox binaries are deployed to
>> > +:term:`DEPLOY_DIR_IMAGE` as well as installed to ``BAREBOX_INSTALL_PATH``
>> > +(``/boot`` by default) making them part of the recipe’s base package.
>> > +This setup supports both using the barebox binaries as independent artifacts
>> > +and installing them into a rootfs.
>> > +:term:`BAREBOX_BINARY` can be used to select a distinct binary to deploy and
>> > +install.
>> > +If ``barebox`` is set as the :term:`EFI_PROVIDER`, the class will leverage
>> > +``conf/image-uefi.conf`` to define the default installation paths and naming
>> 
>> :oe_git:`conf/image-uefi.conf </openembedded-core/tree/meta/conf/image-uefi.conf>`
>> instead of
>> ``conf/image-uefi.conf``
>
> Yes.
>
>> > +conventions.
>> > +
>> > +The compiled-in barebox environment can be extended by adding environment files
>> > +to the ``BAREBOX_ENV_DIR``.
>> 
>> :term:`BAREBOX_ENV_DIR` and some documentation for it in variables.rst? Since
>> it's defined with ??=. But not sure it would be frequently overriden.
>> 
>> Same remark for BAREBOX_FIRMWARE_DIR.
>
> Yes, I have actually thought about documenting but noted some potential inconsistencies where I
> wasn't sure yet if this should be documented as external API. I'd clarify this and maybe add the
> documentation for these in a later release if that's ok.

Ok, thanks for the details - I also agree on documenting it later then.

>> > +The ``BAREBOX_FIRMWARE_DIR`` variable allows you to specify the firmware blob
>> > +search directory, enabling loading of additional firmware like TF-A or OP-TEE.
>> > +
>> >  .. _ref-classes-base:
>> >  
>> >  ``base``
>> > diff --git a/documentation/ref-manual/variables.rst b/documentation/ref-manual/variables.rst
>> > index 702a6caf8..f29f38588 100644
>> > --- a/documentation/ref-manual/variables.rst
>> > +++ b/documentation/ref-manual/variables.rst
>> > @@ -293,6 +293,19 @@ system and gives an overview of their function and contents.
>> >        :term:`PACKAGE_EXCLUDE` variables for related
>> >        information.
>> >  
>> > +   :term:`BAREBOX_BINARY`
>> > +      The barebox build system can build multiple barebox binaries at once.
>> > +      By default, all built binaries will be deployed and installed under their
>> > +      original name.
>> > +      If only a specific binary should be deployed/installed, its name can
>> > +      be specified in :term:`BAREBOX_BINARY`.
>> 
>> Maybe specify the default value, or give an example of what this variable can
>> contain?
>
> The default value depends on inline python, but will check if I can better reflect this in the
> documentation.

I'd say if the default value is not easily predictable, maybe just give an
example for users to know what the content is supposed to look like.

>> > +
>> > +   :term:`BAREBOX_CONFIG`
>> > +      Specifies the name of the barebox defconfig to build.
>> > +      The name must be a defconfig file known to the barebox build environment.
>> > +      For more information on how to provide a barebox configuration, see the
>> > +      :ref:`ref-classes-barebox` class.
>> 
>> At least specify that it's empty by default?
>
> Might be useful, yes.
>
>> > +
>> >     :term:`BASE_LIB`
>> >        The library directory name for the CPU or Application Binary
>> >        Interface (ABI) tune. The :term:`BASE_LIB` applies only in the Multilib
>> 
>> 
>> Thank you for taking time to document this,
>
> Thank you for the review! Will come up with a v2 as soon as I've addressed the open points.
>
>
> Regards, Enrico
>
>> Antonin
>> 
>
> [1] https://barebox.org/

Thanks,
Antonin

-- 
Antonin Godard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com