[PATCH] classes/patch: move QUILT_PC for patching consistency

[PATCH] classes/patch: move QUILT_PC for patching consistency

[Andriy Danylovskyy]

This will move the quilt cache from the default location '$S/.pc' to
'$S/patches/.pc', to ensure source invalidation always wipes it out,
together with all patches.

Recipes which set $S to $WORKDIR are susceptible to a weird issue: if
a source file is patched by quilt (a .bbappend adds a patch), updates
to it are ignored by incremental builds, the first obsolete version is
picked again and again. This is because quilt keeps its own cache in
'$S/.pc', and this one survives source invalidation on do_unpack.

This is a follow-up for a56fb90dc380 and 42a513489dc6

Signed-off-by: Andriy Danylovskyy <andriy.danylovskyy@streamunlimited.com>
---
 meta/classes-global/patch.bbclass | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meta/classes-global/patch.bbclass b/meta/classes-global/patch.bbclass
index e3157c7b18..6fcac18d9c 100644
--- a/meta/classes-global/patch.bbclass
+++ b/meta/classes-global/patch.bbclass
@@ -5,6 +5,9 @@
 # Point to an empty file so any user's custom settings don't break things
 QUILTRCFILE ?= "${STAGING_ETCDIR_NATIVE}/quiltrc"
 
+# Move quilt's cache to ensure it always gets removed together with "patches"
+export QUILT_PC = "${S}/patches/.pc"
+
 PATCHDEPENDENCY = "${PATCHTOOL}-native:do_populate_sysroot"
 
 # There is a bug in patch 2.7.3 and earlier where index lines
-- 
2.17.1

Re: [PATCH] classes/patch: move QUILT_PC for patching consistency

[Andriy Danylovskyy]

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

I am aware of the "force-unpack + patch" scenario, discussed in https://lists.yoctoproject.org/g/yocto/topic/89948013#56602 ,
and this patch, sadly, doesn't fix that.
But it does fix another face of it, which IMO is even worse than a failing build - a false positive, where updates to a source are ignored,
and the package is "successfully" built from the previous version.

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

Re: [OE-core] [PATCH] classes/patch: move QUILT_PC for patching consistency

[Richard Purdie]

On Mon, 2022-09-26 at 11:06 +0200, Andriy Danylovskyy wrote:
> This will move the quilt cache from the default location '$S/.pc' to
> '$S/patches/.pc', to ensure source invalidation always wipes it out,
> together with all patches.
> 
> Recipes which set $S to $WORKDIR are susceptible to a weird issue:

There are a number of problems with recipes which use S = WORKDIR
unfortunately. Another is that rerunning fetch/unpack doesn't clean up
files properly and can lead to build corruption.

I'm leaning towards making S == WORKDIR a warning and migrating recipes
to always use a subdir. That isn't entirely straight forward but
probably the only way to solve all the issues.

>  if
> a source file is patched by quilt (a .bbappend adds a patch), updates
> to it are ignored by incremental builds, the first obsolete version is
> picked again and again. This is because quilt keeps its own cache in
> '$S/.pc', and this one survives source invalidation on do_unpack.
> 
> This is a follow-up for a56fb90dc380 and 42a513489dc6
> 
> Signed-off-by: Andriy Danylovskyy <andriy.danylovskyy@streamunlimited.com>
> ---
>  meta/classes-global/patch.bbclass | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/meta/classes-global/patch.bbclass b/meta/classes-global/patch.bbclass
> index e3157c7b18..6fcac18d9c 100644
> --- a/meta/classes-global/patch.bbclass
> +++ b/meta/classes-global/patch.bbclass
> @@ -5,6 +5,9 @@
>  # Point to an empty file so any user's custom settings don't break things
>  QUILTRCFILE ?= "${STAGING_ETCDIR_NATIVE}/quiltrc"
>  
> +# Move quilt's cache to ensure it always gets removed together with "patches"
> +export QUILT_PC = "${S}/patches/.pc"
> +

This would break all other commandline use of quilt without the right
environment. Sadly that is a usecase I personally use quite heavily too
:/.

Cheers,

Richard

Re: [OE-core] [PATCH] classes/patch: move QUILT_PC for patching consistency

[Martin Jansa]

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

> I am aware of the "force-unpack + patch" scenario, discussed in
https://lists.yoctoproject.org/g/yocto/topic/89948013#56602, and this
patch, sadly, doesn't fix that.

Wasn't this scenario fixed by: c9d36882044b1c633d8611a77df54cd68c9bee25 ?

