Merge lp:~brian-murray/apport/retrace-package-versions into lp:~apport-hackers/apport/trunk

[Brian Murray]

These changes to apport and its retracing process will, in some cases, attempt to install the specific package version that the client was using when they experienced the crash.

It does this by examining the report's Package and Dependencies keys to determine the version of the package used. 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. Given that _search_contents is searching each pocket individually we would know the pocket from which the package came, so perhaps we could do another search using those two things. Do you have any ideas?

I've tested these changes using some crashes and corresponding coredumps I received from the error tracker. I added some information to apport-retrace to determine how many of the packages remained without a version:

/mnt/sec-machines/20140204-cores/03273f1e-1f10-11e3-b2fe-2c768aafd08c.crash
57 packages have a version, 38 do not.
/mnt/sec-machines/20140204-cores/10165494-3e32-11e3-9577-2c768aafd08c.crash
9 packages have a version, 2 do not.
/mnt/sec-machines/20140204-cores/2e2ee904-3d70-11e3-af86-2c768aafd08c.crash
60 packages have a version, 4 do not.
/mnt/sec-machines/20140204-cores/4dd5303e-2096-11e3-921f-2c768aafd08c.crash
85 packages have a version, 20 do not.
/mnt/sec-machines/20140204-cores/586432ac-8bb7-11e3-baa3-e4115b0f8a4a.crash
115 packages have a version, 17 do not.
/mnt/sec-machines/20140204-cores/6f6484e6-145e-11e3-a19d-e4115b0f8a4a.crash
47 packages have a version, 11 do not.
/mnt/sec-machines/20140204-cores/89c60494-3ecb-11e3-afb2-e4115b0f8a4a.crash
30 packages have a version, 0 do not.
/mnt/sec-machines/20140204-cores/8e546922-8d60-11e3-a75b-e4115b0f8a4a.crash
68 packages have a version, 41 do not.
/mnt/sec-machines/20140204-cores/a4f86840-0f43-11e3-9f48-e4115b0f8a4a.crash
59 packages have a version, 48 do not.
/mnt/sec-machines/20140204-cores/b925ddf4-3d9f-11e3-a1bf-2c768aafd08c.crash
102 packages have a version, 4 do not.
/mnt/sec-machines/20140204-cores/ec88c4be-3d21-11e3-a287-2c768aafd08c.crash
1 packages have a version, 55 do not.

So it seems like there are still some improvements to be made. I've also renamed a variable from c to cache and as c is short for continue in ipdb and this made it hard to debug cache.

[Martin Pitt (pitti)]

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!

review: Needs Fixing

[lp:~brian-murray/apport/retrace-package-versions]

2761. By Brian Murray on 2014-03-26

make changes based on pitti's feedback

[Brian Murray (brian-murray)]

On Fri, Mar 14, 2014 at 09:23:01AM -0000, Martin Pitt wrote:
> Review: Needs Fixing
>
> 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.

I've made the above changes.

> 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.

I'll work on the test case now.

> > 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.

This is what I am referring to but instead of just installing the most
current version of the package I'm suggesting that we record which
pocket '_search_contents' found the file in and then query Launchpad (or
something) to determine what is the current version of that package in
that pocket. Then we would be installing a more correct package version
and could look at enabling the -proposed repository in the retracer
configurations.

--
Brian Murray

[lp:~brian-murray/apport/retrace-package-versions]

2762. By Brian Murray on 2014-03-26

update test_backend_apt_dpkg.py to install the version of coreutils from -updates and the other packages from the release pocket

[Martin Pitt (pitti)]

I committed the c → cache renaming in http://bazaar.launchpad.net/~apport-hackers/apport/trunk/revision/2790 as it is quite a lot of noise and unrelated to the actual change here.

I'm not happy about the test case, as it now assumes fixed version numbers in precise-updates, which may change in the future. I think it's better (as suggested above) to pin the version down to the precise-final one and confirm that those get installed instead of the latest precise-updates versions. I'll work on that.

