[RFC PATCH 0/4] fix externalsrc task dependency issues

[RFC PATCH 0/4] fix externalsrc task dependency issues

[Rasmus Villemoes]

This is all entirely untested, hence RFC. But if this doesn't work for
some reason, I think externalsrc.bbclass should grow some comments as
to why deltask is used instead of the noexec flag.

Patch 4 is a revert of patch 1, but I did it that way to facilitate
backporting, in case patch 2 is too dangerous for backporting. If that
is considered safe enough, it should be enough to apply patch 2 to
avoid the churn. In any case, patch 3 is just a little simplification
made possible by patch 2.


Rasmus Villemoes (4):
  kernel.bbclass: ensure symlink_kernsrc task gets run even with
    externalsrc
  externalsrc.bbclass: make SRCTREECOVEREDTASKS noexec instead of
    deleting them
  kernel-yocto.bbclass: remove now unnecessary externalsrc workaround
  Revert "kernel.bbclass: ensure symlink_kernsrc task gets run even with
    externalsrc"

 meta/classes/externalsrc.bbclass  | 5 +----
 meta/classes/kernel-yocto.bbclass | 6 ------
 2 files changed, 1 insertion(+), 10 deletions(-)

-- 
2.23.0

[RFC PATCH 1/4] kernel.bbclass: ensure symlink_kernsrc task gets run even with externalsrc

[Rasmus Villemoes]

Commit c5dfc2586b41 (kernel.bbclass: run do_symlink_kernsrc before
do_patch) fixed a race between do_symlink_kernsrc and
do_populate_lic. However, I missed the fact that when
externalsrc.bbclass is in use, the do_patch task doesn't exist,
meaning that do_symlink_kernsrc now doesn't get run at all, breaking
the build.

While I think the proper fix is to stop deltask'ing do_patch and
instead make it noexec in externalsrc.bbclass, a more targeted fix
suitable for backporting is to simply make this a dependency of both
do_patch and do_configure.

Fixes: c5dfc2586b41 (kernel.bbclass: run do_symlink_kernsrc before
do_patch)
Reported-by: Chanho Park <parkch98@gmail.com>

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 meta/classes/kernel.bbclass | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index 48135b3d41..78def5bbc1 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -172,7 +172,10 @@ python do_symlink_kernsrc () {
             shutil.move(s, kernsrc)
             os.symlink(kernsrc, s)
 }
-addtask symlink_kernsrc before do_patch after do_unpack
+# do_patch is normally ordered before do_configure, but
+# externalsrc.bbclass deletes do_patch, breaking the dependency of
+# do_configure on do_symlink_kernsrc.
+addtask symlink_kernsrc before do_patch do_configure after do_unpack
 
 inherit kernel-arch deploy
 
-- 
2.23.0

[RFC PATCH 2/4] externalsrc.bbclass: make SRCTREECOVEREDTASKS noexec instead of deleting them

[Rasmus Villemoes]

