Merge lp:~ev/apport/cache_extracted_debs into lp:~apport-hackers/apport/trunk

[Evan]

As discussed on IRC, here's a branch to cache already extracted debs. I tried filtering these out at the Cache() level, looking at package.candidate.uri. Unfortunately, this doesn't exactly map to the resulting filename, as it doesn't take epochs into account.

So the solution I've come up with is to mark the time before we did a fetch, and only process debs that have been downloaded since by checking their ctime.

I've made this the non-default -k, --keep-unpackaged option. If we cannot squeeze the upload in before final freeze, or you aren't quite happy with this living in trunk, I can always run it from a branch as you suggested.

Thanks!

[Evan (ev)]

And the timings against my previous merge:

ubuntu@server-8149:~$ ./judge/judge './previous_changes' './with_changes'
v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^

    n mean sd min max cmd
       50 22647.1ms 1468.2 20855.4 26954.3 ./previous_changes
          50 8572.3ms 562.3 7958.8 11131.5 ./with_changes
          -14074.836ms -62.1% p=0.000
          difference is significant at 95.0% confidence (p=0.000):
          based on these samples, suggested sample size is n>=1 to have a 3518.71ms confidence interval
          ubuntu@server-8149:~$

[Martin Pitt (pitti)]

Thanks for working on this!

I would like to clean this up a little though, so that it fits better into the existing code:

9 + verbose=False, cache_dir=None, keep_unpacked=False):

How about adding a sandbox_dir=None instead? If you specify it, it uses an existing one and just unpacks the debs that it actually downloaded in this run. If it is None, it uses a temporary directory. This works exactly like the cache_dir argument. Please document the new argument in the docstring, and update the function/docstring in apport/packaging.py (the abstract base class).

Filtering by time should work OK in most circumstances, but I wonder if apt.Cache() does not have a more elegant way to ask "which debs did you actually download"?

42 + optparser.add_option('-k', '--keep-unpacked', metavar='DIR',

Likewise, I think a --sandbox-dir argument would be a bit clearer, and behave similar to --cache. --keep-unpacked sounds too much like a boolean option, specifying a directory looks strange. Please document this in man/apport-retrace.1.

This should also help to simplify the patch a bit, you won't need a separate "keep" variable, etc.

I'm quite happy to have that functionality in trunk with those changes. I originally thought you wanted to make this much more complex.

At some point we need to teach crash-digger to purge its caches and sandboxes regularly. In particular Contents.gz ages rather quickly, and the permanent sandboxes will grow pretty much indefinitely (and pile up a lot of cruft which might even get in the way). But that's a separate problem indeed.

review: Needs Fixing

[lp:~ev/apport/cache_extracted_debs]

2279. By Evan on 2012-04-10

Move to a --sandbox-dir option, as suggested by Martin. Document.

[Evan (ev)]

On Tue, Apr 10, 2012 at 6:14 PM, Martin Pitt <email address hidden> wrote:
> How about adding a sandbox_dir=None instead? If you specify it, it uses an existing one and just unpacks the debs that it actually downloaded in this run. If it is None, it uses a temporary directory. This works exactly like the cache_dir argument. Please document the new argument in the docstring, and update the function/docstring in apport/packaging.py (the abstract base class).

So you would specify this and rootdir to be the same thing?

I agree that the name is better.

[Martin Pitt (pitti)]

> So you would specify this and rootdir to be the same thing?

Argh, sorry for that; I should have looked more closely when I reviewed that. Indeed we already have rootdir, so no need for a new sandbox_dir argument in install_packages(). So for install_packages() we indeed either need an explicit boolean option which will only unpack packages that it downloaded ("permanent_rootdir=False" perhaps?), or it gets implied by checking if the rootdir is nonempty.

I would like to keep the --sandbox-dir option for apport-retrace though, IMHO that looks much nicer.

54 + try:
55 + os.makedirs(sandbox_dir)
56 + except OSError:
57 + pass

Please let's rather use "if not os.path.isdir()... os.makedirs()", to catch errors resulting from EPERM, etc. I know the existing code in install_packages() uses this pattern as well once, but other places in Apport get this right. Let's fix it while we are at it.

77 + archives = os.path.join(aptroot, 'var/cache/apt/archives')

Please let's get this from Dir::Cache::archives instead of hardcoding the path. But I wonder why we need this at all?

80 + cached = os.path.join(archives, os.path.basename(i.destfile))

Instead of line 77 and this, could you not just call os.path.getctime(i.destfile)? It should be the very same.

95 + help=_('Cache directory for unpacked packages. Future runs will assume that any already downloaded package is also extracted to this sandbox.'))