Thanks!

[Martin Pitt (pitti)]

Also, in make_sandbox()'es second hunk it only checks report['Package'] but not 'Dependencies'? Should mostly be harmless though. But the inner if is missing an else path, otherwise we won't append the package at all.

Looks good to me otherwise, thanks!

[Martin Pitt (pitti)]

For the record, this also needs adjustment of install_packages()'s grabbing of the -dbg/-dbgsym package versions. I adjusted the test case to check the installed package versions, and with that it's now installing non-matching -dbg versions.

[Martin Pitt (pitti)]

And more generally, I'm afraid this patch is currently a no-op. While it now picks the right "candidate", this isn't being used for fetching packages, as

       for p in real_pkgs:
            cache[p].mark_install(False, False)

is always using the latest version. So this needs to be reworked completely.

review: Needs Fixing

[Martin Pitt (pitti)]

I merged the backends/packaging-apt-dpkg.py part of this and reworked the whole install_packages() to actually select the specified version in apt rather than just complaining about mismatches. I also changed the test case to actually verify the downloaded/installed package versions and be robust against further precise-updates versions: http://bazaar.launchpad.net/~apport-hackers/apport/trunk/revision/2791

This does not yet include the sandboxutils changes, as they go on top of this and aren't covered by tests.

[Martin Pitt (pitti)]

And finally, the cleaned up sandboxutils.py improvements: http://bazaar.launchpad.net/~apport-hackers/apport/trunk/revision/2792

Thanks!

[Martin Pitt (pitti)]

For the record, I rolled this out to the Launchpad retracers now.

[Brian Murray (brian-murray)]

Thanks for finishing this! I've had daisy updated now too. While we've now addressed versioning for the Package and Dependencies keys there may be still some improvements to make regarding ProcMaps. Previously we talked about this:

> 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.

This is what I am referring to but instead of just installing the most
current version of the package I'm suggesting that we record which
pocket '_search_contents' found the file in and then query Launchpad (or
something) to determine what is the current version of that package in
that pocket. Then we would be installing a more correct package version
than just using the latest version.

[Martin Pitt (pitti)]

Hey Brian,

Brian Murray [2014-04-23 17:22 -0000]:
> This is what I am referring to but instead of just installing the most
> current version of the package I'm suggesting that we record which
> pocket '_search_contents' found the file in and then query Launchpad (or
> something) to determine what is the current version of that package in
> that pocket.

How would that help? The file would be present in all pockets (unless
we are dealing with things like a library transition in an SRU, which
should be fairly uncommon -- but then the _search_contents query will
give us even better results), so we already know the current version
during apport-retrace time.

The only thing which would actually help with this is to do the file
-> package -> version mapping on the client side at report time. I. e.
at report time we need to walk all files in ProcMaps and add their
packages to Dependencies: if they aren't there already.
That'd be fairly expensive though.

=== modified file 'apport/sandboxutils.py'
--- apport/sandboxutils.py	2013-09-30 06:07:14 +0000
+++ apport/sandboxutils.py	2014-03-26 19:33:35 +0000
@@ -83,9 +83,22 @@
                 apport.log('dynamically loaded %s needs package %s, queueing' % (l, pkg))
             pkgs.add(pkg)
         else:
-                apport.warning('%s is needed, but cannot be mapped to a package', l)
-
-    return [(p, None) for p in pkgs]
+            apport.warning('%s is needed, but cannot be mapped to a package', l)
+
+    pkg_vers = {}
+    # create a package -> version map from the report
+    for l in (report.get('Package', '') + '\n' + report.get('Dependencies', '')).splitlines():
+        if not l.strip():
+            continue
+        try:
+            (pkg, version) = l.split()[:2]
+        except ValueError:
+            apport.warning('invalid Package/Dependencies line: %s', l)
+            # invalid line, ignore
+            continue
+        pkg_vers[pkg] = version
+
+    return [(p, pkg_vers.get(p)) for p in pkgs]
 
 
 def make_sandbox(report, config_dir, cache_dir=None, sandbox_dir=None,
@@ -184,7 +197,12 @@
                                                     arch=report.get('Architecture'))
             if pkg:
                 apport.log('Installing extra package %s to get %s' % (pkg, path), log_timestamps)