The preferred way to prevent a task from running is not to deltask it,
but setting the noexec flag. That way various implicit dependencies
through the task in question are preserved, avoiding ad hoc
workarounds, such as the one right here.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 meta/classes/externalsrc.bbclass | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/meta/classes/externalsrc.bbclass b/meta/classes/externalsrc.bbclass
index d200129987..7b4d8b369f 100644
--- a/meta/classes/externalsrc.bbclass
+++ b/meta/classes/externalsrc.bbclass
@@ -100,14 +100,11 @@ python () {
                 d.setVarFlag(task, 'cleandirs', ' '.join(cleandirs))
 
         fetch_tasks = ['do_fetch', 'do_unpack']
-        # If we deltask do_patch, there's no dependency to ensure do_unpack gets run, so add one
-        # Note that we cannot use d.appendVarFlag() here because deps is expected to be a list object, not a string
-        d.setVarFlag('do_configure', 'deps', (d.getVarFlag('do_configure', 'deps', False) or []) + ['do_unpack'])
 
         for task in d.getVar("SRCTREECOVEREDTASKS").split():
             if local_srcuri and task in fetch_tasks:
                 continue
-            bb.build.deltask(task, d)
+            d.setVarFlag(task, 'noexec', '1')
 
         d.prependVarFlag('do_compile', 'prefuncs', "externalsrc_compile_prefunc ")
         d.prependVarFlag('do_configure', 'prefuncs', "externalsrc_configure_prefunc ")
-- 
2.23.0

[RFC PATCH 3/4] kernel-yocto.bbclass: remove now unnecessary externalsrc workaround

[Rasmus Villemoes]

Now that SRCTREECOVEREDTASKS gets marked as noexec rather than being
deltask'ed, do_kernel_configme is still ordered after do_patch (and
recursively, after do_unpack), so this workaround is no longer needed.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 meta/classes/kernel-yocto.bbclass | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/meta/classes/kernel-yocto.bbclass b/meta/classes/kernel-yocto.bbclass
index a35c5923df..741b0b961c 100644
--- a/meta/classes/kernel-yocto.bbclass
+++ b/meta/classes/kernel-yocto.bbclass
@@ -656,12 +656,6 @@ python () {
     # If diffconfig is available, ensure it runs after kernel_configme
     if 'do_diffconfig' in d:
         bb.build.addtask('do_diffconfig', None, 'do_kernel_configme', d)
-
-    externalsrc = d.getVar('EXTERNALSRC')
-    if externalsrc:
-        # If we deltask do_patch, do_kernel_configme is left without
-        # dependencies and runs too early
-        d.setVarFlag('do_kernel_configme', 'deps', (d.getVarFlag('do_kernel_configme', 'deps', False) or []) + ['do_unpack'])
 }
 
 # extra tasks
-- 
2.23.0

[RFC PATCH 4/4] Revert "kernel.bbclass: ensure symlink_kernsrc task gets run even with externalsrc"

[Rasmus Villemoes]

With the changes to externalsrc.bbclass to make tasks noexec instead
of deleting them, we no longer need to explicitly make
do_symlink_kernsrc a dependency of do_configure.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 meta/classes/kernel.bbclass | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index 78def5bbc1..48135b3d41 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -172,10 +172,7 @@ python do_symlink_kernsrc () {
             shutil.move(s, kernsrc)
             os.symlink(kernsrc, s)
 }
-# do_patch is normally ordered before do_configure, but
-# externalsrc.bbclass deletes do_patch, breaking the dependency of
-# do_configure on do_symlink_kernsrc.
-addtask symlink_kernsrc before do_patch do_configure after do_unpack
+addtask symlink_kernsrc before do_patch after do_unpack
 
 inherit kernel-arch deploy
 
-- 
2.23.0

Re: [RFC PATCH 0/4] fix externalsrc task dependency issues

[Rasmus Villemoes]

On 22/09/2020 17.15, Rasmus Villemoes wrote:
> This is all entirely untested, hence RFC. But if this doesn't work for
> some reason, I think externalsrc.bbclass should grow some comments as
> to why deltask is used instead of the noexec flag.
> 
> Patch 4 is a revert of patch 1, but I did it that way to facilitate
> backporting, in case patch 2 is too dangerous for backporting. If that
> is considered safe enough, it should be enough to apply patch 2 to
> avoid the churn. In any case, patch 3 is just a little simplification
> made possible by patch 2.
> 
> 

Ping. Chanho, can you tell if this would fix the externalsrc problem?
Richard, any comments on the deltask vs noexec usage?

Rasmus

Re: [RFC PATCH 0/4] fix externalsrc task dependency issues

[Richard Purdie]

On Fri, 2020-09-25 at 11:06 +0200, Rasmus Villemoes wrote:
> On 22/09/2020 17.15, Rasmus Villemoes wrote:
> > This is all entirely untested, hence RFC. But if this doesn't work
> > for
> > some reason, I think externalsrc.bbclass should grow some comments
> > as
> > to why deltask is used instead of the noexec flag.
> > 
> > Patch 4 is a revert of patch 1, but I did it that way to facilitate
> > backporting, in case patch 2 is too dangerous for backporting. If
> > that
> > is considered safe enough, it should be enough to apply patch 2 to
> > avoid the churn. In any case, patch 3 is just a little
> > simplification
> > made possible by patch 2.
> > 
> > 
> 
> Ping. Chanho, can you tell if this would fix the externalsrc problem?
> Richard, any comments on the deltask vs noexec usage?