I'd drop the "Cache" here, same in the manpage.

Finally this could do with a test case, something like this:

 - First call with a package or two, starting from an empty sandbox
 - Check that packages are installed
 - Second call with an extra package
 - Check that both the old and the new packages are still installed
 - Make it impossible to download new packages (damaging apt/sources.list perhaps?)
 - Ensure that a third call with an already installed package succeeds
 - Ensure that a fourth call with a new package fails with a FetchFailedException

Thanks!

review: Needs Fixing

[lp:~ev/apport/cache_extracted_debs]

2280. By Evan on 2012-04-11

Use a boolean option to specify that we have a permanent root directory in install packages, rather than passing a redundant directory as the final argument.

2281. By Evan on 2012-04-11

Accidentally inverted test.

[Evan (ev)]

On Wed, Apr 11, 2012 at 8:38 AM, Martin Pitt <email address hidden> wrote:
> Argh, sorry for that; I should have looked more closely when I reviewed that. Indeed we already have rootdir, so no need for a new sandbox_dir argument in install_packages(). So for install_packages() we indeed either need an explicit boolean option which will only unpack packages that it downloaded ("permanent_rootdir=False" perhaps?), or it gets implied by checking if the rootdir is nonempty.

Done as permanent_rootdir.

> I would like to keep the --sandbox-dir option for apport-retrace though, IMHO that looks much nicer.

Agreed.

> 54      + try:
> 55      + os.makedirs(sandbox_dir)
> 56      + except OSError:
> 57      + pass
>
> Please let's rather use "if not os.path.isdir()... os.makedirs()", to catch errors resulting from EPERM, etc. I know the existing code in install_packages() uses this pattern as well once, but other places in Apport get this right. Let's fix it while we are at it.

Indeed, at the time I was just trying to keep consistent and hadn't
considered -EPERM. Fixed.

> 77      + archives = os.path.join(aptroot, 'var/cache/apt/archives')
>
> Please let's get this from Dir::Cache::archives instead of hardcoding the path. But I wonder why we need this at all?
>
> 80      + cached = os.path.join(archives, os.path.basename(i.destfile))
>
> Instead of line 77 and this, could you not just call os.path.getctime(i.destfile)? It should be the very same.

Indeed. I think this was the result of copying code around as the
functionality was changing. Fixed.

> 95      + help=_('Cache directory for unpacked packages. Future runs will assume that any already downloaded package is also extracted to this sandbox.'))
>
> I'd drop the "Cache" here, same in the manpage.

Fixed.

> Finally this could do with a test case, something like this:
>
>  - First call with a package or two, starting from an empty sandbox
>  - Check that packages are installed
>  - Second call with an extra package
>  - Check that both the old and the new packages are still installed
>  - Make it impossible to download new packages (damaging apt/sources.list perhaps?)
>  - Ensure that a third call with an already installed package succeeds
>  - Ensure that a fourth call with a new package fails with a FetchFailedException

Done with Acquire::http::Proxy.

[lp:~ev/apport/cache_extracted_debs]

2282. By Evan on 2012-04-11

Test for permanent sandbox (--sandbox-dir).

[Martin Pitt (pitti)]

Perfect, thanks! Merged with slight whitespace fix (I keep an empty line after single-line docstrings).

review: Approve

