diff mbox series

[1/1] kernelsrc.bbclass: Move virtual/kernel:do_shared_workdir to do_configure

Message ID 73daecdd8298f7cc64730877148ea11b3f113bc4.1727069614.git.liezhi.yang@windriver.com
State Under Review
Headers show
Series [1/1] kernelsrc.bbclass: Move virtual/kernel:do_shared_workdir to do_configure | expand

Commit Message

Robert Yang Sept. 23, 2024, 5:34 a.m. UTC 
From: Robert Yang <liezhi.yang@windriver.com>

The do_shared_workdir populates build artifacts to
work-shared/${MACHINE}/kernel-build-artifacts, which isn't useful for do_patch,
make do_patch depend on it will make a lot of recipes compiling which makes
world's do_patch very slow, e.g.:

Download sources locally:
$ bitbake world --runall=fetch

* Before the patch:
$ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
real    23m31.739s
user    0m25.086s
sys     0m4.630s

* Now
$ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
real    12m25.650s
user    0m35.641s
sys     0m5.699s

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/classes-recipe/kernelsrc.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bruce Ashfield Oct. 1, 2024, 2:42 p.m. UTC | #1 
On Mon, Sep 23, 2024 at 1:34 AM Robert Yang via lists.openembedded.org
<liezhi.yang=windriver.com@lists.openembedded.org> wrote:
>
> From: Robert Yang <liezhi.yang@windriver.com>
>
> The do_shared_workdir populates build artifacts to
> work-shared/${MACHINE}/kernel-build-artifacts, which isn't useful for do_patch,
> make do_patch depend on it will make a lot of recipes compiling which makes
> world's do_patch very slow, e.g.:
>
> Download sources locally:
> $ bitbake world --runall=fetch
>
> * Before the patch:
> $ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
> real    23m31.739s
> user    0m25.086s
> sys     0m4.630s
>
> * Now
> $ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
> real    12m25.650s
> user    0m35.641s
> sys     0m5.699s

The motivation for why this is a priority should be included in
the commit message. It helps us decide if any issues this may
trigger are worth the risk.

I assume you are doing some sort of source archiving, or
licensing and all you care about is the patched source
for this, and not any potentially generated files from the
build(s) ?

See below for why I ask.

>
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  meta/classes-recipe/kernelsrc.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meta/classes-recipe/kernelsrc.bbclass b/meta/classes-recipe/kernelsrc.bbclass
> index ecb02dc9edc..f23a1e6bff2 100644
> --- a/meta/classes-recipe/kernelsrc.bbclass
> +++ b/meta/classes-recipe/kernelsrc.bbclass
> @@ -7,8 +7,8 @@
>  S = "${STAGING_KERNEL_DIR}"
>  deltask do_fetch
>  deltask do_unpack
> -do_patch[depends] += "virtual/kernel:do_shared_workdir"
>  do_patch[noexec] = "1"
> +do_configure[depends] += "virtual/kernel:do_shared_workdir"

From what I can see (bitbake expansion and dependency experts
can feel free to correct the below):

There's already an issue with the KERNEL_VERSION / LOCAL_VERSION
variables, in that they depend on STAGING_KERNEL_BUILDDIR, which
is populated from the do_shared_workdir. We are moving that population
later / to a different task with this patch, so those variables are now going to
be defined at a different point in the build. There's potential risk in that.

Looking at the routines get_kernel* that the variables are using, they
gracefully return None if the file doesn't exist, so the question is .. would
an impacted user of the variables notice a potential issue ?

One wonders if they shouldn't just propagate the exception to the caller
so it could be dealt with, and harder to ignore.

So while it does speed up do_patch, there is a behaviour change in
when the variables are going to be valid/defined.

Bruce

>  do_package[depends] += "virtual/kernel:do_populate_sysroot"
>  KERNEL_VERSION = "${@get_kernelversion_file("${STAGING_KERNEL_BUILDDIR}")}"
>  LOCAL_VERSION = "${@get_kernellocalversion_file("${STAGING_KERNEL_BUILDDIR}")}"
> --
> 2.25.1
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#204794): https://lists.openembedded.org/g/openembedded-core/message/204794
> Mute This Topic: https://lists.openembedded.org/mt/108603063/1050810
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Robert Yang Oct. 8, 2024, 3:17 p.m. UTC | #2 
Hi Bruce,

Sorry for the late reply, I just came back from holiday,