You started this patchset with "This is all entirely untested" which
I'm afraid made me rather nervous. Much as I'd love to, I don't have
time to debug every patch series people send.

I'm fairly sure the deltask vs. noexec was deliberate and Paul and I
went around in circles on this several times.

I have some memory that at least part of the problem was usability. If
someone can "see" the task in listtasks and trys to run it but nothing
happens, its very confusing for the user. By deleting it, we make it
very clear its not there.

I think there were technical issues as well, such as recipes which
inject other tasks between unpack and patch. We needed to stop those
other tasks running too which deltasks does effectively, noexec would
not.

Cheers,

Richard

Re: [RFC PATCH 0/4] fix externalsrc task dependency issues

[Rasmus Villemoes]

On 25/09/2020 11.22, Richard Purdie wrote:
> On Fri, 2020-09-25 at 11:06 +0200, Rasmus Villemoes wrote:
>> On 22/09/2020 17.15, Rasmus Villemoes wrote:
>>> This is all entirely untested, hence RFC. But if this doesn't work
>>> for
>>> some reason, I think externalsrc.bbclass should grow some comments
>>> as
>>> to why deltask is used instead of the noexec flag.
>>>
>>> Patch 4 is a revert of patch 1, but I did it that way to facilitate
>>> backporting, in case patch 2 is too dangerous for backporting. If
>>> that
>>> is considered safe enough, it should be enough to apply patch 2 to
>>> avoid the churn. In any case, patch 3 is just a little
>>> simplification
>>> made possible by patch 2.
>>>
>>>
>>
>> Ping. Chanho, can you tell if this would fix the externalsrc problem?
>> Richard, any comments on the deltask vs noexec usage?
> 
> You started this patchset with "This is all entirely untested" which
> I'm afraid made me rather nervous. Much as I'd love to, I don't have
> time to debug every patch series people send.
> 
> I'm fairly sure the deltask vs. noexec was deliberate and Paul and I
> went around in circles on this several times.
> 
> I have some memory that at least part of the problem was usability. If
> someone can "see" the task in listtasks and trys to run it but nothing
> happens, its very confusing for the user. By deleting it, we make it
> very clear its not there.

Well, perhaps listtasks could be taught to say "[noexec flag set]" in
the description - currently it seems to be somewhat random whether a
class uses noexec or deltask.

Oh, and for debugging task ordering issues, it would also be nice if
listtasks showed tasks in topological rather than alphabetical order -
that won't necessarily always show an implicit dependency that went
missing due to a deltask, but it would at least reveal some of them.

> I think there were technical issues as well, such as recipes which
> inject other tasks between unpack and patch. We needed to stop those
> other tasks running too which deltasks does effectively, noexec would
> not.

Eh, but the other side of that coin is that there are obviously tasks
injected between unpack and patch, or some other combination, which must
_not_ be prevented from running - cf. at least the two or three
workarounds that seem to become unnecessary. So can you give examples of
such tasks that _should_ implicitly not run when externalsrc disables
(one way or another) the SRCTREECOVEREDTASKS tasks? That's actually the
RFC part of this, I hope somebody with far more knowledge of various
corner cases and odd recipes can demonstrate why deltask is more
appropriate - and then, as I said, some comments to that effect in
externalsrc.bbclass would be much appreciated.

I think it would be weird for a recipe to inherit externalsrc and at the
same time define a task ordered before do_patch, then not expect that
task to run at all. So I'm genuinely curious how such examples would look.

Thanks,
Rasmus

Re: [RFC PATCH 0/4] fix externalsrc task dependency issues

[Richard Purdie]

On Fri, 2020-09-25 at 12:31 +0200, Rasmus Villemoes wrote:
> Well, perhaps listtasks could be taught to say "[noexec flag set]" in
> the description - currently it seems to be somewhat random whether a
> class uses noexec or deltask.

No. That would just confuse things even more. That would hide do_build
for example so the default target would be missing?

> Oh, and for debugging task ordering issues, it would also be nice if
> listtasks showed tasks in topological rather than alphabetical order -
> that won't necessarily always show an implicit dependency that went
> missing due to a deltask, but it would at least reveal some of them.

I think showing half of some information would be much more confusing
that clearly not showing it. We'd just get bug reports that the order
was "wrong" and it can't be fixed in all cases.

