Merge lp:~mvo/python-apt/mvo into lp:~ubuntu-core-dev/python-apt/ubuntu

[Michael Vogt]

The work from Jason Conti to fix leaking FDs

[lp:~mvo/python-apt/mvo]

619. By Michael Vogt on 2012-10-15

apt/cache.py: add comment

[Jason Conti (jconti)]

Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.

I like the other additional tests, and actually had a very similar test to test_cache_close_download_fails() but it didn't occur to me to include it for some reason. It also appears I completely missed the case about leaking fds on cache reopen, thanks for fixing it.

review: Approve

[Michael Vogt (mvo)]

On Mon, Oct 15, 2012 at 05:08:22PM -0000, Jason Conti wrote:
> Review: Approve
>
> Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.

Thanks a lot! Oh, interessting. I had hoped that the gc.collect()
would ensure that its deleted and thereforce closed. Does the test
fails reliable for you? If so, any hints what I can do trying to
reproduce the issue?

> I like the other additional tests, and actually had a very similar test to test_cache_close_download_fails() but it didn't occur to me to include it for some reason. It also appears I completely missed the case about leaking fds on cache reopen, thanks for fixing it.

Great, thanks for the review!

Cheers,
 Michael

[lp:~mvo/python-apt/mvo]

620. By Michael Vogt on 2012-10-17

tests/test_apt_cache.py: delete test_cache_delete_leasks_fds() as its not reliable

[Michael Vogt (mvo)]

On Mon, Oct 15, 2012 at 05:08:22PM -0000, Jason Conti wrote:
> Review: Approve
>
> Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.
[..]

Nevermind, its also failing for me now, not sure why I did not see
that before. I deleted the test again.

Cheers,
 Michael

[lp:~mvo/python-apt/mvo]

621. By Michael Vogt on 2012-10-23

build fixes for python3.3

=== modified file 'apt/auth.py'
--- apt/auth.py	2012-10-12 09:06:34 +0000
+++ apt/auth.py	2012-10-23 10:39:23 +0000
@@ -78,7 +78,8 @@
                                 stderr=subprocess.PIPE)
 
         content = kwargs.get("stdin", None)
-        if isinstance(content, unicode):
+        # py2 needs this encoded, py3.3 will crash if it is
+        if isinstance(content, unicode) and sys.version_info[:2] < (3, 3):
             content = content.encode("utf-8")
 
         output, stderr = proc.communicate(content)
 
=== modified file 'apt/cache.py'
--- apt/cache.py	2012-04-17 19:17:10 +0000
+++ apt/cache.py	2012-10-23 10:39:23 +0000
@@ -42,6 +42,9 @@
 class LockFailedException(IOError):
     """Exception that is thrown when locking fails."""
 
+class CacheClosedException(Exception):
+    """Exception that is thrown when the cache is used after close()."""
+
 
 class Cache(object):
     """Dictionary-like package cache.
@@ -139,6 +142,8 @@
         """
         if progress is None:
             progress = apt.progress.base.OpProgress()
+        # close old cache on (re)open
+        self.close()
         self.op_progress = progress
         self._run_callbacks("cache_pre_open")
 
@@ -172,6 +177,20 @@
         progress.done()
         self._run_callbacks("cache_post_open")
 
+    def close(self):
+        """ Close the package cache """
+        # explicitely free the FDs that _records has open
+        del self._records
+        self._records = None
+
+    def __enter__(self):
+        """ Enter the with statement """
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        """ Exit the with statement """
+        self.close()
+
     def __getitem__(self, key):
         """ look like a dictionary (get key) """
         try:
@@ -238,6 +257,8 @@
     @property
     def required_download(self):
         """Get the size of the packages that are required to download."""
+        if self._records is None:
+            raise CacheClosedException("Cache object used after close() called")
         pm = apt_pkg.PackageManager(self._depcache)
         fetcher = apt_pkg.Acquire()
         pm.get_archives(fetcher, self._list, self._records)
@@ -288,6 +309,8 @@
 
     def _fetch_archives(self, fetcher, pm):
         """ fetch the needed archives """
+        if self._records is None:
+            raise CacheClosedException("Cache object used after close() called")
 
         # get lock
         lockfile = apt_pkg.config.find_dir("Dir::Cache::Archives") + "lock"
 
=== modified file 'debian/changelog'
--- debian/changelog	2012-10-12 09:11:14 +0000
+++ debian/changelog	2012-10-23 10:39:23 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
 python-apt (0.8.8ubuntu1) UNRELEASED; urgency=low
 
   NOTE: THIS is intended for R, use lp:~ubuntu-core-dev/python-apt/quantal
@@ -107,6 +108,81 @@
 
  -- Michael Vogt <[e-mail]>  Fri, 29 Jun 2012 16:05:52 +0200
 