On 10/1/24 22:42, Bruce Ashfield wrote:
> On Mon, Sep 23, 2024 at 1:34 AM Robert Yang via lists.openembedded.org
> <liezhi.yang=windriver.com@lists.openembedded.org> wrote:
>>
>> From: Robert Yang <liezhi.yang@windriver.com>
>>
>> The do_shared_workdir populates build artifacts to
>> work-shared/${MACHINE}/kernel-build-artifacts, which isn't useful for do_patch,
>> make do_patch depend on it will make a lot of recipes compiling which makes
>> world's do_patch very slow, e.g.:
>>
>> Download sources locally:
>> $ bitbake world --runall=fetch
>>
>> * Before the patch:
>> $ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
>> real    23m31.739s
>> user    0m25.086s
>> sys     0m4.630s
>>
>> * Now
>> $ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
>> real    12m25.650s
>> user    0m35.641s
>> sys     0m5.699s
> 
> The motivation for why this is a priority should be included in
> the commit message. It helps us decide if any issues this may
> trigger are worth the risk.
> 
> I assume you are doing some sort of source archiving, or
> licensing and all you care about is the patched source
> for this, and not any potentially generated files from the
> build(s) ?

Yes, I'm trying to collect the patched sources for ip scanning, and there are a
lot of do_compile tasks are running when run --runall=patch which looks strange.

> 
> See below for why I ask.
> 
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> ---
>>   meta/classes-recipe/kernelsrc.bbclass | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/meta/classes-recipe/kernelsrc.bbclass b/meta/classes-recipe/kernelsrc.bbclass
>> index ecb02dc9edc..f23a1e6bff2 100644
>> --- a/meta/classes-recipe/kernelsrc.bbclass
>> +++ b/meta/classes-recipe/kernelsrc.bbclass
>> @@ -7,8 +7,8 @@
>>   S = "${STAGING_KERNEL_DIR}"
>>   deltask do_fetch
>>   deltask do_unpack
>> -do_patch[depends] += "virtual/kernel:do_shared_workdir"
>>   do_patch[noexec] = "1"
>> +do_configure[depends] += "virtual/kernel:do_shared_workdir"
> 
>>From what I can see (bitbake expansion and dependency experts
> can feel free to correct the below):
> 
> There's already an issue with the KERNEL_VERSION / LOCAL_VERSION
> variables, in that they depend on STAGING_KERNEL_BUILDDIR, which
> is populated from the do_shared_workdir. We are moving that population
> later / to a different task with this patch, so those variables are now going to
> be defined at a different point in the build. There's potential risk in that.
> 
> Looking at the routines get_kernel* that the variables are using, they
> gracefully return None if the file doesn't exist, so the question is .. would
> an impacted user of the variables notice a potential issue ?

Maybe we should make get_kernel_* report errors rather than None?

// Robert