> > I think there were technical issues as well, such as recipes which
> > inject other tasks between unpack and patch. We needed to stop those
> > other tasks running too which deltasks does effectively, noexec would
> > not.
> 
> Eh, but the other side of that coin is that there are obviously tasks
> injected between unpack and patch, or some other combination, which must
> _not_ be prevented from running

Some need to be prevented, for example where they do extra unpacking
type operations on files. Those would actively break externalsrc.

>  - cf. at least the two or three workarounds that seem to become unnecessary.

Some would be unnecessary, it would break other use cases.

> So can you give examples of
> such tasks that _should_ implicitly not run when externalsrc disables
> (one way or another) the SRCTREECOVEREDTASKS tasks? That's actually the
> RFC part of this, I hope somebody with far more knowledge of various
> corner cases and odd recipes can demonstrate why deltask is more
> appropriate - and then, as I said, some comments to that effect in
> externalsrc.bbclass would be much appreciated.

I will try my best to find them. I don't have a list of these things I
can just recite from memory. One that did come up in a quick search is:

classes/dos2unix.bbclass:addtask convert_crlf_to_lf after do_unpack before do_patch

I would note that the class doesn't appear used in OE-Core any longer.

I know there were other recipes that did inject processing between
unpack and patch but over time, for example to deal with mistakes when
projects have made releases. We have cleaned up OE-Core a lot and I
couldn't find any left at a quick search. I suspect other layers will
have them though.

Proving OE-Core is "safe" for such a change is only part of the
challenge too, there is the wider ecosystem to consider.

> I think it would be weird for a recipe to inherit externalsrc and at the
> same time define a task ordered before do_patch, then not expect that
> task to run at all. So I'm genuinely curious how such examples would look.

devtool takes recipes and adds externalsrc behind the scenes to them to
enable it to do things. Its expected these scenarios work, its used for
example by the automated upgrade helper. We're not risking breaking all
these tools just because you think the code might be cleaner.

I do agree it would be nice if we had some better way of encapsulating
the "source availability" tasks for these purposes and it would make
the work of things like externalsrc and devtool easier. Swapping
deltask and noexec around isn't going to do that though.

Your approach also concerns me as you are basically asking me to
"prove" your code has issues and it seems aren't interested in testing
even the patch you did send. If we did take the change and things
broke, who would fix that?

I'm struggling to try and keep things building as it is, without
changes like this which introduce a lot of risk :(.

Cheers,

Richard

Re: [RFC PATCH 0/4] fix externalsrc task dependency issues

[Rasmus Villemoes]

On 25/09/2020 13.43, Richard Purdie wrote:
> On Fri, 2020-09-25 at 12:31 +0200, Rasmus Villemoes wrote:
>> Well, perhaps listtasks could be taught to say "[noexec flag set]" in
>> the description - currently it seems to be somewhat random whether a
>> class uses noexec or deltask.
> 
> No. That would just confuse things even more. That would hide do_build
> for example so the default target would be missing?

I did not say "don't display noexec tasks". I said 'say "[noexec flag
set]" in the description' - i.e., have the output include an indication
that that task will not be actually be executed. Perhaps a slightly
longer prose would be helpful, say something like "[disabled for this
recipe due to noexec flag being set]" appended after the current
description. There are already lots of noexec tasks, e.g. all image
recipes have a lot of ordinary tasks disabled via noexec, so that should
help address your concern that

> ... If
> someone can "see" the task in listtasks and trys to run it but nothing
> happens, its very confusing for the user.

>> Oh, and for debugging task ordering issues, it would also be nice if
>> listtasks showed tasks in topological rather than alphabetical order -
>> that won't necessarily always show an implicit dependency that went
>> missing due to a deltask, but it would at least reveal some of them.
> 
> I think showing half of some information would be much more confusing
> that clearly not showing it. We'd just get bug reports that the order
> was "wrong" and it can't be fixed in all cases.

Can we then (perhaps via some listtasksfull or whatnot) get bitbake to
display the tasks along with their dependencies. Or how would I, today,
go about figuring out what (intra-recipe) dependencies
some-recipe:do_configure actually has?