=== modified file 'apport/packaging.py'
--- apport/packaging.py	2012-03-01 10:06:25 +0000
+++ apport/packaging.py	2012-04-11 11:30:31 +0000
@@ -172,7 +172,7 @@
         raise NotImplementedError('this method must be implemented by a concrete subclass')
 
     def install_packages(self, rootdir, configdir, release, packages,
-            verbose=False, cache_dir=None):
+            verbose=False, cache_dir=None, permanent_rootdir=False):
         '''Install packages into a sandbox (for apport-retrace).
 
         In order to work without any special permissions and without touching
@@ -194,6 +194,9 @@
         If cache_dir is given, then the downloaded packages will be stored
         there, to speed up subsequent retraces.
 
+        If permanent_rootdir is True, then the sandbox created from the
+        downloaded packages will be reused, to speed up subsequent retraces.
+
         Return a string with outdated packages, or None if all packages were
         installed.
 

=== modified file 'backends/packaging-apt-dpkg.py'
--- backends/packaging-apt-dpkg.py	2012-04-10 12:39:32 +0000
+++ backends/packaging-apt-dpkg.py	2012-04-11 11:30:31 +0000
@@ -463,7 +463,7 @@
         return (installed, outdated)
 
     def install_packages(self, rootdir, configdir, release, packages,
-            verbose=False, cache_dir=None):
+            verbose=False, cache_dir=None, permanent_rootdir=False):
         '''Install packages into a sandbox (for apport-retrace).
 
         In order to work without any special permissions and without touching
@@ -485,6 +485,9 @@
         If cache_dir is given, then the downloaded packages will be stored
         there, to speed up subsequent retraces.
 
+        If permanent_rootdir is True, then the sandbox created from the
+        downloaded packages will be reused, to speed up subsequent retraces.
+
         Return a string with outdated packages, or None if all packages were
         installed.
 
@@ -506,10 +509,8 @@
                 aptroot = os.path.join(cache_dir, release, 'apt')
             else:
                 aptroot = os.path.join(cache_dir, 'system', 'apt')
-            try:
+            if not os.path.isdir(aptroot):
                 os.makedirs(aptroot)
-            except OSError:
-                pass
         else:
             tmp_aptroot = True
             aptroot = tempfile.mkdtemp()
@@ -561,6 +562,7 @@
         for p in real_pkgs:
             c[p].mark_install(False, False)
 
+        last_written = time.time()
         # fetch packages
         fetcher = apt.apt_pkg.Acquire(fetchProgress)
         try:
@@ -573,7 +575,8 @@
         if verbose:
             print('Extracting downloaded debs...')
         for i in fetcher.items:
-            subprocess.check_call(['dpkg', '-x', i.destfile, rootdir])
+            if not permanent_rootdir or os.path.getctime(i.destfile) > last_written:
+                subprocess.check_call(['dpkg', '-x', i.destfile, rootdir])
             real_pkgs.remove(os.path.basename(i.destfile).split('_', 1)[0])
 
         if tmp_aptroot:

=== modified file 'bin/apport-retrace'
--- bin/apport-retrace	2012-02-29 11:13:45 +0000
+++ bin/apport-retrace	2012-04-11 11:30:31 +0000
@@ -59,6 +59,8 @@
         help=_('Prepend timestamps to log messages, for batch operation'))
     optparser.add_option('-C', '--cache', metavar='DIR',
         help=_('Cache directory for packages downloaded in the sandbox'))
+    optparser.add_option('--sandbox-dir', metavar='DIR',
+        help=_('Directory for unpacked packages. Future runs will assume that any already downloaded package is also extracted to this sandbox.'))
     optparser.add_option('-p', '--extra-package', action='append', default=[],
         help=_('Install an extra package into the sandbox (can be specified multiple times)'))
     optparser.add_option('--auth',
@@ -336,8 +338,15 @@
 sandbox = None
 outdated_msg = None
 if options.sandbox:
-    sandbox = tempfile.mkdtemp()
-    atexit.register(shutil.rmtree, sandbox)
+    if options.sandbox_dir:
+        sandbox = os.path.abspath(options.sandbox_dir)
+        if not os.path.isdir(sandbox):
+            os.makedirs(sandbox)
+        permanent_rootdir = True
+    else:
+        sandbox = tempfile.mkdtemp()
+        atexit.register(shutil.rmtree, sandbox)
+        permanent_rootdir = False
 
     pkgs = needed_packages(report)
     for p in options.extra_package:
@@ -355,7 +364,8 @@
 
     try:
         outdated_msg = apport.packaging.install_packages(sandbox, sandbox_arg,
-            report['DistroRelease'], pkgs, options.verbose, options.cache)
+            report['DistroRelease'], pkgs, options.verbose, options.cache,
+            permanent_rootdir)
     except SystemError as e:
         sys.stderr.write(str(e) + '\n')
         sys.exit(1)