> 
> One wonders if they shouldn't just propagate the exception to the caller
> so it could be dealt with, and harder to ignore.
> 
> So while it does speed up do_patch, there is a behaviour change in
> when the variables are going to be valid/defined.
> 
> Bruce
> 
>>   do_package[depends] += "virtual/kernel:do_populate_sysroot"
>>   KERNEL_VERSION = "${@get_kernelversion_file("${STAGING_KERNEL_BUILDDIR}")}"
>>   LOCAL_VERSION = "${@get_kernellocalversion_file("${STAGING_KERNEL_BUILDDIR}")}"
>> --
>> 2.25.1
>>
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#204794): https://lists.openembedded.org/g/openembedded-core/message/204794
>> Mute This Topic: https://lists.openembedded.org/mt/108603063/1050810
>> Group Owner: openembedded-core+owner@lists.openembedded.org
>> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>
> 
>
Bruce Ashfield Oct. 8, 2024, 3:35 p.m. UTC | #3 
On Tue, Oct 8, 2024 at 11:18 AM Robert Yang <liezhi.yang@windriver.com> wrote:
>
> Hi Bruce,
>
> Sorry for the late reply, I just came back from holiday,
>
> On 10/1/24 22:42, Bruce Ashfield wrote:
> > On Mon, Sep 23, 2024 at 1:34 AM Robert Yang via lists.openembedded.org
> > <liezhi.yang=windriver.com@lists.openembedded.org> wrote:
> >>
> >> From: Robert Yang <liezhi.yang@windriver.com>
> >>
> >> The do_shared_workdir populates build artifacts to
> >> work-shared/${MACHINE}/kernel-build-artifacts, which isn't useful for do_patch,
> >> make do_patch depend on it will make a lot of recipes compiling which makes
> >> world's do_patch very slow, e.g.:
> >>
> >> Download sources locally:
> >> $ bitbake world --runall=fetch
> >>
> >> * Before the patch:
> >> $ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
> >> real    23m31.739s
> >> user    0m25.086s
> >> sys     0m4.630s
> >>
> >> * Now
> >> $ rm -fr sstate-cache/ tmp/; time bitbake world --runall=patch
> >> real    12m25.650s
> >> user    0m35.641s
> >> sys     0m5.699s
> >
> > The motivation for why this is a priority should be included in
> > the commit message. It helps us decide if any issues this may
> > trigger are worth the risk.
> >
> > I assume you are doing some sort of source archiving, or
> > licensing and all you care about is the patched source
> > for this, and not any potentially generated files from the
> > build(s) ?
>
> Yes, I'm trying to collect the patched sources for ip scanning, and there are a
> lot of do_compile tasks are running when run --runall=patch which looks strange.
>
> >
> > See below for why I ask.
> >
> >>
> >> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> >> ---
> >>   meta/classes-recipe/kernelsrc.bbclass | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/meta/classes-recipe/kernelsrc.bbclass b/meta/classes-recipe/kernelsrc.bbclass
> >> index ecb02dc9edc..f23a1e6bff2 100644
> >> --- a/meta/classes-recipe/kernelsrc.bbclass
> >> +++ b/meta/classes-recipe/kernelsrc.bbclass
> >> @@ -7,8 +7,8 @@
> >>   S = "${STAGING_KERNEL_DIR}"
> >>   deltask do_fetch
> >>   deltask do_unpack
> >> -do_patch[depends] += "virtual/kernel:do_shared_workdir"
> >>   do_patch[noexec] = "1"
> >> +do_configure[depends] += "virtual/kernel:do_shared_workdir"
> >
> >>From what I can see (bitbake expansion and dependency experts
> > can feel free to correct the below):
> >
> > There's already an issue with the KERNEL_VERSION / LOCAL_VERSION
> > variables, in that they depend on STAGING_KERNEL_BUILDDIR, which
> > is populated from the do_shared_workdir. We are moving that population
> > later / to a different task with this patch, so those variables are now going to
> > be defined at a different point in the build. There's potential risk in that.
> >
> > Looking at the routines get_kernel* that the variables are using, they
> > gracefully return None if the file doesn't exist, so the question is .. would
> > an impacted user of the variables notice a potential issue ?
>
> Maybe we should make get_kernel_* report errors rather than None?

That was the idea that Ross and I had when I was looking through
the series.

It would definitely let us know if this is an issue, and flag it in the
future if it becomes an issue.

Bruce

>
> // Robert
>
> >
> > One wonders if they shouldn't just propagate the exception to the caller
> > so it could be dealt with, and harder to ignore.
> >
> > So while it does speed up do_patch, there is a behaviour change in
> > when the variables are going to be valid/defined.
> >
> > Bruce
> >
> >>   do_package[depends] += "virtual/kernel:do_populate_sysroot"
> >>   KERNEL_VERSION = "${@get_kernelversion_file("${STAGING_KERNEL_BUILDDIR}")}"
> >>   LOCAL_VERSION = "${@get_kernellocalversion_file("${STAGING_KERNEL_BUILDDIR}")}"
> >> --
> >> 2.25.1
> >>
> >>
> >> -=-=-=-=-=-=-=-=-=-=-=-
> >> Links: You receive all messages sent to this group.
> >> View/Reply Online (#204794): https://lists.openembedded.org/g/openembedded-core/message/204794
> >> Mute This Topic: https://lists.openembedded.org/mt/108603063/1050810
> >> Group Owner: openembedded-core+owner@lists.openembedded.org
> >> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com]
> >> -=-=-=-=-=-=-=-=-=-=-=-
> >>
> >
> >
diff mbox series

Patch

diff --git a/meta/classes-recipe/kernelsrc.bbclass b/meta/classes-recipe/kernelsrc.bbclass
index ecb02dc9edc..f23a1e6bff2 100644
--- a/meta/classes-recipe/kernelsrc.bbclass
+++ b/meta/classes-recipe/kernelsrc.bbclass
@@ -7,8 +7,8 @@ 
 S = "${STAGING_KERNEL_DIR}"
 deltask do_fetch
 deltask do_unpack
-do_patch[depends] += "virtual/kernel:do_shared_workdir"
 do_patch[noexec] = "1"
+do_configure[depends] += "virtual/kernel:do_shared_workdir"
 do_package[depends] += "virtual/kernel:do_populate_sysroot"
 KERNEL_VERSION = "${@get_kernelversion_file("${STAGING_KERNEL_BUILDDIR}")}"
 LOCAL_VERSION = "${@get_kernellocalversion_file("${STAGING_KERNEL_BUILDDIR}")}"