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

[Michael Terry]

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.

[Michael Terry (mterry)]

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)]

OK, fixed. Setting back to ready-for-review.

[Colin Watson (cjwatson)]

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)]

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)]

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.

[lp:~mterry/ubuntu-archive-tools/or-deps]

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)]

Alright, try this. Just adds the "why lists" to the HTML for rdeps with non-trivial ones.

[Michael Terry (mterry)]

poke

[Steve Langasek (vorlon)]

resurrected as https://code.launchpad.net/~brian-murray/ubuntu-archive-tools/or-deps/+merge/405498

review: Disapprove

Unmerged revisions

=== modified file 'checkrdepends'
--- checkrdepends	2012-07-18 14:26:37 +0000
+++ checkrdepends	2012-07-18 15:38:21 +0000
@@ -40,10 +40,13 @@
 # Cheaper version of deb822.PkgRelation.parse_relations.
 def parse_relation_packages(raw):
     for or_dep in raw.split(','):
+        dep_list = []
         for dep in or_dep.split('|'):
             match = re_dep.match(dep.strip())
             if match:
-                yield match.group(1)
+                dep_list.append(match.group(1))
+        for dep in dep_list:
+            yield dep, dep_list
 
 
 def primary_arches(suite):
@@ -106,9 +109,9 @@
         for field in ('build-depends', 'build-depends-indep'):
             if field not in stanza:
                 continue
-            for depname in parse_relation_packages(stanza[field]):
+            for depname, depwhy in parse_relation_packages(stanza[field]):
                 build_deps.setdefault(depname, set())
-                build_deps[depname].add(name)
+                build_deps[depname].add((name, '|'.join(depwhy)))
     return ret
 
 
@@ -121,7 +124,7 @@
             for field in ('pre-depends', 'depends', 'recommends'):
                 if field not in stanza:
                     continue
-                for depname in parse_relation_packages(stanza[field]):
+                for depname, depwhy in parse_relation_packages(stanza[field]):
                     if depname not in debs:
                         continue
                     # skip dependencies that are built from the same source,
@@ -129,7 +132,7 @@
                     if name in ignores:
                         continue
                     deps.setdefault(depname, set())
-                    deps[depname].add(name)
+                    deps[depname].add((name, '|'.join(depwhy)))
     except IOError:
         if not missing_ok:
             raise
@@ -202,8 +205,8 @@
                 if deb in build_deps:
                     print("-- %s%s/%s build deps on %s:" %
                           (opts.suite, pocket, comp, deb), file=out)
-                    for pkg in sorted(build_deps[deb]):
-                        print(pkg, file=out)
+                    for pkg, why in sorted(build_deps[deb]):
+                        print(pkg, why, file=out)
 
                 # binary dependencies
                 for arch in arches:
@@ -235,14 +238,14 @@
                     if deb in deps:
                         print("-- %s%s/%s %s deps on %s:" %
                               (opts.suite, pocket, comp, arch, deb), file=out)
-                        for pkg in sorted(deps[deb]):
-                            print(pkg, file=out)
+                        for pkg, why in sorted(deps[deb]):
+                            print(pkg, why, file=out)
                     if deb in di_deps:
                         print("-- %s%s/%s %s deps on %s:" %
                               (opts.suite, pocket, di_comp, arch, deb),
                               file=out)
-                        for pkg in sorted(di_deps[deb]):
-                            print(pkg, file=out)
+                        for pkg, why in sorted(di_deps[deb]):
+                            print(pkg, why, file=out)
 
                 if opts.directory is not None:
                     out.close()

=== 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 @@
             assert cur_component
             assert cur_arch
 
-            rdep = line.strip()
-            pkgmap.setdefault(rdep, (cur_component, []))[1].append(cur_arch)
+            rdep, why = line.strip().split(None, 1)
+            pkgmap.setdefault(rdep, (cur_component, why, []))[2].append(cur_arch)
 
 
 def _pkg_removable(pkg, nbs, checked_v):
@@ -66,7 +66,7 @@
             except KeyError:
                 pass
             return False
-        if not _pkg_removable(rdep, nbs, checked_v):
+        elif not _pkg_removable(rdep, nbs, checked_v):
             try:
                 checked_v.remove(rdep)
             except KeyError:
@@ -147,10 +147,10 @@
             cls = 'removable'
         else:
             cls = 'normal'
-        print('<tr><th colspan="4"><span class="%s">%s</span></td></tr>' %
+        print('<tr><th colspan="5"><span class="%s">%s</span></td></tr>' %
               (cls, pkg), end="")
         for rdep in sorted(nbsmap):
-            (component, arches) = nbsmap[rdep]
+            (component, why, arches) = nbsmap[rdep]
 
             if component in ('main', 'restricted'):
                 component_cls = 'sup'
@@ -167,11 +167,16 @@
                 reverse_nbs.setdefault(rdep, []).append(pkg)
                 pkg_component[rdep] = (component, component_cls)
 
+            whyhtml = ''
+            if why != pkg:
+                whyhtml = ' | '.join(why.split('|'))
+
             print('<tr><td>&nbsp; &nbsp; </td>', end='')
             print('<td><span class="%s">%s</span></td> ' % (cls, rdep), end='')
             print('<td><span class="component%s">%s</span></td>' %
                   (component_cls, component), end='')
-            print('<td>%s</td></tr>' % ' '.join(arches))
+            print('<td>%s</td>' % ' '.join(arches), end='')
+            print('<td>%s</td></tr>' % whyhtml)
 
     print('''</table>
 <h2>Packages which depend on NBS packages</h2>
@@ -201,7 +206,7 @@
 # main
 #
 
-nbs = {}  # pkg -> rdep_pkg -> (component, [arch1, arch2, ...])
+nbs = {}  # pkg -> rdep_pkg -> (component, [why], [arch1, arch2, ...])
 
 for f in os.listdir(sys.argv[1]):
     if f.startswith('.') or f.endswith('.html'):