>>> I think there were technical issues as well, such as recipes which
>>> inject other tasks between unpack and patch. We needed to stop those
>>> other tasks running too which deltasks does effectively, noexec would
>>> not.
>>
>> Eh, but the other side of that coin is that there are obviously tasks
>> injected between unpack and patch, or some other combination, which must
>> _not_ be prevented from running
> 
> Some need to be prevented, for example where they do extra unpacking
> type operations on files. Those would actively break externalsrc.
> 
>>  - cf. at least the two or three workarounds that seem to become unnecessary.
> 
> Some would be unnecessary, it would break other use cases.
> 
>> So can you give examples of
>> such tasks that _should_ implicitly not run when externalsrc disables
>> (one way or another) the SRCTREECOVEREDTASKS tasks? That's actually the
>> RFC part of this, I hope somebody with far more knowledge of various
>> corner cases and odd recipes can demonstrate why deltask is more
>> appropriate - and then, as I said, some comments to that effect in
>> externalsrc.bbclass would be much appreciated.
> 
> I will try my best to find them. I don't have a list of these things I
> can just recite from memory. One that did come up in a quick search is:
> 
> classes/dos2unix.bbclass:addtask convert_crlf_to_lf after do_unpack before do_patch
> 
> I would note that the class doesn't appear used in OE-Core any longer.

OK, that counts, even if that class appears unused - I can see how such
things can exist.

> Proving OE-Core is "safe" for such a change is only part of the
> challenge too, there is the wider ecosystem to consider.

Absolutely.

>> I think it would be weird for a recipe to inherit externalsrc and at the
>> same time define a task ordered before do_patch, then not expect that
>> task to run at all. So I'm genuinely curious how such examples would look.
> 
> devtool takes recipes and adds externalsrc behind the scenes to them to
> enable it to do things. Its expected these scenarios work, its used for
> example by the automated upgrade helper. We're not risking breaking all
> these tools just because you think the code might be cleaner.

See, that's the kind of explanation I'm looking for - and again, I'd
like to see in externalsrc.bbclass itself. The thing is, I never knew
about the existence of the externalsrc class until I noticed the bug
reported on c5dfc2586b. So when I tried to figure out how to fix that
bug, I had to understand why externalsrc removed the do_patch task,
breaking the configure->patch->symlink_kernsrc chain.

> Your approach also concerns me as you are basically asking me to
> "prove" your code has issues and it seems aren't interested in testing
> even the patch you did send.

That is not how I intended this to be interpreted. But I was really
curious why deltask was chosen over noexec, when even the very same
class then knew it had to do a fixup - it could have been that the class
simply predated the existence of noexec.

> I'm struggling to try and keep things building as it is, without
> changes like this which introduce a lot of risk :(.

You've convinced me that changing deltask to noexec is too
wide-reaching, which means that I think just patch 1/4 should be
applied. Please consider doing that.

Rasmus

Re: [RFC PATCH 0/4] fix externalsrc task dependency issues

[Chanho Park]

Hi,

On Fri, Sep 25, 2020 at 6:06 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 22/09/2020 17.15, Rasmus Villemoes wrote:
> > This is all entirely untested, hence RFC. But if this doesn't work for
> > some reason, I think externalsrc.bbclass should grow some comments as
> > to why deltask is used instead of the noexec flag.
> >
> > Patch 4 is a revert of patch 1, but I did it that way to facilitate
> > backporting, in case patch 2 is too dangerous for backporting. If that
> > is considered safe enough, it should be enough to apply patch 2 to
> > avoid the churn. In any case, patch 3 is just a little simplification
> > made possible by patch 2.
> >
> >
>
> Ping. Chanho, can you tell if this would fix the externalsrc problem?
> Richard, any comments on the deltask vs noexec usage?
>
> Rasmus

It works for me.

Tested-by: Chanho Park <chanho61.park@samsung.com>

-- 
Best Regards,
Chanho Park

Re: [OE-core] [RFC PATCH 0/4] fix externalsrc task dependency issues

[Rasmus Villemoes]

On 25/09/2020 15.14, Rasmus Villemoes via lists.openembedded.org wrote:

> You've convinced me that changing deltask to noexec is too
> wide-reaching, which means that I think just patch 1/4 should be
> applied. Please consider doing that.

Or rather, please apply the v2 I just sent to update the commit log to
reflect my new understanding.

Thanks,
Rasmus