Re: [docs] [PATCH 2/2] dev-manual: document how to provide confs from layer.conf

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

Hi Quentin,

On Wed Oct 30, 2024 at 11:18 AM CET, Quentin Schulz wrote:
[...]
>> +Providing Global-level Configurations With Your Layer
>> +-----------------------------------------------------
>> +
>> +When creating a layer, you may need to define configurations that should take
>> +effect globally in your build environment when it is part of the
>> +:term:`BBLAYERS` variable. The ``layer.conf`` file is a :term:`configuration
>
> "when it is part of the BBLAYERS variable" is a complicated way of 
> saying "when it is used"/"when it is part of the build setup". Maybe we 
> can find something more explicit?
>
> I would also suggest adding "in the new layer" after ``layer.conf`` to 
> explicit the location of layer.conf (maybe we should even add it's in 
> conf/?). We have many different possible locations for configuration 
> files so I feel like we should tell people where exactly to expect 
> files. (e.g. local.conf, distro conf file, machine conf file, 
> (multiconfig?), bblayers.conf, are all expected in different locations).
>
>> +file` that affects the build system globally, so it is a candidate for this
>> +use-case.
>> +
>> +For example, if your layer provides an alternative to the linux kernel named
>
> s/provides an alternative to the Linux kernel/provides a Linux kernel 
> recipe/
>
> s/linux/Linux/

+1, thanks.

>> +``linux-custom``, you may want to define the :term:`PREFERRED_PROVIDER` for the
>> +``linux-custom`` recipe::
>> +
>> +  PREFERRED_PROVIDER_virtual/kernel = "linux-custom"
>> +
>> +This can be defined in the ``layer.conf`` file. If your layer has a higher
>> +:term:`BBFILE_PRIORITY` value, it will take precedence over previous definitions
>> +of the same ``PREFERRED_PROVIDER_virtual/kernel`` assignments.
>> +
>
> I seem to recall this is the perfect example of what we do NOT want in a 
> layer.conf though (might be bad recollection though :) ). I actually do 
> not even know what'd be a valid reason to have variables directly 
> defined in layer.conf (I see you're providing an example in the next 
> section, but do we really need this section to provide an example of 
> something the user shouldn't be doing?).
>
> I would add a very big note/warning at the end of this section saying 
> that this is almost always wrong as it is mostly expected that adding a 
> layer should only minimally change the build output. And that for this 
> very example, it should be in a machine or distro configuration file 
> instead.

You're right, it's not a good practice. Initially, I wanted to document only the
conditional case below, but it felt strange introducing it without a note on the
role of layers.conf & setting things from there. What about the following
changes instead:

* Remove the first section that shows how to set configurations unconditionally.
* Instead, introduce the next section by saying something like "Since it's not
  good practice to unconditionally set global configurations from the layer.conf
  file, we can use the following technique...".
* Simply rename the section "Providing Global-level Configurations With Your
  Layer" instead of "Conditionally Provide Global-level Configurations With Your
  Layer". This way we only show the "good" way to make changes from layer.conf.

> I am not sure that BBFILE_PRIORITY is applicable for this as what 
> matters is the parsing order of all layer.conf files from entries in 
> BBLAYERS. I believe it's just parsed in the order it appears in 
> BBLAYERS? Last one has priority (because we use = and not ?=). If that 
> is actually working, we should then expand 
> https://docs.yoctoproject.org/ref-manual/variables.html#term-BBFILE_PRIORITY 
> as well because it clearly only defines priority for recipes.

My bad you're right, it's only the order in BBLAYERS that matters, I'll remove
this sentence.

>> +Conditionally Provide Global-level Configurations With Your Layer
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +In some cases, your layer may provide global configurations only if some
>> +features it provides are enabled. Since the ``layer.conf`` file is parsed at an
>> +earlier stage in the parsing process, the :term:`DISTRO_FEATURES` and
>> +:term:`MACHINE_FEATURES` variables are not yet available to ``layer.conf``, and
>> +declaring conditional assignments based on these variables is not possible. The
>> +following technique shows a way to leverage this limitation by using the
>> +:term:`USER_CLASSES` variable and a conditional ``include`` command.
>> +
>> +In the following steps, let's assume our layer is named ``meta-mylayer`` and
>> +that this layer defines a custom :ref:`distro feature <ref-features-distro>`
>> +named ``mylayer-kernel``. We will set the :term:`PREFERRED_PROVIDER` variable
>> +for the kernel only if our feature ``mylayer-kernel`` is part of the
>> +:term:`DISTRO_FEATURES`:
>> +
>> +#. Create an include file in the directory
>> +   ``meta-mylayer/conf/distro/include/``, for example a file named
>> +   ``mylayer-kernel-provider.inc`` that sets the kernel provider to
>> +   ``linux-custom``::
>> +
>> +     PREFERRED_PROVIDER_virtual/kernel = "linux-custom"
>
> Random thought but maybe we should have align this with three 
> whitespaces starting from the first character in the list. Here it is 
> only one and difficult to notice at first sight. Actually, it's two 
> whitespaces, I think that proves my point :D

Yes, it should be 3 actually, missed that!

>> +
>> +#. Provide a path to this include file in your ``layer.conf``::
>> +
>> +     META_MYLAYER_KERNEL_PROVIDER_PATH = "${LAYERDIR}/conf/distro/include/mylayer-kernel-provider.inc"
>> +
>> +#. Create a new class in ``meta-mylayer/classes-global/``, for example a class
>> +   ``meta-mylayer-cfg.bbclass``. It Conditionally includes the file
>
> s/Conditionally/conditionally/
>
> Maybe say "Make it conditionally include the file" instead, as the way 
> it's phrased could mean that because it's named this way or/and in that 
> special directory that it magically includes the file, instead of 
> telling the user what to put in that file.

+1, better that way :)

