Bug 7355

Created attachment 2418 [details]
covered-by.png

Created attachment 2419 [details]
toaster.sqlite

Created attachment 2420 [details]
covered by (a to z)

I've also attached a csv file with the covered by information shown in the screenshot, but sorted alphabetically. That might make it easier to spot some kind of pattern.

Alex has implemented some constraints when tracing the covering tasks: he is now stopping at the first not covered task(s) in the tree. 

This solves the Toaster performance issue, but arguably the information is incomplete, because when traversing the tree we are skipping any other covered tasks that might have contributed to produce the covered task output. 

To give you an example of what you get. For the same recipe (quilt-native) in the same build:

fetch, unpack, patch and populate_license now show as covered by quilt-native do_build

configure, compile, install and populate_sysroot now show as covered by a whole bunch of do_patch tasks from other recipes (which as per my conversations with you somehow matches your expectations).

I have no idea if this is better or worse, though.

I have to admit I'm completely confused what this is trying to show. I'd have thought what the user expects to see is that a task was either:

a) Previously Built (stamp already existed from a previous build)
b) From SState (not executed since it was available and used from sstate)
c) Not Needed (since sstate was available, we didn't nee this task)
d) Executed

I'm hoping we can drop the confusing idea of "covered" and try to map to something more like the above states instead?

The tricky part about "From SState" is that it may have been installed from sstate in a previous build, or it may have been installed from SState in this build. The Executed/Not Executed field may make that clearer I guess.

>... The tricky part about "From SState" is that it may have been installed from sstate in a previous build ...

"Covered" tasks are exactly these - setscene-skipped tasks because the task was deployed in a previous build.

(In reply to comment #7)
> >... The tricky part about "From SState" is that it may have been installed from sstate in a previous build ...
> 
> "Covered" tasks are exactly these - setscene-skipped tasks because the task
> was deployed in a previous build.

I think the UI needs to reflect whether this was a task with a setscene stamp, i.e. a directly installed sstate task or whether its something "Not needed" since all tasks which depend on it came from sstate. Does that make sense?

(In reply to comment #5)
> I have to admit I'm completely confused what this is trying to show. I'd
> have thought what the user expects to see is that a task was either:
> 
> a) Previously Built (stamp already existed from a previous build)
> b) From SState (not executed since it was available and used from sstate)
> c) Not Needed (since sstate was available, we didn't nee this task)
> d) Executed
> 
> I'm hoping we can drop the confusing idea of "covered" and try to map to
> something more like the above states instead?

This is very close to what we currently have: 

a) Previously Built (stamp already existed from a previous build) > We call those prebuilt tasks

b) installed from SState in this build (not executed since it was available and used from sstate) > We call those cached tasks

d) Executed > we have those too, and we show them as Succeeded when they complete, or Failed when they don't complete

