kernel-module-split: Allow for external conf files

Some recipes might provide conf files produced during build phase or
simply tracked in the VCS instead of generating them with Yocto.
In such cases those conf files wouldn't be assigned to correct packages.
With this change, if user wants to generate a conf file they still can,
but not generating them won't prevent assigning the file to proper
package given the file exists.

Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
---
 .../kernel-module-split.bbclass               | 25 +++++++++++--------
 1 file changed, 14 insertions(+), 11 deletions(-)

On Thu, Apr 10, 2025 at 5:04 AM Michal Sieron via lists.openembedded.org
<michalwsieron=gmail.com@lists.openembedded.org> wrote:

> Some recipes might provide conf files produced during build phase or
> simply tracked in the VCS instead of generating them with Yocto.
> In such cases those conf files wouldn't be assigned to correct packages.
> With this change, if user wants to generate a conf file they still can,
> but not generating them won't prevent assigning the file to proper
> package given the file exists.
>

Whenever we add a different code path, we need to add a test to
the oe selftests, both for the existing case (if there isn't one already)
and the new one. Without those tests, we'll get a better understanding
of how this is supposed to work, that it doesn't break existing users
and we'll know when it breaks in the future (since this obviously won't
be a default path with our reference distros).


> Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
> ---
>  .../kernel-module-split.bbclass               | 25 +++++++++++--------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/meta/classes-recipe/kernel-module-split.bbclass
> b/meta/classes-recipe/kernel-module-split.bbclass
> index 9487365eb7..6da3cb7fc7 100644
> --- a/meta/classes-recipe/kernel-module-split.bbclass
> +++ b/meta/classes-recipe/kernel-module-split.bbclass
> @@ -99,9 +99,9 @@ python split_kernel_module_packages () {
>              bb.warn("module_autoload_%s was replaced by
> KERNEL_MODULE_AUTOLOAD for cases where basename == module name, please drop
> it" % basename)
>          if autoload and basename not in autoloadlist:
>              bb.warn("module_autoload_%s is defined but '%s' isn't
> included in KERNEL_MODULE_AUTOLOAD, please add it there" % (basename,
> basename))
> +        conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
> +        name = '%s%s' % (dvar, conf)
>          if basename in autoloadlist:
> -            conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
> -            name = '%s%s' % (dvar, conf)
>

A comment about how your high level description in the long log relates
to needing to move these variables outside of the conditional would be
helpful.


>              os.makedirs(os.path.dirname(name), exist_ok=True)
>              with open(name, 'w') as f:
>                  if autoload:
> @@ -109,31 +109,34 @@ python split_kernel_module_packages () {
>                          f.write('%s\n' % m)
>                  else:
>                      f.write('%s\n' % basename)
> -            conf2append = ' %s' % conf
> -            d.appendVar('FILES:%s' % pkg, conf2append)
> -            d.appendVar('CONFFILES:%s' % pkg, conf2append)
>              postinst = d.getVar('pkg_postinst:%s' % pkg)
>              if not postinst:
>                  bb.fatal("pkg_postinst:%s not defined" % pkg)
>              postinst += d.getVar('autoload_postinst_fragment') %
> (autoload or basename)
>              d.setVar('pkg_postinst:%s' % pkg, postinst)
>
> +        if os.path.exists(name):
> +            conf2append = ' %s' % conf
> +            d.appendVar('FILES:%s' % pkg, conf2append)
> +            d.appendVar('CONFFILES:%s' % pkg, conf2append)
>

A quick comment here would help as well. This is now outside the
conditional, so a comment indicating that the file at "name" can either
be generated above or placed there by a recipe will help with
maintenance.

I'm thinking that we should also log when a supplied configuration
file is used. Otherwise, it may not be obvious if a runtime issue
is caused.

Also, what happens if the file at "name" is updated ? I don't
think the module splitting would be re-run, so would we have a
"stale" file ? Should that file be something in the kernel-module
recipe that triggers a rebuild if it is changed ? I don't see that
mentioned (maybe I'm imagining the problem) or a requirement
on the recipe.

Bruce


> +
>          # Write out any modconf fragment
>          modconflist = (d.getVar("KERNEL_MODULE_PROBECONF") or "").split()
>          modconf = d.getVar('module_conf_%s' % basename)
> +        conf = '%s/%s.conf' % (d.getVar('modprobedir'), basename)
> +        name = '%s%s' % (dvar, conf)
>          if modconf and basename in modconflist:
> -            conf = '%s/%s.conf' % (d.getVar('modprobedir'), basename)
> -            name = '%s%s' % (dvar, conf)
>              os.makedirs(os.path.dirname(name), exist_ok=True)
>              with open(name, 'w') as f:
>                  f.write("%s\n" % modconf)
> -            conf2append = ' %s' % conf
> -            d.appendVar('FILES:%s' % pkg, conf2append)
> -            d.appendVar('CONFFILES:%s' % pkg, conf2append)
> -
>          elif modconf:
>              bb.error("Please ensure module %s is listed in
> KERNEL_MODULE_PROBECONF since module_conf_%s is set" % (basename, basename))
>
> +        if os.path.exists(name):
> +            conf2append = ' %s' % conf
> +            d.appendVar('FILES:%s' % pkg, conf2append)
> +            d.appendVar('CONFFILES:%s' % pkg, conf2append)
> +
>          if "description" in vals:
>              old_desc = d.getVar('DESCRIPTION:' + pkg) or ""
>              d.setVar('DESCRIPTION:' + pkg, old_desc + "; " +
> vals["description"])
> --
> 2.49.0
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#214633):
> https://lists.openembedded.org/g/openembedded-core/message/214633
> Mute This Topic: https://lists.openembedded.org/mt/112187964/1050810
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> bruce.ashfield@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

> Whenever we add a different code path, we need to add a test to
> the oe selftests, both for the existing case (if there isn't one already)
> and the new one. Without those tests, we'll get a better understanding
> of how this is supposed to work, that it doesn't break existing users
> and we'll know when it breaks in the future (since this obviously won't
> be a default path with our reference distros).

The problem with this is that there are no existing tests for
kernel-module-split.bbclass. So I would need some heavy assistance with
writing such test cases :(

But I do agree that tests are generally a good idea.
The whole reason I sent the patch is because I noticed that behavior
change between kirkstone and scarthgap caused by this change in
oe-core:71460993f350bca3d5a22115fd5551696f955c9f.

> A comment about how your high level description in the long log relates
> to needing to move these variables outside of the conditional would be
> helpful.
...
> A quick comment here would help as well. This is now outside the
> conditional, so a comment indicating that the file at "name" can either
> be generated above or placed there by a recipe will help with
> maintenance.

Will try to somehow explain it in v2 of the patch.

> Also, what happens if the file at "name" is updated ? I don't
> think the module splitting would be re-run, so would we have a
> "stale" file ? Should that file be something in the kernel-module
> recipe that triggers a rebuild if it is changed ? I don't see that
> mentioned (maybe I'm imagining the problem) or a requirement
> on the recipe.
Wouldn't that alter do_install's checksum? Otherwise I don't think I
have an answer.


Anyway, while I was preparing this response I noticed some other things.

1. pkg_postinst hook
There is this `pkg_postinst` part for "autoload". I guess I should move
that hook so it is also installed when .conf is vendored, is that right?
Although that wasn't there before the regression commit I found.

2. Duplicated entries in (CONF)FILES:*
Even before my patch FILES:* and CONFFILES:* entries for kernel
module packages are for some reason doubled. But that is probably for
another patch.

Best regards,
Michal

In message: Re: [OE-core] [PATCH] kernel-module-split: Allow for external conf files
on 17/04/2025 Michal Sieron wrote:

> > Whenever we add a different code path, we need to add a test to
> > the oe selftests, both for the existing case (if there isn't one already)
> > and the new one. Without those tests, we'll get a better understanding
> > of how this is supposed to work, that it doesn't break existing users
> > and we'll know when it breaks in the future (since this obviously won't
> > be a default path with our reference distros).
> 
> The problem with this is that there are no existing tests for
> kernel-module-split.bbclass. So I would need some heavy assistance with
> writing such test cases :(

We can definitely help with this, having specific questions will
help of course, but this is worth the effort (and in fact may be
the only way to get this merged).

I'm no expert in self-tests either, but can offer some of my
time to sort through the issue.

The code is simply to complex now to not be tested at all. We don't
need something complex, just something that ensures a conf file is
generated in both cases.

> 
> But I do agree that tests are generally a good idea.
> The whole reason I sent the patch is because I noticed that behavior
> change between kirkstone and scarthgap caused by this change in
> oe-core:71460993f350bca3d5a22115fd5551696f955c9f.

Agreed!

> 
> > A comment about how your high level description in the long log relates
> > to needing to move these variables outside of the conditional would be
> > helpful.
> ...
> > A quick comment here would help as well. This is now outside the
> > conditional, so a comment indicating that the file at "name" can either
> > be generated above or placed there by a recipe will help with
> > maintenance.
> 
> Will try to somehow explain it in v2 of the patch.
> 
> > Also, what happens if the file at "name" is updated ? I don't
> > think the module splitting would be re-run, so would we have a
> > "stale" file ? Should that file be something in the kernel-module
> > recipe that triggers a rebuild if it is changed ? I don't see that
> > mentioned (maybe I'm imagining the problem) or a requirement
> > on the recipe.
> Wouldn't that alter do_install's checksum? Otherwise I don't think I
> have an answer.
> 
> 
> Anyway, while I was preparing this response I noticed some other things.
> 
> 1. pkg_postinst hook
> There is this `pkg_postinst` part for "autoload". I guess I should move
> that hook so it is also installed when .conf is vendored, is that right?
> Although that wasn't there before the regression commit I found.

At a glance .. yes, we'd want to keep that same behavior in both
modes.

> 
> 2. Duplicated entries in (CONF)FILES:*
> Even before my patch FILES:* and CONFFILES:* entries for kernel
> module packages are for some reason doubled. But that is probably for
> another patch.

Yes, it would be best to do this patch, and a second separate one
that sorts that out. If the logical changes are separate, they are
easier to review and merge.

Bruce

> 
> Best regards,
> Michal