[PATCH] kernel-module-split: Allow for external conf files

[PATCH] kernel-module-split: Allow for external conf files

[Michal Sieron]

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

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

Re: [OE-core] [PATCH] kernel-module-split: Allow for external conf files

[Bruce Ashfield]

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

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]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end
- "Use the force Harry" - Gandalf, Star Trek II

[-- Attachment #2: Type: text/html, Size: 9960 bytes --]

Re: [OE-core] [PATCH] kernel-module-split: Allow for external conf files

[Michal Sieron]

> 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

Re: [OE-core] [PATCH] kernel-module-split: Allow for external conf files

[Bruce Ashfield]

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

[PATCH v2] kernel-module-split: Allow for external conf files

[Michal Sieron]

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

I am quite busy lately so didn't have time to go back to this topic.

This v2 differs from v1 by added comments and post install hook is now
treated the same way as (CONF)FILES:* variable modification.

 .../kernel-module-split.bbclass               | 27 +++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/meta/classes-recipe/kernel-module-split.bbclass b/meta/classes-recipe/kernel-module-split.bbclass
index 9487365eb7..a2d81f18e2 100644
--- a/meta/classes-recipe/kernel-module-split.bbclass
+++ b/meta/classes-recipe/kernel-module-split.bbclass
@@ -99,9 +99,12 @@ 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))
+
+        # The .conf file can either be installed by a recipe or generated from module_autoload_*
+        conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
+        name = '%s%s' % (dvar, conf)
+        # If module name is in KERNEL_MODULE_AUTOLOAD, then generate the .conf file and write to `name`.
         if basename in autoloadlist:
-            conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
-            name = '%s%s' % (dvar, conf)
             os.makedirs(os.path.dirname(name), exist_ok=True)
             with open(name, 'w') as f:
                 if autoload:
@@ -109,6 +112,9 @@ python split_kernel_module_packages () {
                         f.write('%s\n' % m)
                 else:
                     f.write('%s\n' % basename)
+        # If the .conf file exits, then add it to FILES:* and CONFFILES:* and add postinstall hook.
+        # It doesn't matter if it was generated from module_autoload_* or installed by the recipe.
+        if os.path.exists(name):
             conf2append = ' %s' % conf
             d.appendVar('FILES:%s' % pkg, conf2append)
             d.appendVar('CONFFILES:%s' % pkg, conf2append)
@@ -121,18 +127,23 @@ python split_kernel_module_packages () {
         # Write out any modconf fragment
         modconflist = (d.getVar("KERNEL_MODULE_PROBECONF") or "").split()
         modconf = d.getVar('module_conf_%s' % basename)
+
+        # The .conf file can either be installed by a recipe or generated from module_conf_*
+        conf = '%s/%s.conf' % (d.getVar('modprobedir'), basename)
+        name = '%s%s' % (dvar, conf)
+        # If module name is in KERNEL_MODULE_PROBECONF, then generate the .conf file and write to `name`.
         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 the .conf file exits, then add it to FILES:* and CONFFILES:*.
+        # It doesn't matter if it was generated from module_conf_* or installed by the recipe.
+        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 ""
-- 
2.49.0

Re: [PATCH v2] kernel-module-split: Allow for external conf files

[Bruce Ashfield]

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

On Tue, Apr 29, 2025 at 9:21 AM Michal Sieron <michalwsieron@gmail.com>
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.
>
> Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
> ---
>
> I am quite busy lately so didn't have time to go back to this topic.
>
> This v2 differs from v1 by added comments and post install hook is now
> treated the same way as (CONF)FILES:* variable modification.
>

And sorry for my very slow reply.

This version of the patch was hidden below the original, so I missed it
when it arrived.



>  .../kernel-module-split.bbclass               | 27 +++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/meta/classes-recipe/kernel-module-split.bbclass
> b/meta/classes-recipe/kernel-module-split.bbclass
> index 9487365eb7..a2d81f18e2 100644
> --- a/meta/classes-recipe/kernel-module-split.bbclass
> +++ b/meta/classes-recipe/kernel-module-split.bbclass
> @@ -99,9 +99,12 @@ 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))
> +
> +        # The .conf file can either be installed by a recipe or generated
> from module_autoload_*
> +        conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
> +        name = '%s%s' % (dvar, conf)
> +        # If module name is in KERNEL_MODULE_AUTOLOAD, then generate the
> .conf file and write to `name`.
>          if basename in autoloadlist:
> -            conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
> -            name = '%s%s' % (dvar, conf)
>              os.makedirs(os.path.dirname(name), exist_ok=True)
>              with open(name, 'w') as f:
>                  if autoload:
> @@ -109,6 +112,9 @@ python split_kernel_module_packages () {
>                          f.write('%s\n' % m)
>                  else:
>                      f.write('%s\n' % basename)
> +        # If the .conf file exits, then add it to FILES:* and CONFFILES:*
> and add postinstall hook.
>

Minor typo s/exits/exists/



> +        # It doesn't matter if it was generated from module_autoload_* or
> installed by the recipe.
> +        if os.path.exists(name):
>              conf2append = ' %s' % conf
>              d.appendVar('FILES:%s' % pkg, conf2append)
>              d.appendVar('CONFFILES:%s' % pkg, conf2append)
> @@ -121,18 +127,23 @@ python split_kernel_module_packages () {
>          # Write out any modconf fragment
>          modconflist = (d.getVar("KERNEL_MODULE_PROBECONF") or "").split()
>          modconf = d.getVar('module_conf_%s' % basename)
> +
> +        # The .conf file can either be installed by a recipe or generated
> from module_conf_*
> +        conf = '%s/%s.conf' % (d.getVar('modprobedir'), basename)
> +        name = '%s%s' % (dvar, conf)
> +        # If module name is in KERNEL_MODULE_PROBECONF, then generate the
> .conf file and write to `name`.
>          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 the .conf file exits, then add it to FILES:* and CONFFILES:*.
> +        # It doesn't matter if it was generated from module_conf_* or
> installed by the recipe.
> +        if os.path.exists(name):
> +            conf2append = ' %s' % conf
> +            d.appendVar('FILES:%s' % pkg, conf2append)
> +            d.appendVar('CONFFILES:%s' % pkg, conf2append)
>
>
This version looks good to me.

I would still be nice to have a test for this, but the code paths are
sufficiently
similar now that I don't see anything that could break.

Bruce



>          if "description" in vals:
>              old_desc = d.getVar('DESCRIPTION:' + pkg) or ""
> --
> 2.49.0
>
>

-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end
- "Use the force Harry" - Gandalf, Star Trek II

[-- Attachment #2: Type: text/html, Size: 7532 bytes --]