Bug 1218

Summary: [HOB] package selection logic inconsistent after select/deselect several times
Product: [Build System, Metadata & Runtime] Hob Reporter: Jiajun Xu <jiajun.xu>
Component: hobAssignee: Joshua Lock - Disabled <josh>
Status: VERIFIED FIXED QA Contact:
Severity: major    
Priority: High CC: jessica.zhang, lianhao.lu, liping.ke, poky.bs.watcher, poky.watcher
Version: unspecified   
Target Milestone: 1.1 M2   
Hardware: x86   
OS: Multiple   
Whiteboard: Patch submitted for review
OS type for building Yocto: --- Type of Regression: ---
Verified: Documentation change: ---

Description Jiajun Xu 2011-07-06 23:59:33 UTC
Tree/Branch: Poky/Master
Commit: d4132fa12885fc050313a5c9aa6903e4fa92c94f

With latest master, for HOB, package selection logic is not consistent after select/deselect for several times. If we select package acl, there are 3 packages introduced - acl, attr and ncurses. If we deselect acl, ncurses will not be removed. But if we re-try the same steps, select acl -> deselect acl, ncurses will be removed.
Comment 1 Jiajun Xu 2011-07-07 00:46:36 UTC
I think the bug is major becasue it shows different result to user and makes confusion.
Comment 2 liping Ke 2011-07-07 01:40:40 UTC
We spent some digging into this problem:
Take acl as an example.
acl (brought in by user selected) needs attr (brought in by acl), attr needs
ncurses (brought in by attr).

If we remove acl, ncurses will be left, and brought in by column is cleared.
When we reselect acl, ncurses brought in column still not correct.

Obviously there're two problems.
1. How to handle orphan? It's not clean and when having large amounts of
orphans, it's confusing.
2. If one package is used by many packages, one value in brought in can't
describe the dependency tree relationship correctly. It's also the root cause
of orphan problem.

Maybe the accurate way is that we need a multi-value column (depend-packages
list) to describe how many packages depend on this one, and a ref-count. When
ref-count decrease to zero, this package could be drop... Something like
that... There're many different data structures we can use... I guess. 
When removing a package, dec all it's dependencies. If zero, remove too. It's
the cleanest way?

This is only my feelings... Only for ref..

Regards,
criping
Comment 3 Joshua Lock - Disabled 2011-07-07 10:25:49 UTC
Agree this is high priority. It's top of my task list.
Comment 4 Joshua Lock - Disabled 2011-07-08 11:31:51 UTC
OK, so I did a little digging myself. The inconsistency you're seeing is because the brought in by column is not correctly updated for orphan packages. I have a patch that will fix this but I'm still not certain what's the right thing to do in the orphan case - should orphans be left in the image?

I'm beginning to think not, but I've certainly felt yes at some stage...
Comment 5 Jessica 2011-07-08 11:59:53 UTC
So with the patch, will you correctly show the brought in list which reflects the reference count for a certain package, instead just one depency shows up as in the current impl?

As to orphan packages, I think we can left them in the list and provide a separate button called remove orphan packages to allow user explicitly remove them, when we do bake and the image will contain orphan packages, we'll prompt the user there're orphan packages included in the current image, if it's not desired, please clean orphan pakcage first then bake...
Comment 6 Joshua Lock - Disabled 2011-07-08 12:22:40 UTC
(In reply to comment #5)
> So with the patch, will you correctly show the brought in list which reflects
> the reference count for a certain package, instead just one depency shows up as
> in the current impl?

Yes, if I follow the examples above of selecting attr, deselecting attr then selecting attr again it works as expected. ncurses has it's brought-in-by correctly updated and the package selections are consistent.
 
> As to orphan packages, I think we can left them in the list and provide a
> separate button called remove orphan packages to allow user explicitly remove
> them, when we do bake and the image will contain orphan packages, we'll prompt
> the user there're orphan packages included in the current image, if it's not
> desired, please clean orphan pakcage first then bake...

The code comments and implementation imply that I had intended orphans to be cleaned automatically, I think I want/need to fix this.
Comment 7 Jessica 2011-07-08 12:30:21 UTC
The code comments and implementation imply that I had intended orphans to be
cleaned automatically, I think I want/need to fix this.

[JZ] had a conversation with Dave earlier and I think for the initial release of HOB, it's OK that we leave certain knobs to the end user to turn manually instead of automate the handling behind the scene.  1) this way the function flow is cleaner 2) we don't know much about user preference yet, even though as engineers we like to automate things to the maximum, but probably user prefer to have better control of things, step by step?
Comment 8 Joshua Lock - Disabled 2011-07-08 12:37:19 UTC
(In reply to comment #7)
> The code comments and implementation imply that I had intended orphans to be
> cleaned automatically, I think I want/need to fix this.
> 
> [JZ] had a conversation with Dave earlier and I think for the initial release
> of HOB, it's OK that we leave certain knobs to the end user to turn manually
> instead of automate the handling behind the scene.  1) this way the function
> flow is cleaner 2) we don't know much about user preference yet, even though as
> engineers we like to automate things to the maximum, but probably user prefer
> to have better control of things, step by step?

