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

[Bruce Ashfield <bruce.ashfield@gmail.com>]

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