[OE-core][PATCH] kernel.bbclass: Add shared_workdir_prepare task

[OE-core][PATCH] kernel.bbclass: Add shared_workdir_prepare task

[Jose Quaresma]

The task do_compile_kernelmodules runs after the shared_workdir and
is installing some files in STAGING_KERNEL_BUILDDIR, this can races
in other recipes that depends on "virtual/kernel:do_shared_workdir"
as the STAGING_KERNEL_BUILDDIR is not fully populated when the
shared_workdir task ends.

To address this issue a new task is added in place of the previows one
so the shared_workdir will run after the do_compile_kernelmodules and
the new shared_workdir_prepare will replce of the old shared_workdir.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
---
 meta/classes/kernel.bbclass | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index 5d2f17c3be..5558769c92 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -504,7 +504,8 @@ do_kernel_version_sanity_check() {
 	exit 0
 }
 
-addtask shared_workdir after do_compile before do_compile_kernelmodules
+addtask shared_workdir_prepare after do_compile before do_compile_kernelmodules
+addtask shared_workdir after do_compile_kernelmodules
 addtask shared_workdir_setscene
 
 do_shared_workdir_setscene () {
@@ -520,10 +521,16 @@ emit_depmod_pkgdata() {
 
 PACKAGEFUNCS += "emit_depmod_pkgdata"
 
-do_shared_workdir[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
 do_shared_workdir () {
 	cd ${B}
 
+	kerneldir=${STAGING_KERNEL_BUILDDIR}
+}
+
+do_shared_workdir_prepare[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
+do_shared_workdir_prepare () {
+	cd ${B}
+
 	kerneldir=${STAGING_KERNEL_BUILDDIR}
 	install -d $kerneldir
 
-- 
2.37.1

Re: [OE-core][PATCH] kernel.bbclass: Add shared_workdir_prepare task

[Jacob Kroon]

On 7/20/22 13:30, Jose Quaresma wrote:
> The task do_compile_kernelmodules runs after the shared_workdir and
> is installing some files in STAGING_KERNEL_BUILDDIR, this can races
> in other recipes that depends on "virtual/kernel:do_shared_workdir"
> as the STAGING_KERNEL_BUILDDIR is not fully populated when the
> shared_workdir task ends.
> 
> To address this issue a new task is added in place of the previows one
> so the shared_workdir will run after the do_compile_kernelmodules and
> the new shared_workdir_prepare will replce of the old shared_workdir.
> 
> Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
> ---
>   meta/classes/kernel.bbclass | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
> index 5d2f17c3be..5558769c92 100644
> --- a/meta/classes/kernel.bbclass
> +++ b/meta/classes/kernel.bbclass
> @@ -504,7 +504,8 @@ do_kernel_version_sanity_check() {
>   	exit 0
>   }
>   
> -addtask shared_workdir after do_compile before do_compile_kernelmodules
> +addtask shared_workdir_prepare after do_compile before do_compile_kernelmodules
> +addtask shared_workdir after do_compile_kernelmodules
>   addtask shared_workdir_setscene
>   
>   do_shared_workdir_setscene () {
> @@ -520,10 +521,16 @@ emit_depmod_pkgdata() {
>   
>   PACKAGEFUNCS += "emit_depmod_pkgdata"
>   
> -do_shared_workdir[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
>   do_shared_workdir () {
>   	cd ${B}
>   
> +	kerneldir=${STAGING_KERNEL_BUILDDIR}
> +}

Does the above do anything actually useful ? I thought neither the 
current workdir or a variable set in a shell function would be preserved 
for the next task ?

> +
> +do_shared_workdir_prepare[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
> +do_shared_workdir_prepare () {
> +	cd ${B}
> +
>   	kerneldir=${STAGING_KERNEL_BUILDDIR}
>   	install -d $kerneldir
>   

Jacob

Re: [OE-core][PATCH] kernel.bbclass: Add shared_workdir_prepare task

[Jose Quaresma]

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

Hi Jacob,

Jacob Kroon <jacob.kroon@gmail.com> escreveu no dia terça, 2/08/2022 à(s)
17:19:

> On 7/20/22 13:30, Jose Quaresma wrote:
> > The task do_compile_kernelmodules runs after the shared_workdir and
> > is installing some files in STAGING_KERNEL_BUILDDIR, this can races
> > in other recipes that depends on "virtual/kernel:do_shared_workdir"
> > as the STAGING_KERNEL_BUILDDIR is not fully populated when the
> > shared_workdir task ends.
> >
> > To address this issue a new task is added in place of the previows one
> > so the shared_workdir will run after the do_compile_kernelmodules and
> > the new shared_workdir_prepare will replce of the old shared_workdir.
> >
> > Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
> > ---
> >   meta/classes/kernel.bbclass | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
> > index 5d2f17c3be..5558769c92 100644
> > --- a/meta/classes/kernel.bbclass
> > +++ b/meta/classes/kernel.bbclass
> > @@ -504,7 +504,8 @@ do_kernel_version_sanity_check() {
> >       exit 0
> >   }
> >
> > -addtask shared_workdir after do_compile before do_compile_kernelmodules
> > +addtask shared_workdir_prepare after do_compile before
> do_compile_kernelmodules
> > +addtask shared_workdir after do_compile_kernelmodules
> >   addtask shared_workdir_setscene
> >
> >   do_shared_workdir_setscene () {
> > @@ -520,10 +521,16 @@ emit_depmod_pkgdata() {
> >
> >   PACKAGEFUNCS += "emit_depmod_pkgdata"
> >
> > -do_shared_workdir[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
> >   do_shared_workdir () {
> >       cd ${B}
> >
> > +     kerneldir=${STAGING_KERNEL_BUILDDIR}
> > +}
>
> Does the above do anything actually useful ? I thought neither the
> current workdir or a variable set in a shell function would be preserved
> for the next task ?
>

It is strictly necessary to be backward compatible with what we have before,
otherwise any possible do_shared_workdir:appends that may exist in other
layers would stop working.

For setting the task work dir you can use the [dirs] flag in the task where
the last directory will be the task workdir.
https://docs.yoctoproject.org/bitbake/2.0/bitbake-user-manual/bitbake-user-manual-metadata.html#variable-flags

Without it the work dir of the task is the current directory where you run
bitbake.
About the variable created during tasks it only exists until the end of the
task where they were created.
You can check that inside $WORKDIR/temp/run.TASK, in each case is
$WORKDIR/temp/run.do_shared_workdir

So we have to keep the working directory as well as the variables from
the do_shared_workdir_prepare
that are expected at the end of the do_shared_workdir.

Jose


> > +
> > +do_shared_workdir_prepare[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
> > +do_shared_workdir_prepare () {
> > +     cd ${B}
> > +
> >       kerneldir=${STAGING_KERNEL_BUILDDIR}
> >       install -d $kerneldir
> >
>
> Jacob
>


-- 
Best regards,

José Quaresma

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

Re: [OE-core][PATCH] kernel.bbclass: Add shared_workdir_prepare task

[Bruce Ashfield]

Sorry for the late reply, but we were discussing this patch on IRC
today, and I wanted
to follow up with some questions / clarifications.

On Wed, Jul 20, 2022 at 7:30 AM Jose Quaresma <quaresma.jose@gmail.com> wrote:
>
> The task do_compile_kernelmodules runs after the shared_workdir and
> is installing some files in STAGING_KERNEL_BUILDDIR, this can races
> in other recipes that depends on "virtual/kernel:do_shared_workdir"
> as the STAGING_KERNEL_BUILDDIR is not fully populated when the
> shared_workdir task ends.

I remember considering this when we did the update of the module.lds in
do_compile_kernemodules(), but we had at the time (and still do) use cases
that there are no modules, so there isn't always an update to the shared_workdir

Is that the race that you were seeing ? And dependent builds are seeing
missing elements of it ? Or is there something else ?

We also were (and also still are) concerned about making the already long
kernel build dependency even longer, without a way to at least allow
recipe authors that know what they are doing, to avoid the extra time required
for modules to build.

Which leads to one question. Tasks that do need module.lds or update it,
could just use do_compile_kernelmodules() as their explicit dependency.
Was that considered and did meet the need ? Admittedly doing that leaves
a gap that a new developer could shoot through and cause themselves
some failures or subtle mis-behaviour.

>
> To address this issue a new task is added in place of the previows one
> so the shared_workdir will run after the do_compile_kernelmodules and
> the new shared_workdir_prepare will replce of the old shared_workdir.
>
> Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
> ---
>  meta/classes/kernel.bbclass | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
> index 5d2f17c3be..5558769c92 100644
> --- a/meta/classes/kernel.bbclass
> +++ b/meta/classes/kernel.bbclass
> @@ -504,7 +504,8 @@ do_kernel_version_sanity_check() {
>         exit 0
>  }
>
> -addtask shared_workdir after do_compile before do_compile_kernelmodules
> +addtask shared_workdir_prepare after do_compile before do_compile_kernelmodules
> +addtask shared_workdir after do_compile_kernelmodules

Assuming that we do go for the extra compilation in the default shared_workdir
dependency chain, can't we just change shared_workdir to be after
do_compile_kernelmodules
for a smaller footprint change ? ... unless the
do_compile_kernelmodules() are using
part of the shared_workdir for its build, but I don't recall that
being the case.

Bruce

>  addtask shared_workdir_setscene
>
>  do_shared_workdir_setscene () {
> @@ -520,10 +521,16 @@ emit_depmod_pkgdata() {
>
>  PACKAGEFUNCS += "emit_depmod_pkgdata"
>
> -do_shared_workdir[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
>  do_shared_workdir () {
>         cd ${B}
>
> +       kerneldir=${STAGING_KERNEL_BUILDDIR}
> +}
> +
> +do_shared_workdir_prepare[cleandirs] += " ${STAGING_KERNEL_BUILDDIR}"
> +do_shared_workdir_prepare () {
> +       cd ${B}
> +
>         kerneldir=${STAGING_KERNEL_BUILDDIR}
>         install -d $kerneldir
>
> --
> 2.37.1
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#168359): https://lists.openembedded.org/g/openembedded-core/message/168359
> Mute This Topic: https://lists.openembedded.org/mt/92502346/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