=== modified file 'man/apport-retrace.1'
--- man/apport-retrace.1	2011-08-25 05:18:52 +0000
+++ man/apport-retrace.1	2012-04-11 11:30:31 +0000
@@ -123,6 +123,14 @@
 recommended.
 
 .TP
+.B \-\-sandbox\-dir=\fIDIR
+Permanent directory for the sandbox of extracted packages. If not specified all
+cached packages will have to be re-extracted at each run of
+.B apport\-retrace\fR.
+If you use sandbox mode regularly, using a permanent cache directory is highly
+recommended.
+
+.TP
 .B \-h, \-\-help
 Print a short help that documents all options.
 

=== modified file 'test/test_backend_apt_dpkg.py'
--- test/test_backend_apt_dpkg.py	2012-04-03 11:52:41 +0000
+++ test/test_backend_apt_dpkg.py	2012-04-11 11:30:31 +0000
@@ -1,4 +1,6 @@
 import unittest, gzip, imp, subprocess, tempfile, shutil, os, os.path, time
+import apt_pkg
+import glob
 
 if os.environ.get('APPORT_TEST_LOCAL'):
     impl = imp.load_source('', 'backends/packaging-apt-dpkg.py').impl
@@ -497,6 +499,47 @@
             self.assertTrue('nosuchdistro' in str(e), str(e))
             self.assertTrue('index files failed to download' in str(e))
 
+    @unittest.skipUnless(_has_default_route(), 'online test')
+    def test_install_packages_permanent_sandbox(self):
+        '''install_packages() with a permanent sandbox'''
+        self._setup_foonux_config()
+        zonetab = os.path.join(self.rootdir, 'usr/share/zoneinfo/zone.tab')
+
+        impl.install_packages(self.rootdir, self.configdir, 'Foonux 1.2',
+            [('tzdata', None)], False, self.cachedir, permanent_rootdir=True)
+
+        # This will now be using a Cache with our rootdir.
+        archives = apt_pkg.Config.FindDir('Dir::Cache::archives')
+        tzdata = glob.glob(os.path.join(archives, 'tzdata*.deb'))
+        if not tzdata:
+            self.fail('tzdata was not downloaded')
+        tzdata_written = os.path.getctime(tzdata[0])
+        zonetab_written = os.path.getctime(zonetab)
+
+        impl.install_packages(self.rootdir, self.configdir, 'Foonux 1.2',
+            [('coreutils', None), ('tzdata', None)], False, self.cachedir,
+            permanent_rootdir=True)
+
+        if not glob.glob(os.path.join(archives, 'coreutils*.deb')):
+            self.fail('coreutils was not downloaded.')
+        self.assertEqual(os.path.getctime(tzdata[0]), tzdata_written,
+            'tzdata downloaded twice.')
+        self.assertEqual(zonetab_written, os.path.getctime(zonetab),
+            'zonetab written twice.')
+        self.assertTrue(os.path.exists(os.path.join(self.rootdir,
+            'usr/bin/stat')))
+
+        # Prevent packages from downloading.
+        apt_pkg.Config.set('Acquire::http::Proxy', 'http://nonexistent')
+        self.assertRaises(SystemExit, impl.install_packages, self.rootdir,
+            self.configdir, 'Foonux 1.2', [('libc6', None)], False,
+            self.cachedir, permanent_rootdir=True)
+        # These packages exist, so attempting to install them should not fail.
+        impl.install_packages(self.rootdir, self.configdir, 'Foonux 1.2',
+            [('coreutils', None), ('tzdata', None)], False, self.cachedir,
+            permanent_rootdir=True)
+        apt_pkg.Config.set('Acquire::http::Proxy', '')
+
     def _setup_foonux_config(self):
         '''Set up directories and configuration for install_packages()'''