c) Not Needed (since sstate was available, we didn't need this task) > This are our "Covered" tasks, like Alex pointed out

We also identify tasks that have the noexec flag set to 1 (we called them 'Empty')

(In reply to comment #8)
> (In reply to comment #7)
> > >... The tricky part about "From SState" is that it may have been installed from sstate in a previous build ...
> > 
> > "Covered" tasks are exactly these - setscene-skipped tasks because the task
> > was deployed in a previous build.
> 
> I think the UI needs to reflect whether this was a task with a setscene
> stamp, i.e. a directly installed sstate task or whether its something "Not
> needed" since all tasks which depend on it came from sstate. Does that make
> sense?

I think we are already doing this, since we split the two into different task types. 

If I understand this comment correctly, what is important is the difference between those 2 types of tasks (the ones that have a setscene stamp vs. the ones that are "not needed"). If that's the case, it looks like the "covered by" information that prompted this bug is not really that important, so I am going to suggest we drop it from Toaster. 

Would you both be ok with this?

Thanks

Belén

(In reply to comment #9)
> (In reply to comment #5)
> > I have to admit I'm completely confused what this is trying to show. I'd
> > have thought what the user expects to see is that a task was either:
> > 
> > a) Previously Built (stamp already existed from a previous build)
> > b) From SState (not executed since it was available and used from sstate)
> > c) Not Needed (since sstate was available, we didn't nee this task)
> > d) Executed
> > 
> > I'm hoping we can drop the confusing idea of "covered" and try to map to
> > something more like the above states instead?
> 
> This is very close to what we currently have: 
> 
> a) Previously Built (stamp already existed from a previous build) > We call
> those prebuilt tasks

This doesn't work as there is huge confusion between "executed on a previous run" and "came from sstate". I understand which "prebuilt" means but most users will not and we do need to show the difference as it can mean something significant to certain users.

> b) installed from SState in this build (not executed since it was available
> and used from sstate) > We call those cached tasks
> 
> d) Executed > we have those too, and we show them as Succeeded when they
> complete, or Failed when they don't complete
> 
> c) Not Needed (since sstate was available, we didn't need this task) > This
> are our "Covered" tasks, like Alex pointed out

The terminology doesn't work then, I simply cannot associated "covered" with "not needed" :(.

> We also identify tasks that have the noexec flag set to 1 (we called them
> 'Empty')

This one is ok.

Looking at the sample build I've run there is something going wrong as the data does not look as I'd expect it to knowing what happens underneath so there is definitely a problem that needs fixing here (and its not just terminology)

> > a) Previously Built (stamp already existed from a previous build) > We call
> > those prebuilt tasks
> 
> This doesn't work as there is huge confusion between "executed on a previous
> run" and "came from sstate". I understand which "prebuilt" means but most
> users will not and we do need to show the difference as it can mean
> something significant to certain users.

I would need to understand what exactly doesn't work. It might be useful to discuss this with Toaster in front of us. 