-                pkgs.append((pkg, None))
+                if report['Package'].startswith(pkg + ' '):
+                    version = report['Package'].split()[1]
+                    if version:
+                        pkgs.append((pkg, version))
+                else:
+                    pkgs.append((pkg, None))
             else:
                 apport.warning('Cannot find package which ships %s', path)
 

=== modified file 'backends/packaging-apt-dpkg.py'
--- backends/packaging-apt-dpkg.py	2014-01-22 08:24:55 +0000
+++ backends/packaging-apt-dpkg.py	2014-03-26 19:33:35 +0000
@@ -362,9 +362,11 @@
             match = self.__fgrep_files(file, all_lists)
 
         if match:
+            # no version here
             return os.path.splitext(os.path.basename(match))[0].split(':')[0]
 
         if uninstalled:
+            # no version here
             return self._search_contents(file, map_cachedir, release, arch)
         else:
             return None
@@ -605,15 +607,15 @@
         else:
             fetchProgress = apt.progress.base.AcquireProgress()
         if not tmp_aptroot:
-            c = self._sandbox_cache(aptroot, apt_sources, fetchProgress)
+            cache = self._sandbox_cache(aptroot, apt_sources, fetchProgress)
         else:
             self._build_apt_sandbox(aptroot, apt_sources)
-            c = apt.Cache(rootdir=os.path.abspath(aptroot))
+            cache = apt.Cache(rootdir=os.path.abspath(aptroot))
             try:
-                c.update(fetchProgress)
+                cache.update(fetchProgress)
             except apt.cache.FetchFailedException as e:
                 raise SystemError(str(e))
-            c.open()
+            cache.open()
 
         obsolete = ''
 
@@ -623,7 +625,12 @@
         real_pkgs = set()
         for (pkg, ver) in packages:
             try:
-                candidate = c[pkg].candidate
+                versions = cache[pkg].versions
+                if ver not in versions:
+                    # try the most recent if we don't have the right version
+                    candidate = cache[pkg].candidate
+                else:
+                    candidate = cache[pkg].versions[ver]
             except KeyError:
                 candidate = None
             if not candidate:
@@ -657,7 +664,7 @@
                     # Replaces/Depends, we can safely choose the first value
                     # here.
                     conflict = conflict[0]
-                    if c.is_virtual_package(conflict[0]):
+                    if cache.is_virtual_package(conflict[0]):
                         try:
                             providers = virtual_mapping[conflict[0]]
                         except KeyError:
@@ -680,32 +687,32 @@
                                 os.unlink(path)
 
             if candidate.architecture != 'all':
-                if pkg + '-dbg' in c:
+                if pkg + '-dbg' in cache:
                     real_pkgs.add(pkg + '-dbg')
                 else:
                     # install all -dbg from the source package
                     if src_records.lookup(candidate.source_name):
-                        dbgs = [p for p in src_records.binaries if p.endswith('-dbg') and p in c]
+                        dbgs = [p for p in src_records.binaries if p.endswith('-dbg') and p in cache]
                     else:
                         dbgs = []
                     if dbgs:
                         for p in dbgs:
                             real_pkgs.add(p)
                     else:
-                        if pkg + '-dbgsym' in c:
+                        if pkg + '-dbgsym' in cache:
                             real_pkgs.add(pkg + '-dbgsym')
-                            if c[pkg + '-dbgsym'].candidate.version != candidate.version:
+                            if cache[pkg + '-dbgsym'].candidate.version != candidate.version:
                                 obsolete += 'outdated debug symbol package for %s: package version %s dbgsym version %s\n' % (
-                                    pkg, candidate.version, c[pkg + '-dbgsym'].candidate.version)
+                                    pkg, candidate.version, cache[pkg + '-dbgsym'].candidate.version)
 
         for p in real_pkgs:
-            c[p].mark_install(False, False)
+            cache[p].mark_install(False, False)
 
         last_written = time.time()
         # fetch packages
         fetcher = apt.apt_pkg.Acquire(fetchProgress)
         try:
-            c.fetch_archives(fetcher=fetcher)
+            cache.fetch_archives(fetcher=fetcher)
         except apt.cache.FetchFailedException as e:
             apport.error('Package download error, try again later: %s', str(e))
             sys.exit(99)  # transient error
@@ -879,6 +886,7 @@
             out = zgrep.communicate()[0].decode('UTF-8')
             # we do not check the return code, since zgrep -m1 often errors out
             # with 'stdout: broken pipe'
+            # no version here
             if out:
                 package = out.split()[1].split(',')[0].split('/')[-1]
             if package:

=== modified file 'test/test_backend_apt_dpkg.py'
--- test/test_backend_apt_dpkg.py	2014-01-27 14:20:26 +0000
+++ test/test_backend_apt_dpkg.py	2014-03-26 19:33:35 +0000
@@ -480,7 +480,7 @@
         self._setup_foonux_config()
         obsolete = impl.install_packages(self.rootdir, self.configdir,
                                          'Foonux 1.2',
-                                         [('coreutils', '8.13-3ubuntu3'),
+                                         [('coreutils', '8.13-3ubuntu3.2'),
                                           ('libc6', '2.15-0ubuntu10'),
                                           ('tzdata', '2012b-1'),
                                          ], False, self.cachedir)
@@ -518,7 +518,7 @@
         os.unlink(os.path.join(self.rootdir, 'usr/bin/stat'))
         obsolete = impl.install_packages(self.rootdir, self.configdir,
                                          'Foonux 1.2',
-                                         [('coreutils', '8.13-3ubuntu3'),
+                                         [('coreutils', '8.13-3ubuntu3.2'),
                                          ], False, self.cachedir)
         self.assertEqual(obsolete, '')
         self.assertTrue(os.path.exists(
@@ -553,7 +553,7 @@
         # still installs packages after above operations
         os.unlink(os.path.join(self.rootdir, 'usr/bin/stat'))
         impl.install_packages(self.rootdir, self.configdir, 'Foonux 1.2',
-                              [('coreutils', '8.13-3ubuntu3'),
+                              [('coreutils', '8.13-3ubuntu3.2'),
                                ('dpkg', None),
                               ], False, self.cachedir)
         self.assertTrue(os.path.exists(os.path.join(self.rootdir,
@@ -764,7 +764,7 @@
         self.assertTrue(os.path.isdir(os.path.join(res, 'debian')))
         # this needs to be updated when the release in _setup_foonux_config
         # changes
-        self.assertTrue(res.endswith('/base-files-6.5ubuntu6'),
+        self.assertTrue(res.endswith('/base-files-6.5ubuntu6.7'),
                         'unexpected version: ' + res.split('/')[-1])
 
     def _setup_foonux_config(self):
@@ -781,6 +781,9 @@
             f.write('deb http://archive.ubuntu.com/ubuntu/ precise main\n')
             f.write('deb-src http://archive.ubuntu.com/ubuntu/ precise main\n')
             f.write('deb http://ddebs.ubuntu.com/ precise main\n')
+            f.write('deb http://archive.ubuntu.com/ubuntu/ precise-updates main\n')
+            f.write('deb-src http://archive.ubuntu.com/ubuntu/ precise-updates main\n')
+            f.write('deb http://ddebs.ubuntu.com/ precise-updates main\n')
         os.mkdir(os.path.join(self.configdir, 'Foonux 1.2', 'armhf'))
         with open(os.path.join(self.configdir, 'Foonux 1.2', 'armhf', 'sources.list'), 'w') as f:
             f.write('deb http://ports.ubuntu.com/ precise main\n')