[OE-core] [PATCH] archiver.bbclass: Fix archiver interaction with kernel recipes

[OE-core] [PATCH] archiver.bbclass: Fix archiver interaction with kernel recipes

[Phil Reid]

Changes to the logic of is_work_shared where made in
commit: 5fbb4ca8da4f4f1ea426275c45634802dcb5a575
"archiver.bbclass: Improve work-shared checking"

The resuled in a change of the logic (simplifed here) from:
  inherits(gcc-source) or inherits(kernel) or (inherits(kernelsrc) and srcin(work-shared))
to just
  srcin(work-shared)

With INHERIT += "archiver" in the local.conf and a kernel recipe that uses
KERNEL_PACKAGE_NAME. When KERNEL_PACKAGE_NAME is defined the kernel
source is not placed into work-shared, but the archiver ends up
deleting the source folder in the work dir, and the build subsequently fails.

Restore the previous logic while also mainting the referenced commits intent
to consider all recipes that use work-shared. Logic is now
  inherits(gcc-source) or inherits(kernel) or srcin(work-shared)

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 meta/classes/archiver.bbclass | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meta/classes/archiver.bbclass b/meta/classes/archiver.bbclass
index 9d286224d6..2112b696ce 100644
--- a/meta/classes/archiver.bbclass
+++ b/meta/classes/archiver.bbclass
@@ -472,9 +472,12 @@ def create_diff_gz(d, src_orig, src, ar_outdir):
         os.chdir(cwd)
 
 def is_work_shared(d):
+    pn = d.getVar('PN')
     sharedworkdir = os.path.join(d.getVar('TMPDIR'), 'work-shared')
     sourcedir = os.path.realpath(d.getVar('S'))
