| Message ID | 20241114054808.2945905-1-a-shenai@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ryan Eatmon |
| Headers | show |
| Series | [meta-ti,master/scarthgap,v1] meta-ti: Linux configs: Bring selinux related configs | expand |
There are several issues with the patch itself, but that is secondary. The primary issue is that this adds a new layer dependency to meta-ti-bsp (which is also not explicitly configured in layer.conf) - that is a very undesirable thing for a BSP layer. This should be done in a Distro or Product layer (I'm not even shure meta-arago-distro should have this by default, to be honest). On Thu, Nov 14, 2024 at 11:18:08AM +0530, Aashvij Shenai via lists.yoctoproject.org wrote: > Kernel configs that are important for SELinux to be included in the > Linux kernel are present in the meta-selinux layer. > > Ideally, we wouldn't want to be calling a file from another > layer since it would become messy. However, bringing those configs > into meta-ti layer would hit maintainbility issues. > > The root cause of this problem lies in the recipe name. While > meta-selinux names their bbapend as linux_yocto_%.bbappend, TI has their > recipe named as linux-ti-staging_%.bb > > Signed-off-by: Aashvij Shenai <a-shenai@ti.com> > --- > .../linux/linux-ti-staging_%.bbappend | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > create mode 100644 meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend > > diff --git a/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend > new file mode 100644 > index 00000000..460df5de > --- /dev/null > +++ b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend > @@ -0,0 +1,15 @@ > +# The meta-selinux layer includes an selinux.cfg file containing > +# configs necessary for the Linux kernel to enable SELinux > + > +# In order to reduce maintainability issues, the file will > +# be retained in meta-selinux layer > + > +FILESEXTRAPATHS:prepend := "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', '${TOPDIR}/../sources/meta-selinux/recipes-kernel/linux/files:', '', d)}" > + > +SRC_URI += "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', 'file://selinux.cfg', '', d)}" > + > +do_configure:append() { > + if echo "${DISTRO_FEATURES}" | grep -q "selinux"; then > + cat ${WORKDIR}/selinux.cfg >> ${B}/.config > + fi > +} > \ No newline at end of file > -- > 2.34.1 >
On 11/13/2024 11:48 PM, Aashvij Shenai wrote: > Kernel configs that are important for SELinux to be included in the > Linux kernel are present in the meta-selinux layer. > > Ideally, we wouldn't want to be calling a file from another > layer since it would become messy. However, bringing those configs > into meta-ti layer would hit maintainbility issues. > > The root cause of this problem lies in the recipe name. While > meta-selinux names their bbapend as linux_yocto_%.bbappend, TI has their > recipe named as linux-ti-staging_%.bb > > Signed-off-by: Aashvij Shenai <a-shenai@ti.com> > --- > .../linux/linux-ti-staging_%.bbappend | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > create mode 100644 meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend > > diff --git a/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend > new file mode 100644 > index 00000000..460df5de > --- /dev/null > +++ b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend > @@ -0,0 +1,15 @@ > +# The meta-selinux layer includes an selinux.cfg file containing > +# configs necessary for the Linux kernel to enable SELinux > + > +# In order to reduce maintainability issues, the file will > +# be retained in meta-selinux layer > + > +FILESEXTRAPATHS:prepend := "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', '${TOPDIR}/../sources/meta-selinux/recipes-kernel/linux/files:', '', d)}" NAK. We cannot hard code paths like this. This assumes several things about our layer-config and directory structure that is not present in all use cases of the meta-ti layer for all customers. We cannot accept this patch as is. I just tested the following: require ${@bb.utils.contains('DISTRO_FEATURES', 'selinux', 'recipes-kernel/linux/linux-yocto_selinux.inc', '', d)} And it works fine. It pulls in the file from meta-selinux without needing a bunch of other fluff. > +SRC_URI += "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', 'file://selinux.cfg', '', d)}" > + > +do_configure:append() { > + if echo "${DISTRO_FEATURES}" | grep -q "selinux"; then > + cat ${WORKDIR}/selinux.cfg >> ${B}/.config > + fi > +} > \ No newline at end of file
On 11/14/2024 1:17 PM, Denys Dmytriyenko wrote: > There are several issues with the patch itself, but that is secondary. > > The primary issue is that this adds a new layer dependency to meta-ti-bsp > (which is also not explicitly configured in layer.conf) - that is a very > undesirable thing for a BSP layer. That is a very good point. I just sent my own reply to this patch listing the issues with it. But this is a very very good point. Question though... if the inclusion of the setup files in meta-selinux are hidden behind a DISTRO_FEATURES then are we really adding a dependency on meta-selinux? Or are we adding a conditional dependency that assumes that if you are turning on "selinux" then meta-selinux must be in the bblayers.conf? > This should be done in a Distro or Product layer (I'm not even shure > meta-arago-distro should have this by default, to be honest). > > > On Thu, Nov 14, 2024 at 11:18:08AM +0530, Aashvij Shenai via lists.yoctoproject.org wrote: >> Kernel configs that are important for SELinux to be included in the >> Linux kernel are present in the meta-selinux layer. >> >> Ideally, we wouldn't want to be calling a file from another >> layer since it would become messy. However, bringing those configs >> into meta-ti layer would hit maintainbility issues. >> >> The root cause of this problem lies in the recipe name. While >> meta-selinux names their bbapend as linux_yocto_%.bbappend, TI has their >> recipe named as linux-ti-staging_%.bb >> >> Signed-off-by: Aashvij Shenai <a-shenai@ti.com> >> --- >> .../linux/linux-ti-staging_%.bbappend | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> create mode 100644 meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >> >> diff --git a/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >> new file mode 100644 >> index 00000000..460df5de >> --- /dev/null >> +++ b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >> @@ -0,0 +1,15 @@ >> +# The meta-selinux layer includes an selinux.cfg file containing >> +# configs necessary for the Linux kernel to enable SELinux >> + >> +# In order to reduce maintainability issues, the file will >> +# be retained in meta-selinux layer >> + >> +FILESEXTRAPATHS:prepend := "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', '${TOPDIR}/../sources/meta-selinux/recipes-kernel/linux/files:', '', d)}" >> + >> +SRC_URI += "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', 'file://selinux.cfg', '', d)}" >> + >> +do_configure:append() { >> + if echo "${DISTRO_FEATURES}" | grep -q "selinux"; then >> + cat ${WORKDIR}/selinux.cfg >> ${B}/.config >> + fi >> +} >> \ No newline at end of file >> -- >> 2.34.1 >>
Thanks for the reviews. On 15/11/24 02:11, Ryan Eatmon wrote: > > > On 11/14/2024 1:17 PM, Denys Dmytriyenko wrote: >> There are several issues with the patch itself, but that is secondary. >> >> The primary issue is that this adds a new layer dependency to >> meta-ti-bsp >> (which is also not explicitly configured in layer.conf) - that is a very >> undesirable thing for a BSP layer. > > That is a very good point. I just sent my own reply to this patch > listing the issues with it. But this is a very very good point. > > Question though... if the inclusion of the setup files in meta-selinux > are hidden behind a DISTRO_FEATURES then are we really adding a > dependency on meta-selinux? Or are we adding a conditional dependency > that assumes that if you are turning on "selinux" then meta-selinux > must be in the bblayers.conf? > Would moving an updated linux-ti-staging_%.bbappend under `meta-arago/meta-arago-distro/recipes-kernel`or `meta-arago/meta-arago-extras/recipes-kernel` resolve this impasse? > > > >> This should be done in a Distro or Product layer (I'm not even shure >> meta-arago-distro should have this by default, to be honest). >> >> >> On Thu, Nov 14, 2024 at 11:18:08AM +0530, Aashvij Shenai via >> lists.yoctoproject.org wrote: >>> Kernel configs that are important for SELinux to be included in the >>> Linux kernel are present in the meta-selinux layer. >>> >>> Ideally, we wouldn't want to be calling a file from another >>> layer since it would become messy. However, bringing those configs >>> into meta-ti layer would hit maintainbility issues. >>> >>> The root cause of this problem lies in the recipe name. While >>> meta-selinux names their bbapend as linux_yocto_%.bbappend, TI has >>> their >>> recipe named as linux-ti-staging_%.bb >>> >>> Signed-off-by: Aashvij Shenai <a-shenai@ti.com> >>> --- >>> .../linux/linux-ti-staging_%.bbappend | 15 >>> +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> create mode 100644 >>> meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >>> >>> diff --git >>> a/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >>> b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >>> new file mode 100644 >>> index 00000000..460df5de >>> --- /dev/null >>> +++ b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend >>> @@ -0,0 +1,15 @@ >>> +# The meta-selinux layer includes an selinux.cfg file containing >>> +# configs necessary for the Linux kernel to enable SELinux >>> + >>> +# In order to reduce maintainability issues, the file will >>> +# be retained in meta-selinux layer >>> + >>> +FILESEXTRAPATHS:prepend := "${@bb.utils.contains('DISTRO_FEATURES', >>> 'selinux', >>> '${TOPDIR}/../sources/meta-selinux/recipes-kernel/linux/files:', '', >>> d)}" >>> + >>> +SRC_URI += "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', >>> 'file://selinux.cfg', '', d)}" >>> + >>> +do_configure:append() { >>> + if echo "${DISTRO_FEATURES}" | grep -q "selinux"; then >>> + cat ${WORKDIR}/selinux.cfg >> ${B}/.config >>> + fi >>> +} >>> \ No newline at end of file >>> -- >>> 2.34.1 >>> > Regards, Aashvij
diff --git a/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend new file mode 100644 index 00000000..460df5de --- /dev/null +++ b/meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend @@ -0,0 +1,15 @@ +# The meta-selinux layer includes an selinux.cfg file containing +# configs necessary for the Linux kernel to enable SELinux + +# In order to reduce maintainability issues, the file will +# be retained in meta-selinux layer + +FILESEXTRAPATHS:prepend := "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', '${TOPDIR}/../sources/meta-selinux/recipes-kernel/linux/files:', '', d)}" + +SRC_URI += "${@bb.utils.contains('DISTRO_FEATURES', 'selinux', 'file://selinux.cfg', '', d)}" + +do_configure:append() { + if echo "${DISTRO_FEATURES}" | grep -q "selinux"; then + cat ${WORKDIR}/selinux.cfg >> ${B}/.config + fi +} \ No newline at end of file
Kernel configs that are important for SELinux to be included in the Linux kernel are present in the meta-selinux layer. Ideally, we wouldn't want to be calling a file from another layer since it would become messy. However, bringing those configs into meta-ti layer would hit maintainbility issues. The root cause of this problem lies in the recipe name. While meta-selinux names their bbapend as linux_yocto_%.bbappend, TI has their recipe named as linux-ti-staging_%.bb Signed-off-by: Aashvij Shenai <a-shenai@ti.com> --- .../linux/linux-ti-staging_%.bbappend | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 meta-ti-bsp/recipes-kernel/linux/linux-ti-staging_%.bbappend