>> +   ``mylayer-kernel-provider.inc`` defined above, using the variable
>> +   ``META_MYLAYER_KERNEL_PROVIDER_PATH`` defined in ``layer.conf``::
>> +
>> +     include ${@bb.utils.contains('DISTRO_FEATURES', 'mylayer-kernel', '${META_MYLAYER_KERNEL_PROVIDER_PATH}', '', d)}
>> +
>> +   For details on the ``bb.utils.contains`` function, see its definition in
>> +   :bitbake_git:`lib/bb/utils.py </tree/lib/bb/utils.py>`.
>> +
>> +   .. note::
>> +
>> +      The ``include`` command is designed to not fail if the function
>> +      ``bb.utils.contains`` returns an empty string.
>
> I believe `require` too and we very likely want to use require here to 
> make sure the path doesn't have a typo :)

Correct, I thought `require` would've failed if contains returned an empty
string (and that it was for that reason they used include in meta-virt) but
apparently not, the parser is smart enough. :) Will change, thanks

> Also, I'm wondering if this isn't actually "breaking" the sstate cache 
> as this META_MYLAYER_KERNEL_PROVIDER_PATH would be different on each 
> machine so people wouldn't be able to reuse the sstate cache.
>
> Building with the exact same setup - and a shared SSTATE_DIR - in two 
> different directories on the same machine should answer that question :)

I've tested it and no, it doesn't affect the sstate-cache.
I've done the following:

1) Build core-image-minimal with virtual/kernel set to linux-yocto-rt (set with
   a custom layer + technique detailed above, through a distro feature).
2) Same directory, rm -r tmp/, rebuild -> setscene tasks are executed as
   expected, so no rebuild, just the rootfs being created.
3) Different directory (to make LAYERDIR change for my custom layer), rm -r
   tmp/, rebuild -> same result as 2).

>> +
>> +#. Back to your ``layer.conf`` file, add the class ``meta-mylayer-cfg`` class to
>> +   the :term:`USER_CLASSES` variable::
>> +
>> +     USER_CLASSES:append = " meta-mylayer-cfg"
>> +
>> +   This will add the class ``meta-mylayer-cfg`` to the list of classes to
>> +   globally inherit. Since the ``include`` command is conditional in
>> +   ``meta-mylayer-cfg.bbclass``, even though inherited the class will have no
>> +   effect until the feature ``mylayer-kernel`` is enabled through
>> +   :term:`DISTRO_FEATURES`.
>> +
>> +This technique can also be used for :ref:`Machine features
>> +<ref-features-machine>` by following the same steps, the only difference being
>> +to place the include file in ``meta-mylayer/conf/machine/include/`` instead.
>> +
>
> I don't think the path needs to be specific?

Not really, but I'd rather advise here to place it in a conventionally used
directories (since we generally use this directory for machine include files)
rather than giving a random example. What do you think?

>>   Managing Layers
>>   ===============
>>   
>> @@ -916,4 +993,3 @@ above:
>>   .. note::
>>      Both the ``create-layers-setup`` and the ``setup-layers`` provided several additional options
>>      that customize their behavior - you are welcome to study them via ``--help`` command line parameter.
>> -
>
> Not sure this line deletion makes sense in this patch?

Oops, will remove.

> Cheers,
> Quentin

Cheers,
Antonin

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