Merge lp:~mterry/ubuntu-archive-tools/or-deps into lp:ubuntu-archive-tools

Proposed by Michael Terry on 2012-07-17
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
Reviewer Review Type Date Requested Status
Ubuntu Package Archive Administrators 2012-07-17 Pending
Review via email: mp+115445@code.launchpad.net

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-node-stringprep, rabbitmq-erlang-client, and rabbitmq-stomp.

To post a comment you must log in.
Michael Terry (mterry) wrote :

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.

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_packages(stanza[field]):
[...]
> + for (depname, depwhy) in parse_relation_packages(stanza[field]):

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_v.remove(rdep)
> + 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:
            checked_v.remove(rdep)
        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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'checkrdepends'
2--- checkrdepends 2012-07-18 14:26:37 +0000
3+++ checkrdepends 2012-07-18 15:38:21 +0000
4@@ -40,10 +40,13 @@
5 # Cheaper version of deb822.PkgRelation.parse_relations.
6 def parse_relation_packages(raw):
7 for or_dep in raw.split(','):
8+ dep_list = []
9 for dep in or_dep.split('|'):
10 match = re_dep.match(dep.strip())
11 if match:
12- yield match.group(1)
13+ dep_list.append(match.group(1))
14+ for dep in dep_list:
15+ yield dep, dep_list
16
17
18 def primary_arches(suite):
19@@ -106,9 +109,9 @@
20 for field in ('build-depends', 'build-depends-indep'):
21 if field not in stanza:
22 continue
23- for depname in parse_relation_packages(stanza[field]):
24+ for depname, depwhy in parse_relation_packages(stanza[field]):
25 build_deps.setdefault(depname, set())
26- build_deps[depname].add(name)
27+ build_deps[depname].add((name, '|'.join(depwhy)))
28 return ret
29
30
31@@ -121,7 +124,7 @@
32 for field in ('pre-depends', 'depends', 'recommends'):
33 if field not in stanza:
34 continue
35- for depname in parse_relation_packages(stanza[field]):
36+ for depname, depwhy in parse_relation_packages(stanza[field]):
37 if depname not in debs:
38 continue
39 # skip dependencies that are built from the same source,
40@@ -129,7 +132,7 @@
41 if name in ignores:
42 continue
43 deps.setdefault(depname, set())
44- deps[depname].add(name)
45+ deps[depname].add((name, '|'.join(depwhy)))
46 except IOError:
47 if not missing_ok:
48 raise
49@@ -202,8 +205,8 @@
50 if deb in build_deps:
51 print("-- %s%s/%s build deps on %s:" %
52 (opts.suite, pocket, comp, deb), file=out)
53- for pkg in sorted(build_deps[deb]):
54- print(pkg, file=out)
55+ for pkg, why in sorted(build_deps[deb]):
56+ print(pkg, why, file=out)
57
58 # binary dependencies
59 for arch in arches:
60@@ -235,14 +238,14 @@
61 if deb in deps:
62 print("-- %s%s/%s %s deps on %s:" %
63 (opts.suite, pocket, comp, arch, deb), file=out)
64- for pkg in sorted(deps[deb]):
65- print(pkg, file=out)
66+ for pkg, why in sorted(deps[deb]):
67+ print(pkg, why, file=out)
68 if deb in di_deps:
69 print("-- %s%s/%s %s deps on %s:" %
70 (opts.suite, pocket, di_comp, arch, deb),
71 file=out)
72- for pkg in sorted(di_deps[deb]):
73- print(pkg, file=out)
74+ for pkg, why in sorted(di_deps[deb]):
75+ print(pkg, why, file=out)
76
77 if opts.directory is not None:
78 out.close()
79
80=== modified file 'nbs-report'
81--- nbs-report 2012-07-18 14:26:37 +0000
82+++ nbs-report 2012-07-18 15:38:21 +0000
83@@ -45,8 +45,8 @@
84 assert cur_component
85 assert cur_arch
86
87- rdep = line.strip()
88- pkgmap.setdefault(rdep, (cur_component, []))[1].append(cur_arch)
89+ rdep, why = line.strip().split(None, 1)
90+ pkgmap.setdefault(rdep, (cur_component, why, []))[2].append(cur_arch)
91
92
93 def _pkg_removable(pkg, nbs, checked_v):
94@@ -66,7 +66,7 @@
95 except KeyError:
96 pass
97 return False
98- if not _pkg_removable(rdep, nbs, checked_v):
99+ elif not _pkg_removable(rdep, nbs, checked_v):
100 try:
101 checked_v.remove(rdep)
102 except KeyError:
103@@ -147,10 +147,10 @@
104 cls = 'removable'
105 else:
106 cls = 'normal'
107- print('<tr><th colspan="4"><span class="%s">%s</span></td></tr>' %
108+ print('<tr><th colspan="5"><span class="%s">%s</span></td></tr>' %
109 (cls, pkg), end="")
110 for rdep in sorted(nbsmap):
111- (component, arches) = nbsmap[rdep]
112+ (component, why, arches) = nbsmap[rdep]
113
114 if component in ('main', 'restricted'):
115 component_cls = 'sup'
116@@ -167,11 +167,16 @@
117 reverse_nbs.setdefault(rdep, []).append(pkg)
118 pkg_component[rdep] = (component, component_cls)
119
120+ whyhtml = ''
121+ if why != pkg:
122+ whyhtml = ' | '.join(why.split('|'))
123+
124 print('<tr><td>&nbsp; &nbsp; </td>', end='')
125 print('<td><span class="%s">%s</span></td> ' % (cls, rdep), end='')
126 print('<td><span class="component%s">%s</span></td>' %
127 (component_cls, component), end='')
128- print('<td>%s</td></tr>' % ' '.join(arches))
129+ print('<td>%s</td>' % ' '.join(arches), end='')
130+ print('<td>%s</td></tr>' % whyhtml)
131
132 print('''</table>
133 <h2>Packages which depend on NBS packages</h2>
134@@ -201,7 +206,7 @@
135 # main
136 #
137
138-nbs = {} # pkg -> rdep_pkg -> (component, [arch1, arch2, ...])
139+nbs = {} # pkg -> rdep_pkg -> (component, [why], [arch1, arch2, ...])
140
141 for f in os.listdir(sys.argv[1]):
142 if f.startswith('.') or f.endswith('.html'):

Subscribers

People subscribed via source and target branches