Merge lp:~mterry/ubuntu-archive-tools/or-deps into lp:ubuntu-archive-tools
Status: | Needs review |
---|---|
Proposed branch: | lp:~mterry/ubuntu-archive-tools/or-deps |
Merge into: | lp:ubuntu-archive-tools |
Diff against target: |
142 lines (+26/-18) 2 files modified
checkrdepends (+14/-11) nbs-report (+12/-7) |
To merge this branch: | bzr merge lp:~mterry/ubuntu-archive-tools/or-deps |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ubuntu Package Archive Administrators | 2012-07-17 | Pending | |
Review via email:
|
Description of the change
I noticed that some of the packages in the NBS report are false-negatives (in the sense that they are reported as not-removable when it would actually be safe to remove them).
In particular, let's say package A depends on "B | C" while B is NBS and C is not. Package A will show up as blocking B from being removed, when B can actually be removed safely.
There are two files I changed:
checkrdepend now reports why a package is include in the rdepend list. The format of the line used to be:
RDEPEND
But now is:
RDEPEND WHY1|WHY2|WHY3
The only consumer that I could see of that output is nbs-report, which was changed to understand the changes. And to examine the "why list" for packages that are not NBS and allow the package to be removed if all of its rdepends are such.
I also added the "why list" to the NBS report HTML, since such imformation will help understand why it is recommending a package for removal when it still has rdepends. It only is shown if the "why list" is not a trivial one-package list.
Obviously, there is a danger here that I will turn false-negatives into false-positives, which are far more disasterous. So please look over with a fine-toothed comb, obviously.
As of this writing, my changes detect several more packages for removal than before, all of which manually are confirmed to be accurate. Specifically: compiz-kde, fb-music-low, libglew1.6-dev, libglewmx1.6, libglewmx1.6-dev, libnode-
Michael Terry (mterry) wrote : | # |
Michael Terry (mterry) wrote : | # |
OK, fixed. Setting back to ready-for-review.
Colin Watson (cjwatson) wrote : | # |
To be honest, I'm a bit sceptical about doing this at all. In many
cases, even if a dependency technically can be satisfied using one of
the other elements in a disjunction, we really should be updating the
outdated elements anyway; consider the recent case of dhcp3-client,
where it was entirely correct to update at least some of those elements
to say isc-dhcp-client instead. I'm concerned that, if these stop
showing up in nbs-report, we'll stop bothering; and there's really
nowhere else we automatically notice this kind of thing.
How about, instead of removing these packages from the NBS report
altogether, we just put a note against them? Something like "but
alternatives are present: C, D" would provide us an immediate scannable
visual clue that a disjunction is involved, without losing information
or risking false positives.
A few minor code nits:
> + yield (dep, dep_list)
[...]
> + for (depname, depwhy) in parse_relation_
[...]
> + for (depname, depwhy) in parse_relation_
These days I tend to prefer to drop the unnecessary parentheses in these
cases.
> + # Check why this rdep is here. If it is part of an or-group, maybe
> + # one of the other rdeps in that group are available, and this rdep
> + # should not be blocking removal of pkg.
> + ignore = False
> + why = nbs[pkg][rdep][1]
> + for whydep in why.split('|'):
> + if whydep not in nbs:
> + ignore = True
> + break
> + if not ignore:
> + try:
> + checked_
> + except KeyError:
> + pass
> + return False
This may be obsolete given my comments above, but FWIW, this could be
more concisely written using Python's handy for/else construction:
why = nbs[pkg][rdep][1]
for whydep in why.split('|'):
if whydep not in nbs:
break
else:
try:
except KeyError:
pass
return False
Michael Terry (mterry) wrote : | # |
Fair enough, regarding concerns about false-positives. One small correction though. This branch doesn't "removing these packages from the NBS report altogether", it just marks them green if appropriate and adds them to the suggested remove-package line.
The branch also shows the "why list" next to any rdepend that has a complicated one.
It sounds like you would be more comfortable with this branch if it only did that latter bit, instead of marking anything green or suggesting it for the remove-package line?
Colin Watson (cjwatson) wrote : | # |
On Tue, Jul 17, 2012 at 10:41:20PM -0000, Michael Terry wrote:
> One small correction though. This branch doesn't "removing these
> packages from the NBS report altogether", it just marks them green if
> appropriate and adds them to the suggested remove-package line.
Sorry, yeah, that's what I meant.
> The branch also shows the "why list" next to any rdepend that has a
> complicated one.
>
> It sounds like you would be more comfortable with this branch if it
> only did that latter bit, instead of marking anything green or
> suggesting it for the remove-package line?
Indeed.
- 502. By Michael Terry on 2012-07-18
-
some style nits from the review
- 503. By Michael Terry on 2012-07-18
-
Don't recommend any why-safe packages for removal, for safety's sake. Instead, just show the why list in the HTML, and color code it
- 504. By Michael Terry on 2012-07-18
-
on second thought, color was too noisy
- 505. By Michael Terry on 2012-07-18
-
a few more style nits
- 506. By Michael Terry on 2012-07-18
-
merge from trunk
Michael Terry (mterry) wrote : | # |
Alright, try this. Just adds the "why lists" to the HTML for rdeps with non-trivial ones.
Michael Terry (mterry) wrote : | # |
poke
Unmerged revisions
- 506. By Michael Terry on 2012-07-18
-
merge from trunk
- 505. By Michael Terry on 2012-07-18
-
a few more style nits
- 504. By Michael Terry on 2012-07-18
-
on second thought, color was too noisy
- 503. By Michael Terry on 2012-07-18
-
Don't recommend any why-safe packages for removal, for safety's sake. Instead, just show the why list in the HTML, and color code it
- 502. By Michael Terry on 2012-07-18
-
some style nits from the review
- 501. By Michael Terry on 2012-07-17
-
don't check rdeps that aren't nbs for removability
- 500. By Michael Terry on 2012-07-17
-
show why a rdep is included and don't treat rdeps that have alternatives as necessary
Hmm, changing to Work in Progress for a bit. The HTML output is correct, but the remove-package line has some packages it shouldn't. Looking into why.