> 
> > b) installed from SState in this build (not executed since it was available
> > and used from sstate) > We call those cached tasks
> > 
> > d) Executed > we have those too, and we show them as Succeeded when they
> > complete, or Failed when they don't complete
> > 
> > c) Not Needed (since sstate was available, we didn't need this task) > This
> > are our "Covered" tasks, like Alex pointed out
> 
> The terminology doesn't work then, I simply cannot associated "covered" with
> "not needed" :(.

We did go back and forth several times about this name. We started from prebuilt, but decided to use that word for the tasks with a stamp; we moved onto 'skipped', which you and Paul didn't like because that word is already in use in a different context. That's when Paul suggested 'covered', and we settled for that in the end. 

We can definitely change it. 

> 
> > We also identify tasks that have the noexec flag set to 1 (we called them
> > 'Empty')
> 
> This one is ok.
> 
> Looking at the sample build I've run there is something going wrong as the
> data does not look as I'd expect it to knowing what happens underneath so
> there is definitely a problem that needs fixing here (and its not just
> terminology)

This is more worrying, since it suggests we are identifying tasks incorrectly. 
This I guess requires you and Alex to look at the code.

I talked with Alex a little, just to summarise, the "covered" data comes from runqueue.py with the task_skip("existing" and task_skip("covered") calls.

"covered" includes setscene tasks which already existed from a previous build.

--- a/bitbake/lib/bb/runqueue.py
+++ b/bitbake/lib/bb/runqueue.py
@@ -1587,7 +1587,10 @@ class RunQueueExecuteTasks(RunQueueExecute):
             if task in self.rq.scenequeue_covered:
                 logger.debug(2, "Setscene covered task %s (%s)", task,
                                 self.rqdata.get_user_idstring(task))
-                self.task_skip(task, "covered")
+                if self.rq.check_stamp_task(task, taskname + "_setscene", cache=self.stampcache):
+                    self.task_skip(task, "covered setscene")
+                else:
+                    self.task_skip(task, "covered")
                 return True
 
and making "covered setscene" map to "existing" would probably remove at least one point of confusion I have with the current display. I'm going to stare at this a bit longer and check we really are showing the correct things from here though as I suspect there may also be some other corner cases not being logged 100% the way we need.

I think there are severa issues here and together they're confusing me. The easiest way for me to illustrate is with a patch:

diff --git a/bitbake/lib/bb/runqueue.py b/bitbake/lib/bb/runqueue.py
index fa90d74..2f8fe64 100644
--- a/bitbake/lib/bb/runqueue.py
+++ b/bitbake/lib/bb/runqueue.py
@@ -1587,7 +1587,10 @@ class RunQueueExecuteTasks(RunQueueExecute):
             if task in self.rq.scenequeue_covered:
                 logger.debug(2, "Setscene covered task %s (%s)", task,
                                 self.rqdata.get_user_idstring(task))
-                self.task_skip(task, "covered")
+                if self.rq.check_stamp_task(task, taskname + "_setscene", cache=self.stampcache):
+                    self.task_skip(task, "existing")
+                else:
+                    self.task_skip(task, "covered")
                 return True
 
             if self.rq.check_stamp_task(task, taskname, cache=self.stampcache):

The above change ensures that if a task in a previous build comes from sstate, it will be listed as "Existing" rather than being "Covered". This is why the covered list looks so odd.

An alternative would be to log an extra "existing sstate" state and then mark this up a previously built task which came from the sstate cache. This would actually be my preferred outcome but complicates the UI and the code changes.

diff --git a/bitbake/lib/toaster/orm/models.py b/bitbake/lib/toaster/orm/models.py
index 4d1dcbc..7c52e41 100644
--- a/bitbake/lib/toaster/orm/models.py
+++ b/bitbake/lib/toaster/orm/models.py
@@ -379,16 +379,16 @@ class Task(models.Model):
     TASK_OUTCOME = (
         (OUTCOME_NA, 'Not Available'),
         (OUTCOME_SUCCESS, 'Succeeded'),
-        (OUTCOME_COVERED, 'Covered'),
+        (OUTCOME_COVERED, 'Not Needed'),
         (OUTCOME_CACHED, 'Cached'),
-        (OUTCOME_PREBUILT, 'Prebuilt'),
+        (OUTCOME_PREBUILT, 'Previous Build'),
         (OUTCOME_FAILED, 'Failed'),
         (OUTCOME_EMPTY, 'Empty'),
     )
 
     TASK_OUTCOME_HELP = (
         (OUTCOME_SUCCESS, 'This task successfully completed'),
-        (OUTCOME_COVERED, 'This task did not run because its output is provided by another task'),
+        (OUTCOME_COVERED, 'This task did not run because everything depending on it came from the cache'),
         (OUTCOME_CACHED, 'This task restored output from the sstate-cache directory or mirrors'),
         (OUTCOME_PREBUILT, 'This task did not run because its outcome was reused from a previous build'),
         (OUTCOME_FAILED, 'This task did not complete'),

This tweaks the naming and description of a couple of states to avoid confusion. I propose "Prebuilt" should be "Previous Build" as the user might thing "Prebuilt" means come from the cache sstate. I know I did.

It also changes "Covered" to "Not needed" since I also feel this is clearer about what it actually means to the end user. I also changed the description to try and be clearer about what it means.

Belen/Alex/Paul: Can you have a look at these changes and see if they make sense to you?

Just realised that for "Previous build" tasks Toaster provides the ability to search for tasks in previous builds with the same signature. Will this functionality work for all of them, or only for tasks with a "normal" stamp?

(In reply to comment #15)
> Just realised that for "Previous build" tasks Toaster provides the ability
> to search for tasks in previous builds with the same signature. Will this
> functionality work for all of them, or only for tasks with a "normal" stamp?

I think this should work for all since the signature should match regardless of what type of stamp it was.

The question is what the next steps here are. Should I send some or all of this out as patches? Is this something we want to look at in 1.8M4? Or 1.9 material? Should I hand this to Alex to handle from here?

I've taken ownership for this bug, with target for 1.9 - I would refrain from making significant changes this late in the game for 1.8.