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
=== modified file 'checkrdepends'
--- checkrdepends 2012-07-18 14:26:37 +0000
+++ checkrdepends 2012-07-18 15:38:21 +0000
@@ -40,10 +40,13 @@
40# Cheaper version of deb822.PkgRelation.parse_relations.40# Cheaper version of deb822.PkgRelation.parse_relations.
41def parse_relation_packages(raw):41def parse_relation_packages(raw):
42 for or_dep in raw.split(','):42 for or_dep in raw.split(','):
43 dep_list = []
43 for dep in or_dep.split('|'):44 for dep in or_dep.split('|'):
44 match = re_dep.match(dep.strip())45 match = re_dep.match(dep.strip())
45 if match:46 if match:
46 yield match.group(1)47 dep_list.append(match.group(1))
48 for dep in dep_list:
49 yield dep, dep_list
4750
4851
49def primary_arches(suite):52def primary_arches(suite):
@@ -106,9 +109,9 @@
106 for field in ('build-depends', 'build-depends-indep'):109 for field in ('build-depends', 'build-depends-indep'):
107 if field not in stanza:110 if field not in stanza:
108 continue111 continue
109 for depname in parse_relation_packages(stanza[field]):112 for depname, depwhy in parse_relation_packages(stanza[field]):
110 build_deps.setdefault(depname, set())113 build_deps.setdefault(depname, set())
111 build_deps[depname].add(name)114 build_deps[depname].add((name, '|'.join(depwhy)))
112 return ret115 return ret
113116
114117
@@ -121,7 +124,7 @@
121 for field in ('pre-depends', 'depends', 'recommends'):124 for field in ('pre-depends', 'depends', 'recommends'):
122 if field not in stanza:125 if field not in stanza:
123 continue126 continue
124 for depname in parse_relation_packages(stanza[field]):127 for depname, depwhy in parse_relation_packages(stanza[field]):
125 if depname not in debs:128 if depname not in debs:
126 continue129 continue
127 # skip dependencies that are built from the same source,130 # skip dependencies that are built from the same source,
@@ -129,7 +132,7 @@
129 if name in ignores:132 if name in ignores:
130 continue133 continue
131 deps.setdefault(depname, set())134 deps.setdefault(depname, set())
132 deps[depname].add(name)135 deps[depname].add((name, '|'.join(depwhy)))
133 except IOError:136 except IOError:
134 if not missing_ok:137 if not missing_ok:
135 raise138 raise
@@ -202,8 +205,8 @@
202 if deb in build_deps:205 if deb in build_deps:
203 print("-- %s%s/%s build deps on %s:" %206 print("-- %s%s/%s build deps on %s:" %
204 (opts.suite, pocket, comp, deb), file=out)207 (opts.suite, pocket, comp, deb), file=out)
205 for pkg in sorted(build_deps[deb]):208 for pkg, why in sorted(build_deps[deb]):
206 print(pkg, file=out)209 print(pkg, why, file=out)
207210
208 # binary dependencies211 # binary dependencies
209 for arch in arches:212 for arch in arches:
@@ -235,14 +238,14 @@
235 if deb in deps:238 if deb in deps:
236 print("-- %s%s/%s %s deps on %s:" %239 print("-- %s%s/%s %s deps on %s:" %
237 (opts.suite, pocket, comp, arch, deb), file=out)240 (opts.suite, pocket, comp, arch, deb), file=out)
238 for pkg in sorted(deps[deb]):241 for pkg, why in sorted(deps[deb]):
239 print(pkg, file=out)242 print(pkg, why, file=out)
240 if deb in di_deps:243 if deb in di_deps:
241 print("-- %s%s/%s %s deps on %s:" %244 print("-- %s%s/%s %s deps on %s:" %
242 (opts.suite, pocket, di_comp, arch, deb),245 (opts.suite, pocket, di_comp, arch, deb),
243 file=out)246 file=out)
244 for pkg in sorted(di_deps[deb]):247 for pkg, why in sorted(di_deps[deb]):
245 print(pkg, file=out)248 print(pkg, why, file=out)
246249
247 if opts.directory is not None:250 if opts.directory is not None:
248 out.close()251 out.close()
249252
=== modified file 'nbs-report'
--- nbs-report 2012-07-18 14:26:37 +0000
+++ nbs-report 2012-07-18 15:38:21 +0000
@@ -45,8 +45,8 @@
45 assert cur_component45 assert cur_component
46 assert cur_arch46 assert cur_arch
4747
48 rdep = line.strip()48 rdep, why = line.strip().split(None, 1)
49 pkgmap.setdefault(rdep, (cur_component, []))[1].append(cur_arch)49 pkgmap.setdefault(rdep, (cur_component, why, []))[2].append(cur_arch)
5050
5151
52def _pkg_removable(pkg, nbs, checked_v):52def _pkg_removable(pkg, nbs, checked_v):
@@ -66,7 +66,7 @@
66 except KeyError:66 except KeyError:
67 pass67 pass
68 return False68 return False
69 if not _pkg_removable(rdep, nbs, checked_v):69 elif not _pkg_removable(rdep, nbs, checked_v):
70 try:70 try:
71 checked_v.remove(rdep)71 checked_v.remove(rdep)
72 except KeyError:72 except KeyError:
@@ -147,10 +147,10 @@
147 cls = 'removable'147 cls = 'removable'
148 else:148 else:
149 cls = 'normal'149 cls = 'normal'
150 print('<tr><th colspan="4"><span class="%s">%s</span></td></tr>' %150 print('<tr><th colspan="5"><span class="%s">%s</span></td></tr>' %
151 (cls, pkg), end="")151 (cls, pkg), end="")
152 for rdep in sorted(nbsmap):152 for rdep in sorted(nbsmap):
153 (component, arches) = nbsmap[rdep]153 (component, why, arches) = nbsmap[rdep]
154154
155 if component in ('main', 'restricted'):155 if component in ('main', 'restricted'):
156 component_cls = 'sup'156 component_cls = 'sup'
@@ -167,11 +167,16 @@
167 reverse_nbs.setdefault(rdep, []).append(pkg)167 reverse_nbs.setdefault(rdep, []).append(pkg)
168 pkg_component[rdep] = (component, component_cls)168 pkg_component[rdep] = (component, component_cls)
169169
170 whyhtml = ''
171 if why != pkg:
172 whyhtml = ' | '.join(why.split('|'))
173
170 print('<tr><td>&nbsp; &nbsp; </td>', end='')174 print('<tr><td>&nbsp; &nbsp; </td>', end='')
171 print('<td><span class="%s">%s</span></td> ' % (cls, rdep), end='')175 print('<td><span class="%s">%s</span></td> ' % (cls, rdep), end='')
172 print('<td><span class="component%s">%s</span></td>' %176 print('<td><span class="component%s">%s</span></td>' %
173 (component_cls, component), end='')177 (component_cls, component), end='')
174 print('<td>%s</td></tr>' % ' '.join(arches))178 print('<td>%s</td>' % ' '.join(arches), end='')
179 print('<td>%s</td></tr>' % whyhtml)
175180
176 print('''</table>181 print('''</table>
177<h2>Packages which depend on NBS packages</h2>182<h2>Packages which depend on NBS packages</h2>
@@ -201,7 +206,7 @@
201# main206# main
202#207#
203208
204nbs = {} # pkg -> rdep_pkg -> (component, [arch1, arch2, ...])209nbs = {} # pkg -> rdep_pkg -> (component, [why], [arch1, arch2, ...])
205210
206for f in os.listdir(sys.argv[1]):211for f in os.listdir(sys.argv[1]):
207 if f.startswith('.') or f.endswith('.html'):212 if f.startswith('.') or f.endswith('.html'):

Subscribers

People subscribed via source and target branches