-    return sourcedir.startswith(sharedworkdir)
+    return sourcedir.startswith(sharedworkdir) or \
+      pn.startswith('gcc-source') or bb.data.inherits_class('kernel', d)
+
 
 # Run do_unpack and do_patch
 python do_unpack_and_patch() {
-- 
2.34.1

Re: [OE-core] [PATCH] archiver.bbclass: Fix archiver interaction with kernel recipes

[Richard Purdie]

On Thu, 2024-10-10 at 13:25 +0800, Phil Reid via lists.openembedded.org
wrote:
> Changes to the logic of is_work_shared where made in
> commit: 5fbb4ca8da4f4f1ea426275c45634802dcb5a575
> "archiver.bbclass: Improve work-shared checking"
> 
> The resuled in a change of the logic (simplifed here) from:
>   inherits(gcc-source) or inherits(kernel) or (inherits(kernelsrc)
> and srcin(work-shared))
> to just
>   srcin(work-shared)
> 
> With INHERIT += "archiver" in the local.conf and a kernel recipe that
> uses
> KERNEL_PACKAGE_NAME. When KERNEL_PACKAGE_NAME is defined the kernel
> source is not placed into work-shared, but the archiver ends up
> deleting the source folder in the work dir, and the build
> subsequently fails.
> 
> Restore the previous logic while also mainting the referenced commits
> intent
> to consider all recipes that use work-shared. Logic is now
>   inherits(gcc-source) or inherits(kernel) or srcin(work-shared)
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  meta/classes/archiver.bbclass | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

You're effectively reverting that commit whilst changing the logic so
it looks slightly different.

Was there a problem with gcc-source archiving as I notice the patch
adds gcc-source back too?

The commit message isn't quite accurate as gcc-source isn't something
which gets inherited.

Does this only happen with kernel recipes which use
KERNEL_PACKAGE_NAME? That might be the key missing detail which would
allow us to reproduce the failure.

Cheers,

Richard

Re: [OE-core] [PATCH] archiver.bbclass: Fix archiver interaction with kernel recipes

[Phil Reid]

On 10/10/2024 15:17, Richard Purdie wrote:
> On Thu, 2024-10-10 at 13:25 +0800, Phil Reid via lists.openembedded.org
> wrote:
>> Changes to the logic of is_work_shared where made in
>> commit: 5fbb4ca8da4f4f1ea426275c45634802dcb5a575
>> "archiver.bbclass: Improve work-shared checking"
>>
>> The resuled in a change of the logic (simplifed here) from:
>>    inherits(gcc-source) or inherits(kernel) or (inherits(kernelsrc)
>> and srcin(work-shared))
>> to just
>>    srcin(work-shared)
>>
>> With INHERIT += "archiver" in the local.conf and a kernel recipe that
>> uses
>> KERNEL_PACKAGE_NAME. When KERNEL_PACKAGE_NAME is defined the kernel
>> source is not placed into work-shared, but the archiver ends up
>> deleting the source folder in the work dir, and the build
>> subsequently fails.
>>
>> Restore the previous logic while also mainting the referenced commits
>> intent
>> to consider all recipes that use work-shared. Logic is now
>>    inherits(gcc-source) or inherits(kernel) or srcin(work-shared)
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>   meta/classes/archiver.bbclass | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> You're effectively reverting that commit whilst changing the logic so
> it looks slightly different.
> 
> Was there a problem with gcc-source archiving as I notice the patch
> adds gcc-source back too?
To be honest I didn't test without the gcc-source, I reverted the commit first after
finding that commit to be the cause and modified it so it consider it always considered
work-shared path and called it done and the old logic conditions.

> 
> The commit message isn't quite accurate as gcc-source isn't something
> which gets inherited.
Fair enough.

> 
> Does this only happen with kernel recipes which use
> KERNEL_PACKAGE_NAME? That might be the key missing detail which would
> allow us to reproduce the failure.
> 

I haven't seen the problem on a virtual/kernel.
When KERNEL_PACKAGE_NAME is specified the kernel source isn't put into work-shared.


How would you like to see this fixed?


-- 
Regards
Phil Reid
  
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au
  
23 Junction Parade, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100

Email: preid@electromag.com.au

Re: [OE-core] [PATCH] archiver.bbclass: Fix archiver interaction with kernel recipes

[Richard Purdie]

On Fri, 2024-10-11 at 07:40 +0800, Phil Reid wrote:
> On 10/10/2024 15:17, Richard Purdie wrote:
> > On Thu, 2024-10-10 at 13:25 +0800, Phil Reid via lists.openembedded.org
> > wrote:
> > > Changes to the logic of is_work_shared where made in
> > > commit: 5fbb4ca8da4f4f1ea426275c45634802dcb5a575
> > > "archiver.bbclass: Improve work-shared checking"
> > > 
> > > The resuled in a change of the logic (simplifed here) from:
> > >    inherits(gcc-source) or inherits(kernel) or (inherits(kernelsrc)
> > > and srcin(work-shared))
> > > to just
> > >    srcin(work-shared)
> > > 
> > > With INHERIT += "archiver" in the local.conf and a kernel recipe that
> > > uses
> > > KERNEL_PACKAGE_NAME. When KERNEL_PACKAGE_NAME is defined the kernel
> > > source is not placed into work-shared, but the archiver ends up
> > > deleting the source folder in the work dir, and the build
> > > subsequently fails.
> > > 
> > > Restore the previous logic while also mainting the referenced commits
> > > intent
> > > to consider all recipes that use work-shared. Logic is now
> > >    inherits(gcc-source) or inherits(kernel) or srcin(work-shared)
> > > 
> > > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > > ---
> > >   meta/classes/archiver.bbclass | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > You're effectively reverting that commit whilst changing the logic so
> > it looks slightly different.
> > 
> > Was there a problem with gcc-source archiving as I notice the patch
> > adds gcc-source back too?
> To be honest I didn't test without the gcc-source, I reverted the commit first after
> finding that commit to be the cause and modified it so it consider it always considered
> work-shared path and called it done and the old logic conditions.
> 
> > 
> > The commit message isn't quite accurate as gcc-source isn't something
> > which gets inherited.
> Fair enough.
> 
> > 
> > Does this only happen with kernel recipes which use
> > KERNEL_PACKAGE_NAME? That might be the key missing detail which would
> > allow us to reproduce the failure.
> > 
> 
> I haven't seen the problem on a virtual/kernel.
> When KERNEL_PACKAGE_NAME is specified the kernel source isn't put into work-shared.
> 
> 
> How would you like to see this fixed?

I deal with many patches a day and part of the review process is to see
if a change makes sense. So far, this patch does not add up properly.
What I need is a change which makes sense and is something I can
review, agree with and merge. What I don't have is time to deep dive
into every issue and work out the proper fix, much as I would love to.
So far on this issue, the information I do have isn't adding up
correctly.

Taking what you say above, "KERNEL_PACKAGE_NAME is specified the kernel
source isn't put into work-shared" - that makes sense.

If the kernel isn't being put into shared workdir, "is_work_shared()"
should be returning False in this case. Looking at the logic, I believe
that is true and it will for a KERNEL_PACKAGE_NAME recipe.

So what you're probably saying is that the code should be using the
"shared" codepath for the KERNEL_PACKAGE_NAME even though the workdir
isn't shared. At this point I'm just having to speculate though.

So how would I like to see this fixed? I'd like to see an explaintion
of the problem which makes sense and I'd like to see changes which
don't make the code "worse", i.e. making a "is_work_shared()" function
return True when it isn't.

It sounds like there is a deeper problem with the archiver code which
needs fixing at the root of the issue. Again, I'm back to speculating
though.

Cheers,

Richard

Re: [OE-core] [PATCH] archiver.bbclass: Fix archiver interaction with kernel recipes

[Robert Yang]

Hi Phil,

On 10/11/24 19:57, Richard Purdie wrote:
> On Fri, 2024-10-11 at 07:40 +0800, Phil Reid wrote:
>> On 10/10/2024 15:17, Richard Purdie wrote:
>>> On Thu, 2024-10-10 at 13:25 +0800, Phil Reid via lists.openembedded.org
>>> wrote:
>>>> Changes to the logic of is_work_shared where made in
>>>> commit: 5fbb4ca8da4f4f1ea426275c45634802dcb5a575
>>>> "archiver.bbclass: Improve work-shared checking"
>>>>
>>>> The resuled in a change of the logic (simplifed here) from:
>>>>     inherits(gcc-source) or inherits(kernel) or (inherits(kernelsrc)
>>>> and srcin(work-shared))
>>>> to just
>>>>     srcin(work-shared)
>>>>
>>>> With INHERIT += "archiver" in the local.conf and a kernel recipe that
>>>> uses
>>>> KERNEL_PACKAGE_NAME. When KERNEL_PACKAGE_NAME is defined the kernel
>>>> source is not placed into work-shared, but the archiver ends up
>>>> deleting the source folder in the work dir, and the build
>>>> subsequently fails.
>>>>
>>>> Restore the previous logic while also mainting the referenced commits
>>>> intent
>>>> to consider all recipes that use work-shared. Logic is now
>>>>     inherits(gcc-source) or inherits(kernel) or srcin(work-shared)
>>>>
>>>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>>>> ---
>>>>    meta/classes/archiver.bbclass | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> You're effectively reverting that commit whilst changing the logic so
>>> it looks slightly different.
>>>
>>> Was there a problem with gcc-source archiving as I notice the patch
>>> adds gcc-source back too?
>> To be honest I didn't test without the gcc-source, I reverted the commit first after
>> finding that commit to be the cause and modified it so it consider it always considered
>> work-shared path and called it done and the old logic conditions.
>>
>>>
>>> The commit message isn't quite accurate as gcc-source isn't something
>>> which gets inherited.
>> Fair enough.
>>
>>>
>>> Does this only happen with kernel recipes which use
>>> KERNEL_PACKAGE_NAME? That might be the key missing detail which would
>>> allow us to reproduce the failure.
>>>
>>
>> I haven't seen the problem on a virtual/kernel.
>> When KERNEL_PACKAGE_NAME is specified the kernel source isn't put into work-shared.
>>
>>
>> How would you like to see this fixed?
> 
> I deal with many patches a day and part of the review process is to see
> if a change makes sense. So far, this patch does not add up properly.
> What I need is a change which makes sense and is something I can
> review, agree with and merge. What I don't have is time to deep dive
> into every issue and work out the proper fix, much as I would love to.
> So far on this issue, the information I do have isn't adding up
> correctly.
> 
> Taking what you say above, "KERNEL_PACKAGE_NAME is specified the kernel
> source isn't put into work-shared" - that makes sense.
> 
> If the kernel isn't being put into shared workdir, "is_work_shared()"
> should be returning False in this case. Looking at the logic, I believe
> that is true and it will for a KERNEL_PACKAGE_NAME recipe.
> 
> So what you're probably saying is that the code should be using the
> "shared" codepath for the KERNEL_PACKAGE_NAME even though the workdir
> isn't shared. At this point I'm just having to speculate though.
> 
> So how would I like to see this fixed? I'd like to see an explaintion
> of the problem which makes sense and I'd like to see changes which
> don't make the code "worse", i.e. making a "is_work_shared()" function
> return True when it isn't.
> 
> It sounds like there is a deeper problem with the archiver code which
> needs fixing at the root of the issue. Again, I'm back to speculating
> though.

Can you provide a simple reproducer, please? I'd like to look into the issue if
I reproduce it.

// Robert

> 
> Cheers,
> 
> Richard
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>