Code review comment for lp:~brian-murray/apport/retrace-package-versions

8 -

Bikeshedding, I know, but I like the empty line here better as the following code is in a different flow branch/context.

22 + # first, grab the versions that we captured at crash time

As there is no "second", perhaps just "# create a package -> version map from report"?

34 + return [(p, pkg_vers.get(p, 'None')) for p in pkgs]

This needs to be None, not 'None'. get() defaults to None as fallback, so just pkg_vers.get(p) will do.

43 + if pkg in report['Package']:

This could fail in weird corner cases if it matches a substring or even the version number. Let's use this instead:

  if report['Package'].startswith(pkg + ' ')

45 + if version:
46 + pkgs.append((pkg, version))
47 + else:
48 + pkgs.append((pkg, None))

This needs to be indented by a block, as version is otherwise not defined.

The other changes look good to me. This still needs a test case. This can be tested by adjusting test_install_packages_versioned() in test/test_backend_apt_dpkg.py, which currently uses Ubuntu precise. There are plenty of different versions in -updates to choose from :-), i. e. selecting a version from precise final should download that version instead of current precise-updates.

> However, this is incomplete in that the lookup for returning a package to which a file belongs (get_file_package used for items in ProcMaps) is unaware of package versions.

If you are speaking of plugins or other things with undeclared Depends:, and thus we don't have the version of the package that we reverse-mapped from ProcMaps, then we lost; we should just continue to install the most current version of the package. If you mean something else, I'm afraid I don't understand. As long as we have a package version in Package: or Dependencies:, that's the one we should install, and we have the report object available in all places which are concerned about package installation.

Thanks for working on this!