It was for me.

On Mon, Sep 26, 2022 at 11:13 AM Andriy Danylovskyy <
andriy.danylovskyy@streamunlimited.com> wrote:

> I am aware of the "force-unpack + patch" scenario, discussed in
> https://lists.yoctoproject.org/g/yocto/topic/89948013#56602,
> and this patch, sadly, doesn't fix that.
> But it does fix another face of it, which IMO is even worse than a failing
> build - a false positive, where updates to a source are ignored,
> and the package is "successfully" built from the previous version.
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#171055):
> https://lists.openembedded.org/g/openembedded-core/message/171055
> Mute This Topic: https://lists.openembedded.org/mt/93922956/3617156
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> Martin.Jansa@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

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

Re: [PATCH] classes/patch: move QUILT_PC for patching consistency

[Andriy Danylovskyy]

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

On 26/09/2022 12:23, Richard Purdie wrote:

> 
> On Mon, 2022-09-26 at 11:06 +0200, Andriy Danylovskyy wrote:
> 
>> This will move the quilt cache from the default location '$S/.pc' to
>> '$S/patches/.pc', to ensure source invalidation always wipes it out,
>> together with all patches.
>> 
>> Recipes which set $S to $WORKDIR are
>> susceptible to a weird issue:
>> 
> 
> There are a number of problems with recipes which use S = WORKDIR
> unfortunately. Another is that rerunning fetch/unpack doesn't clean up
> files properly and can lead to build corruption.
> 
> I'm leaning towards
> making S == WORKDIR a warning and migrating recipes
> to always use a
> subdir. That isn't entirely straight forward but
> probably the only way to
> solve all the issues.
> 

Anything that doesn't let it "pass" silently would be already a big improvement. Although deprecation with a warning sounds like a few more years to go into all affected projects.

> 
>  
>>  if
>> a source file is patched by quilt (a .bbappend adds a patch), updates
>> to it are ignored by incremental builds, the first obsolete version is
>> picked again and again. This is because quilt keeps its own cache in
>> '$S/.pc', and this one survives source invalidation on do_unpack.
>> 
>> This is
>> a follow-up for a56fb90dc380 and 42a513489dc6
>> 
>> Signed-off-by: Andriy
>> Danylovskyy <andriy.danylovskyy@streamunlimited.com>
>> ---
>> 
>> meta/classes-global/patch.bbclass | 3 +++
>>  1 file changed, 3 insertions(+)
>> diff --git a/meta/classes-global/patch.bbclass
>> b/meta/classes-global/patch.bbclass
>> index e3157c7b18..6fcac18d9c 100644
>> --- a/meta/classes-global/patch.bbclass
>> +++
>> b/meta/classes-global/patch.bbclass
>> @@ -5,6 +5,9 @@
>>  # Point to an empty
>> file so any user's custom settings don't break things
>>  QUILTRCFILE ?=
>> "${STAGING_ETCDIR_NATIVE}/quiltrc"
>>  
>> +# Move quilt's cache to ensure it
>> always gets removed together with "patches"
>> +export QUILT_PC =
>> "${S}/patches/.pc"
>> +
>> 
> 
> This would break all other commandline use of quilt without the right
> environment. Sadly that is a usecase I personally use quite heavily too
> :/.
> 
> Cheers,
> 
> Richard
> 

I can't think of any patching in a recipe workdir, outside of do_patch and the devtool, so this wasn't taken into account. But what do I know about other people's workflows...   Then another (dirtier?) option would be to patch quilt-native itself, setting QUILT_PC to the relative ./patches/.pc

--
Best regards,
Andriy

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

Re: [OE-core] [PATCH] classes/patch: move QUILT_PC for patching consistency

[Richard Purdie]

On Mon, 2022-09-26 at 04:46 -0700, Andriy Danylovskyy wrote:
> On 26/09/2022 12:23, Richard Purdie wrote:
> > On Mon, 2022-09-26 at 11:06 +0200, Andriy Danylovskyy wrote:
> > > This will move the quilt cache from the default location '$S/.pc'
> > > to
> > > '$S/patches/.pc', to ensure source invalidation always wipes it
> > > out,
> > > together with all patches.
> > > 
> > > Recipes which set $S to $WORKDIR are susceptible to a weird
> > > issue:
> > There are a number of problems with recipes which use S = WORKDIR
> > unfortunately. Another is that rerunning fetch/unpack doesn't clean
> > up
> > files properly and can lead to build corruption.
> > 
> > I'm leaning towards making S == WORKDIR a warning and migrating
> > recipes
> > to always use a subdir. That isn't entirely straight forward but
> > probably the only way to solve all the issues.
> Anything that doesn't let it "pass" silently would be already a big
> improvement. Although deprecation with a warning sounds like a few
> more years to go into all affected projects.

