| Message ID | 20240304155732.2267310-1-pkj@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] kernel-module-dirs.bbclass: Add class | expand |
On Mon, 2024-03-04 at 16:57 +0100, Peter Kjellerstedt wrote: > Split out the two variables modulesloaddir and modprobedir from > kernel-module-split.bbclass as they can be useful to other recipes than > kernel module recipes. > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> > --- > meta/classes-recipe/kernel-module-dirs.bbclass | 8 ++++++++ > 1 file changed, 8 insertions(+) > create mode 100644 meta/classes-recipe/kernel-module-dirs.bbclass > > diff --git a/meta/classes-recipe/kernel-module-dirs.bbclass b/meta/classes-recipe/kernel-module-dirs.bbclass > new file mode 100644 > index 0000000000..eecc36ab52 > --- /dev/null > +++ b/meta/classes-recipe/kernel-module-dirs.bbclass > @@ -0,0 +1,8 @@ > +# > +# Copyright OpenEmbedded Contributors > +# > +# SPDX-License-Identifier: MIT > +# > + > +modulesloaddir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_libdir}', '${sysconfdir}', d)}/modules-load.d" > +modprobedir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_base_libdir}', '${sysconfdir}', d)}/modprobe.d" Absolutely not. We are not having yet more kernel classes just for two variables. There is probably a better way moving some definitions to a new conf file for the kernel in general. Cheers, Richard
> -----Original Message----- > From: Richard Purdie <richard.purdie@linuxfoundation.org> > Sent: den 4 mars 2024 17:17 > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org > Subject: Re: [OE-core] [PATCH 1/2] kernel-module-dirs.bbclass: Add class > > On Mon, 2024-03-04 at 16:57 +0100, Peter Kjellerstedt wrote: > > Split out the two variables modulesloaddir and modprobedir from > > kernel-module-split.bbclass as they can be useful to other recipes than > > kernel module recipes. > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> > > --- > > meta/classes-recipe/kernel-module-dirs.bbclass | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > create mode 100644 meta/classes-recipe/kernel-module-dirs.bbclass > > > > diff --git a/meta/classes-recipe/kernel-module-dirs.bbclass b/meta/classes-recipe/kernel-module-dirs.bbclass > > new file mode 100644 > > index 0000000000..eecc36ab52 > > --- /dev/null > > +++ b/meta/classes-recipe/kernel-module-dirs.bbclass > > @@ -0,0 +1,8 @@ > > +# > > +# Copyright OpenEmbedded Contributors > > +# > > +# SPDX-License-Identifier: MIT > > +# > > + > > +modulesloaddir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_libdir}', '${sysconfdir}', d)}/modules-load.d" > > +modprobedir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_base_libdir}', '${sysconfdir}', d)}/modprobe.d" > > Absolutely not. We are not having yet more kernel classes just for two > variables. Ok. I considered adding them to bitbake.conf where all other similar *dir variables are defined, but I opted for a bbclass since I expect it to only be relatively few recipes that need either of them (we currently have ~10 non-module recipes that could use them). > > There is probably a better way moving some definitions to a new conf > file for the kernel in general. I am not sure what to make of this. What I wanted to achieve was to be able to make the two path variables available to non-module recipes. I am not sure how that matches what you meant with "a new conf file for the kernel". Did you actually mean a .conf file where the variables would be added to the global state, or did you mean a .bbclass/.inc file similar to the one I proposed, but with a more generic name so that it can take other variables than just the two path variables above? > > Cheers, > > Richard //Peter
On Mon, 2024-03-04 at 17:20 +0000, Peter Kjellerstedt wrote: > > -----Original Message----- > > From: Richard Purdie <richard.purdie@linuxfoundation.org> > > Sent: den 4 mars 2024 17:17 > > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org > > Subject: Re: [OE-core] [PATCH 1/2] kernel-module-dirs.bbclass: Add class > > > > On Mon, 2024-03-04 at 16:57 +0100, Peter Kjellerstedt wrote: > > > Split out the two variables modulesloaddir and modprobedir from > > > kernel-module-split.bbclass as they can be useful to other recipes than > > > kernel module recipes. > > > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> > > > --- > > > meta/classes-recipe/kernel-module-dirs.bbclass | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > create mode 100644 meta/classes-recipe/kernel-module-dirs.bbclass > > > > > > diff --git a/meta/classes-recipe/kernel-module-dirs.bbclass b/meta/classes-recipe/kernel-module-dirs.bbclass > > > new file mode 100644 > > > index 0000000000..eecc36ab52 > > > --- /dev/null > > > +++ b/meta/classes-recipe/kernel-module-dirs.bbclass > > > @@ -0,0 +1,8 @@ > > > +# > > > +# Copyright OpenEmbedded Contributors > > > +# > > > +# SPDX-License-Identifier: MIT > > > +# > > > + > > > +modulesloaddir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_libdir}', '${sysconfdir}', d)}/modules-load.d" > > > +modprobedir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_base_libdir}', '${sysconfdir}', d)}/modprobe.d" > > > > Absolutely not. We are not having yet more kernel classes just for two > > variables. > > Ok. I considered adding them to bitbake.conf where all other similar *dir > variables are defined, but I opted for a bbclass since I expect it to only > be relatively few recipes that need either of them (we currently have ~10 > non-module recipes that could use them). bitbake.conf is also an absolute no. > > There is probably a better way moving some definitions to a new conf > > file for the kernel in general. > > I am not sure what to make of this. What I wanted to achieve was to be > able to make the two path variables available to non-module recipes. I am > not sure how that matches what you meant with "a new conf file for the > kernel". Did you actually mean a .conf file where the variables would be > added to the global state, or did you mean a .bbclass/.inc file similar to > the one I proposed, but with a more generic name so that it can take other > variables than just the two path variables above? I mean something more like meta/conf/image-uefi.conf but kernel focused. We need to do better about more focused conf/inc files. Cheers, Richard
> -----Original Message----- > From: Richard Purdie <richard.purdie@linuxfoundation.org> > Sent: den 4 mars 2024 18:39 > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org > Subject: Re: [OE-core] [PATCH 1/2] kernel-module-dirs.bbclass: Add class > > On Mon, 2024-03-04 at 17:20 +0000, Peter Kjellerstedt wrote: > > > -----Original Message----- > > > From: Richard Purdie <richard.purdie@linuxfoundation.org> > > > Sent: den 4 mars 2024 17:17 > > > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org > > > Subject: Re: [OE-core] [PATCH 1/2] kernel-module-dirs.bbclass: Add class > > > > > > On Mon, 2024-03-04 at 16:57 +0100, Peter Kjellerstedt wrote: > > > > Split out the two variables modulesloaddir and modprobedir from > > > > kernel-module-split.bbclass as they can be useful to other recipes than > > > > kernel module recipes. > > > > > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> > > > > --- > > > > meta/classes-recipe/kernel-module-dirs.bbclass | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > create mode 100644 meta/classes-recipe/kernel-module-dirs.bbclass > > > > > > > > diff --git a/meta/classes-recipe/kernel-module-dirs.bbclass b/meta/classes-recipe/kernel-module-dirs.bbclass > > > > new file mode 100644 > > > > index 0000000000..eecc36ab52 > > > > --- /dev/null > > > > +++ b/meta/classes-recipe/kernel-module-dirs.bbclass > > > > @@ -0,0 +1,8 @@ > > > > +# > > > > +# Copyright OpenEmbedded Contributors > > > > +# > > > > +# SPDX-License-Identifier: MIT > > > > +# > > > > + > > > > +modulesloaddir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_libdir}', '${sysconfdir}', d)}/modules-load.d" > > > > +modprobedir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_base_libdir}', '${sysconfdir}', d)}/modprobe.d" > > > > > > Absolutely not. We are not having yet more kernel classes just for two > > > variables. > > > > Ok. I considered adding them to bitbake.conf where all other similar *dir > > variables are defined, but I opted for a bbclass since I expect it to only > > be relatively few recipes that need either of them (we currently have ~10 > > non-module recipes that could use them). > > bitbake.conf is also an absolute no. Yes, that was what I expected. > > > > There is probably a better way moving some definitions to a new conf > > > file for the kernel in general. > > > > I am not sure what to make of this. What I wanted to achieve was to be > > able to make the two path variables available to non-module recipes. I am > > not sure how that matches what you meant with "a new conf file for the > > kernel". Did you actually mean a .conf file where the variables would be > > added to the global state, or did you mean a .bbclass/.inc file similar to > > the one I proposed, but with a more generic name so that it can take other > > variables than just the two path variables above? > > I mean something more like meta/conf/image-uefi.conf but kernel focused. Hmm, the naming of that file messes with the expectations I've learnt over the years of working with OE. I've always thought that .conf files are used for definitions that are part of the global configuration, and .inc files are used for more local definitions. While I of course know that there is no technical difference between the two, that kind of semantics is helpful when looking at an individual file to know the context in which it is used. > > We need to do better about more focused conf/inc files. In that regard, kernel-module-dirs.bbclass was very focused. ;) Do you see a difference in, e.g., kernel-module-dirs.bbclass vs. kernel-module-dirs.inc? I.e., why is an .inc (or .conf) file more suitable than a .bbclass file in this case? One reason I can see for why a .bbclass would be preferred, is because it is only inherited once even if there are multiple inherits. E.g., say I would take the proposed kernel-module-dirs.bbclass and turn it into a more generic kernel-vars.inc file. This file would then most likely be needed by, e.g., kernel.bbclass and module.bbclass. However, based on its current contents, it would also be needed by kernel-module-split.bbclass that both of them inherit. But since it is an .inc file, requiring it from both kernel.bbclass and kernel-module-split.bbclass would result in a lot of warnings about duplicate inclusion. On the other hand, having kernel-module-split.bbclass rely on that whatever inherits it has already required the kernel-vars.inc file seems wrong. > > Cheers, > > Richard //Peter
On Mon, 2024-03-04 at 19:10 +0000, Peter Kjellerstedt wrote: > > > I mean something more like meta/conf/image-uefi.conf but kernel focused. > > Hmm, the naming of that file messes with the expectations I've learnt over > the years of working with OE. I've always thought that .conf files are > used for definitions that are part of the global configuration, and .inc > files are used for more local definitions. While I of course know that > there is no technical difference between the two, that kind of semantics > is helpful when looking at an individual file to know the context in which > it is used. > > > > > We need to do better about more focused conf/inc files. > > In that regard, kernel-module-dirs.bbclass was very focused. ;) > > Do you see a difference in, e.g., kernel-module-dirs.bbclass vs. > kernel-module-dirs.inc? I.e., why is an .inc (or .conf) file more suitable > than a .bbclass file in this case? There is a big difference, please step back and try and think about the bigger picture on this and other variable definitions. > One reason I can see for why a .bbclass would be preferred, is because it > is only inherited once even if there are multiple inherits. E.g., say I > would take the proposed kernel-module-dirs.bbclass and turn it into a more > generic kernel-vars.inc file. This file would then most likely be needed > by, e.g., kernel.bbclass and module.bbclass. However, based on its current > contents, it would also be needed by kernel-module-split.bbclass that both > of them inherit. But since it is an .inc file, requiring it from both > kernel.bbclass and kernel-module-split.bbclass would result in a lot of > warnings about duplicate inclusion. On the other hand, having > kernel-module-split.bbclass rely on that whatever inherits it has already > required the kernel-vars.inc file seems wrong. What we have today with all the ever increasing maze of ever smaller bbclass files also seems very wrong. I'm trying to give some pointers about what I might find more acceptable since I very much doubt you'd accept an outright "no". Yes, what I'm proposing also isn't perfect and yes, we might need to work on actually improving some of the infrastructure if necessary but so be it, we need to try and improve this rather than making it worse. I'd also highlight that choosing to try and do this now at feature freeze is really not helpful. I do not have the time to spend on this right now. Cheers, Richard
On 4 Mar 2024, at 15:57, Peter Kjellerstedt via lists.openembedded.org <peter.kjellerstedt=axis.com@lists.openembedded.org> wrote: > +modulesloaddir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_libdir}', '${sysconfdir}', d)}/modules-load.d" > +modprobedir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_base_libdir}', '${sysconfdir}', d)}/modprobe.d” Feels like this should be fixed by changing either eudev or systemd-udev (presumably that’s the two recipes which are at blame here) so that they use the same path, as module config files changing location depending on whether systemd is enabled is terrible, especially when you have systems which built both systemd and sysv and expect to be able to build different images with different init systems from the same distro. Ross
diff --git a/meta/classes-recipe/kernel-module-dirs.bbclass b/meta/classes-recipe/kernel-module-dirs.bbclass new file mode 100644 index 0000000000..eecc36ab52 --- /dev/null +++ b/meta/classes-recipe/kernel-module-dirs.bbclass @@ -0,0 +1,8 @@ +# +# Copyright OpenEmbedded Contributors +# +# SPDX-License-Identifier: MIT +# + +modulesloaddir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_libdir}', '${sysconfdir}', d)}/modules-load.d" +modprobedir ??= "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', '${nonarch_base_libdir}', '${sysconfdir}', d)}/modprobe.d"
Split out the two variables modulesloaddir and modprobedir from kernel-module-split.bbclass as they can be useful to other recipes than kernel module recipes. Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> --- meta/classes-recipe/kernel-module-dirs.bbclass | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 meta/classes-recipe/kernel-module-dirs.bbclass