I'm not convinced either way on the 2nd point, we're not designing software for typical users. I expect most of the users will have some engineer about them.

however for the 1st point is kind of moot because we whether it's automatically triggered or whether we have a button to do it we'll need to fix the method which cleans up orphan packages. The function flow is not so much more complex by triggering that method call automatically.
Comment 9 Jessica 2011-07-08 12:59:26 UTC
I'm not convinced either way on the 2nd point, we're not designing software for
typical users. I expect most of the users will have some engineer about them.

however for the 1st point is kind of moot because we whether it's automatically
triggered or whether we have a button to do it we'll need to fix the method
which cleans up orphan packages. The function flow is not so much more complex
by triggering that method call automatically.

[JZ] It's hard to predict user preference, this is typical issue for an UI appl.  But anyway, as you mentioned once we have the remoev orphan work correctly, it just a matter of minor change to trigger it manually or automatically which we can go either way...
Comment 10 liping Ke 2011-07-10 18:23:16 UTC
Hi, Josh

I just want to confirm one thing:
You said that you will update brought in column. Take acl->attr->ncurses as an example, if we select acl, then clear acl. Brought in by column for attr will be "None". After we re-select acl, this column will be updated to "attr", right?

I am thinking about another scenario.
if A->common_dep, B->common_dep, C->common_dep. We assume that the "brought in" column for common_dep is randomly filled in with "A". If we clear "A", the brought in by column for common_dep is what then "None", "B" or "C"? 

I just can't think out a clear solution for this...


Thanks a lot for your help!
criping
Comment 11 liping Ke 2011-07-10 18:24:51 UTC
> be "None". After we re-select acl, this column will be updated to "attr",
> right?
> 
A typo here, the column "brought in" of "attr" will be updated to "acl" again.

Thanks:)
criping
Comment 12 Joshua Lock - Disabled 2011-07-11 10:08:18 UTC
(In reply to comment #10)
> Hi, Josh
> 
> I just want to confirm one thing:
> You said that you will update brought in column. Take acl->attr->ncurses as an
> example, if we select acl, then clear acl. Brought in by column for attr will
> be "None". After we re-select acl, this column will be updated to "attr",
> right?

Correct. Though now I've fixed the automatic orphan clean up (in my dev branch) attr will be automatically removed  when its binb column is empty.
 
> I am thinking about another scenario.
> if A->common_dep, B->common_dep, C->common_dep. We assume that the "brought in"
> column for common_dep is randomly filled in with "A". If we clear "A", the
> brought in by column for common_dep is what then "None", "B" or "C"? 

common_dep will be updated to contain one of B or C, whichever is first in the model. The code iterates the gtk.ListStore to find a potential parent and selects the first it finds for binb.

tasklistmodel.py:355 find_alt_dependency() - although the comment seems to be a little bit rotten :-/
Comment 13 Joshua Lock - Disabled 2011-07-13 09:03:56 UTC
Patch merged into BitBake master and Poky:

"ui/crumbs/tasklistmodel: fix automatic removal of orphaned items

The sweep_up() method intends to remove all packages with an empty brought in by column, this patch changes the implementation to be more reliable. Each time a removal is triggered we begin interating the contents model again at the beginning, only once the contents model has been iterated from start to finish without any removals can we be certain that there will be no more orphaned items."
Comment 15 Jiajun Xu 2011-08-01 22:10:23 UTC
Verify with commit 46cf540e63a848512617b20fd8492f81bfb2f704, the bug is fixed.