It wouldn't be a quick fix for the stable releases certainly but it
would hopefully fairly quickly filter down through from master if we
did start showing warnings.

I have the warnings patch running locally and have had for a while.
Converting recipes isn't as easy as you'd think though so I'm try
trying to work out a better way to handle that.

We may also have to go a step further and fetch into a subdir in order
to address all the issues so I haven't really found a good answer to
all the problems that I'm happy to promote and suggest we change to.

> 
> 
> >  
> > >  if
> > > a source file is patched by quilt (a .bbappend adds a patch),
> > > updates
> > > to it are ignored by incremental builds, the first obsolete
> > > version is
> > > picked again and again. This is because quilt keeps its own cache
> > > in
> > > '$S/.pc', and this one survives source invalidation on do_unpack.
> > > 
> > > This is a follow-up for a56fb90dc380 and 42a513489dc6
> > > 
> > > Signed-off-by: Andriy Danylovskyy
> > > <andriy.danylovskyy@streamunlimited.com>
> > > ---
> > >  meta/classes-global/patch.bbclass | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/meta/classes-global/patch.bbclass b/meta/classes-
> > > global/patch.bbclass
> > > index e3157c7b18..6fcac18d9c 100644
> > > --- a/meta/classes-global/patch.bbclass
> > > +++ b/meta/classes-global/patch.bbclass
> > > @@ -5,6 +5,9 @@
> > >  # Point to an empty file so any user's custom settings don't
> > > break things
> > >  QUILTRCFILE ?= "${STAGING_ETCDIR_NATIVE}/quiltrc"
> > >  
> > > +# Move quilt's cache to ensure it always gets removed together
> > > with "patches"
> > > +export QUILT_PC = "${S}/patches/.pc"
> > > +
> > This would break all other commandline use of quilt without the
> > right
> > environment. Sadly that is a usecase I personally use quite heavily
> > too
> > :/.
> > 
> > Cheers,
> > 
> > Richard
> I can't think of any patching in a recipe workdir, outside of
> do_patch and the devtool, so this wasn't taken into account. But what
> do I know about other people's workflows... 

I'm old school and have used quilt for patching kernels for a couple of
decades and have used it before devtool (or even git) existed! There
are probably better ways now but quilt is muscle memory.

>   Then another (dirtier?) option would be to patch quilt-native
> itself, setting QUILT_PC to the relative ./patches/.pc

Sadly it doesn't help my use case as I'm used to changing to some
random WORKDIR and using the host's copy of quilt to save messing with
PATH :/.

Cheers,

Richard

Re: [PATCH] classes/patch: move QUILT_PC for patching consistency

[Andriy Danylovskyy]

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

Thanks Martin. Yes, it does, just tried it on the latest master. When I cross-checked this before, I must have done it on an older project, with quilt 0.66 (and applying c9d36882044 there doesn't help, either).

On 26/09/2022 13:21, Martin Jansa wrote:

> 
> > I am aware of the "force-unpack + patch" scenario, discussed in https://lists.yoctoproject.org/g/yocto/topic/89948013#56602
> , and this patch, sadly, doesn't fix that.
> 
> Wasn't this scenario fixed by: c9d36882044b1c633d8611a77df54cd68c9bee25 ?
> 
> It was for me.
>

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

Re: [PATCH] classes/patch: move QUILT_PC for patching consistency

[Andriy Danylovskyy]

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

On 26/09/2022 14:21, Richard Purdie wrote:

> 
> I'm old school and have used quilt for patching kernels for a couple of
> decades and have used it before devtool (or even git) existed! There
> are
> probably better ways now but quilt is muscle memory.
> 
> 
>>   Then another (dirtier?) option would be to patch quilt-native
>> itself,
>> setting QUILT_PC to the relative ./patches/.pc
>> 
> 
> Sadly it doesn't help my use case as I'm used to changing to some
> random
> WORKDIR and using the host's copy of quilt to save messing with
> PATH :/.
> 
> Cheers,
> 
> Richard
> 

I see, you never imagine all use cases until you ask.
But the issue needs a solution, one way or another. What's especially bad is this can go unnoticed for quite a while.

--
Best regards,
Andriy

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