+=======
+python-apt (0.8.8.1) UNRELEASED; urgency=low
+
+  [ Michael Vogt ]
+  * python/tag.cc:
+    - make TagSecString_FromStringAndSize, TagSecString_FromString
+      static, thanks to jcristau
+  * python/cache.cc:
+    - add "Codename" to PackageFile object
+  * add dep8 style autopkgtest support
+  * build fixes for python3.3
+
+  [ Jason Conti ]
+  * lp:~jconti/python-apt/closeable-cache:
+    - add apt.Cache.close() method
+
+ -- Michael Vogt <[e-mail]>  Mon, 15 Oct 2012 10:03:21 +0200
+
+python-apt (0.8.8) unstable; urgency=low
+
+  [ Program translation updates ]
+  * po/pl.po: Polish (Michał Kułach) (closes: #684308)
+  * po/da.po: Danish (Joe Hansen) (closes: #689827)
+  
+  [ Michael Vogt ]
+  * merged lp:~sampo555/python-apt/fix_1042916 reuse existing but
+    disabled sources.list entries instead of duplicating them.
+    Thanks to "sampo555", LP: #1042916
+  * lp:~mvo/python-apt/fix-debfile-crash:
+    - fix crash on missing candidates in the multiarch check
+  * lp:~mvo/python-apt/recv-key-lp1016643:
+    - Only support long (v4) keyids when downloading keys and
+      check the keys fingerprint before importing. This avoids
+      man-in-the-middle attacks (LP: #1016643)
+  * consolidate tests/test_lp1030278.py into the new 
+    tests/test_size_to_str.py
+  * apt/auth.py:
+    - support importing long keyids with leading 0x and mixed case
+  * debian/control:
+    - build-depend on python-unittest2 to get "with TestCase.assertRaises"
+      support in python2.6
+
+  [ Barry Warsaw ]
+  * python/string.cc, tests/test_lp1030278.py: Fix StrSizeToStr() so that
+    1) it first checks for PyLong-ness so that in Python 3 on i386, it
+    will be able to convert larger numbers (via doubles rather than ints);
+    2) before doing the conversions through the apt API, check to see if a
+    Python exception occurred, e.g. OverflowError, and return an error
+    condition in that case instead of masking it.  (LP: #1030278)
+
+  [ James Hunt ]
+  * python/cache.cc: PkgCacheGetIsMultiArch(): Return calculated
+    value rather than a random one.
+  * lp:~jamesodhunt/python-apt/test-for-size_to_str:
+    - add test for size_to_str() to help with finding LP: #1030278
+
+ -- Michael Vogt <[e-mail]>  Fri, 12 Oct 2012 10:47:11 +0200
+
+python-apt (0.8.7) unstable; urgency=low
+
+  [ Translation updates ]
+  * po/es.po: Spanish translation updated by Omar Campagne (closes: #679285)
+  * po/ja.po: Japanese translation updated by Kenshi Muto (closes: #679652)
+
+  [ Jakub Wilk ]
+  * Fix typos: the the -> the (closes: #679432)
+
+  [ Julian Andres Klode ]
+  * apt/auth.py:
+    - Do not merge stdout and stderr (Closes: #678706)
+    - Forward stderr from apt-key to our stderr if non-empty
+
+ -- Julian Andres Klode <[e-mail]>  Mon, 30 Jul 2012 13:29:17 +0200
+
+>>>>>>> MERGE-SOURCE
 python-apt (0.8.6) unstable; urgency=low
 
   [ Michael Vogt ]
 
=== modified file 'debian/control'
--- debian/control	2012-10-12 09:06:34 +0000
+++ debian/control	2012-10-23 10:39:23 +0000
@@ -18,11 +18,19 @@
                python3-all-dbg (>= 3.1.2-6~),
                python-distutils-extra (>= 2.0),
                python-sphinx (>= 0.5),
+<<<<<<< TREE
                python-debian
 Vcs-Bzr: http://code.launchpad.net/~ubuntu-core-dev/python-apt/ubuntu
 XS-Debian-Vcs-Bzr: http://bzr.debian.org/apt/python-apt/debian-sid
 XS-Debian-Vcs-Browser: http://bzr.debian.org/loggerhead/apt/python-apt/debian-sid/changes
 XS-Testsuite: autopkgtest
+=======
+               python-debian,
+	       python-unittest2
+Vcs-Bzr: http://bzr.debian.org/apt/python-apt/debian-sid
+Vcs-Browser: http://bzr.debian.org/loggerhead/apt/python-apt/debian-sid/changes
+XS-Testsuite: autopkgtest
+>>>>>>> MERGE-SOURCE
 
 Package: python-apt
 Architecture: any
 
=== modified file 'python/generic.h'
--- python/generic.h	2011-08-01 07:29:25 +0000
+++ python/generic.h	2012-10-23 10:39:23 +0000
@@ -80,10 +80,14 @@
 
 static inline const char *PyUnicode_AsString(PyObject *op) {
     // Convert to bytes object, using the default encoding.
+#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
+    return PyUnicode_AsUTF8(op);
+#else
     // Use Python-internal API, there is no other way to do this
     // without a memory leak.
     PyObject *bytes = _PyUnicode_AsDefaultEncodedString(op, 0);
     return bytes ? PyBytes_AS_STRING(bytes) : 0;
+#endif
 }
 
 // Convert any type of string based object to a const char.
 
=== modified file 'python/tag.cc'
--- python/tag.cc	2012-06-17 21:11:16 +0000
+++ python/tag.cc	2012-10-23 10:39:23 +0000
@@ -74,7 +74,7 @@
     PyString_FromStringAndSize((v), (len))
 #define TagSecString_FromString(self, v) PyString_FromString(v)
 #else
-PyObject *TagSecString_FromStringAndSize(PyObject *self, const char *v,
+static PyObject *TagSecString_FromStringAndSize(PyObject *self, const char *v,
 	 				 Py_ssize_t len) {
    TagSecData *Self = (TagSecData *)self;
    if (Self->Bytes)
@@ -85,7 +85,7 @@
       return PyUnicode_FromStringAndSize(v, len);
 }
 
-PyObject *TagSecString_FromString(PyObject *self, const char *v) {
+static PyObject *TagSecString_FromString(PyObject *self, const char *v) {
    TagSecData *Self = (TagSecData *)self;
    if (Self->Bytes)
       return PyBytes_FromString(v);
 
=== modified file 'tests/test_apt_cache.py'
--- tests/test_apt_cache.py	2011-12-09 08:16:08 +0000
+++ tests/test_apt_cache.py	2012-10-23 10:39:23 +0000
@@ -7,19 +7,27 @@
 # are permitted in any medium without royalty provided the copyright
 # notice and this notice are preserved.
 """Unit tests for verifying the correctness of check_dep, etc in apt_pkg."""
+
+import glob
+import logging
 import os
+import shutil
+import sys
 import tempfile
 import unittest
 
+if sys.version_info[0] == 2 and sys.version_info[1] == 6:
+    from unittest2 import TestCase
+else:
+    from unittest import TestCase
+
+
 from test_all import get_library_dir
-import sys
 sys.path.insert(0, get_library_dir())
 
 import apt
 import apt_pkg
-import shutil
-import glob
-import logging
+
 
 def if_sources_list_is_readable(f):
     def wrapper(*args, **kwargs):
@@ -29,7 +37,17 @@
             logging.warn("skipping '%s' because sources.list is not readable" % f)
     return wrapper
 
-class TestAptCache(unittest.TestCase):
+
+def get_open_file_descriptors():
+    try:
+        fds = os.listdir("/proc/self/fd")
+    except OSError:
+        logging.warn("failed to list /proc/self/fd")
+        return set([])
+    return set(map(int, fds))
+
+
+class TestAptCache(TestCase):
     """ test the apt cache """
 
     def setUp(self):
@@ -68,7 +86,34 @@
                 self.assertEqual(r['Package'], pkg.shortname)
                 self.assertTrue('Version' in r)
                 self.assertTrue(len(r['Description']) > 0)
-                self.assertTrue(str(r).startswith('Package: %s\n' % pkg.shortname))
+                self.assertTrue(
+                    str(r).startswith('Package: %s\n' % pkg.shortname))
+
+    @if_sources_list_is_readable
+    def test_cache_close_leak_fd(self):
+        fds_before_open = get_open_file_descriptors()
+        cache = apt.Cache()
+        opened_fd = get_open_file_descriptors().difference(fds_before_open)
+        cache.close()
+        fds_after_close = get_open_file_descriptors()
+        unclosed_fd = opened_fd.intersection(fds_after_close)
+        self.assertEqual(fds_before_open, fds_after_close)
+        self.assertEqual(unclosed_fd, set())
+
+    def test_cache_open_twice_leaks_fds(self):
+        cache = apt.Cache()
+        fds_before_open = get_open_file_descriptors()
+        cache.open()
+        fds_after_open_twice = get_open_file_descriptors()
+        self.assertEqual(fds_before_open, fds_after_open_twice)
+
+    @if_sources_list_is_readable
+    def test_cache_close_download_fails(self):
+        cache = apt.Cache()
+        self.assertEqual(cache.required_download, 0)
+        cache.close()
+        with self.assertRaises(apt.cache.CacheClosedException):
+            cache.required_download
 
     def test_get_provided_packages(self):
         apt.apt_pkg.config.set("Apt::architecture", "i386")