Merge lp:~pitti/aptdaemon/pygobject-fixes into lp:aptdaemon

[Martin Pitt]

This makes aptdaemon work with both PyGObject 3.7.2 as well as older versions (tested 3.4.2). See bug 1080736 for details.

[Michael Vogt (mvo)]

AFAICT this is not exactly what we want. The __call__ override in the metaclass is called everytime that a new AptTransaction() is created and it will return the transaction from the cache if its there already. This will
only work for metaclasses AFAICT.

With the branch this appears to be no longer working, i.e. the following testcase illustrates it:
$ bzr diff
=== modified file 'tests/test_client.py'
--- tests/test_client.py 2012-08-28 10:50:54 +0000
+++ tests/test_client.py 2012-11-22 15:42:12 +0000
@@ -110,6 +110,11 @@
         self.assertEqual(deferred.result.value.code,
                          aptdaemon.enums.ERROR_DEP_RESOLUTION_FAILED)

+ def test_tid(self):
+ tid = "/meep"
+ trans = aptdaemon.client.AptTransaction(tid, None)
+ trans2 = aptdaemon.client.AptTransaction(tid, None)
+ self.assertEqual(trans, trans2)

 if __name__ == "__main__":

This will work in trunk but fail in this branch. The test should probably be added to the branch just to ensure the behavior stays consistent.

review: Needs Fixing

[lp:~pitti/aptdaemon/pygobject-fixes]

867. By Martin Pitt on 2012-11-26

Add test for TID caching

aptdaemon.client has some metaclass magic to cache transactions with the same
transaction ID. Add a test case that this works properly.

Suggested by Michael Vogt and spotted in
https://code.launchpad.net/~pitti/aptdaemon/pygobject-fixes/+merge/134942

[Martin Pitt (pitti)]

Indeed, thanks for this! I committed this test case to trunk already, as it's independent from this proposed change:
http://bazaar.launchpad.net/~aptdaemon-developers/aptdaemon/main/revision/867

[Martin Pitt (pitti)]

Right, so I misunderstood what this is supposed to do. This is supposed to intercept a constructor call, not a method call on an already constructed Transaction object (which is why this first approach doesn't work).

[Martin Pitt (pitti)]

The new patch overrides __new__() which is the place to do changes in object creation. This satisfies the new test case for verifying the caching as well.

One thing that needs to be investigated with this is this failure:

======================================================================
FAIL: test_replace (tests.test_configfileprompt.ConfigFilePromptTestCase)
Test replacing the current configuration file.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/martin/upstream/aptdaemon/tests/test_configfileprompt.py", line 114, in test_replace
    "#Just another config file.\n")
AssertionError: 'BliBlaBlub' != '#Just another config file.\n'
    "'BliBlaBlub' != '#Just another config file.\\n'" = '%s != %s' % (safe_repr('BliBlaBlub'), safe_repr('#Just another config file.\n'))
    "'BliBlaBlub' != '#Just another config file.\\n'" = self._formatMessage("'BliBlaBlub' != '#Just another config file.\\n'", "'BliBlaBlub' != '#Just another config file.\\n'")
>> raise self.failureException("'BliBlaBlub' != '#Just another config file.\\n'")

I did not get this without the patch, but this looks totally unrelated to transaction caching, and perhaps might be a race condition?

review: Needs Resubmitting

[lp:~pitti/aptdaemon/pygobject-fixes]

868. By Michael Vogt on 2012-11-26

tests/test_pk.py: fix long keyid test

869. By Michael Vogt on 2012-11-26

setup.py: add workaround for nose failure

870. By Michael Vogt on 2012-11-26

tests/test_pk.py: *cough* typo *cough*

[Michael Vogt (mvo)]

Thanks for your update.

It appears the caching is now working but a bit too agressive, here is a testcase that fails with the branch:

    def test_tid_not_equal(self):
        trans = aptdaemon.client.AptTransaction("/tid1", None)
        trans2 = aptdaemon.client.AptTransaction("/tid2", None)
        self.assertNotEqual(trans.tid, trans2.tid)

The problem appears to be that the first arg of "__new__" is the class, not the instance, so the cache will
only ever have one entry AFAICT.

[Martin Pitt (pitti)]

Argh, of course! Thanks for spotting. Fixed now. Now this works fine with python2 + pygi 3.4, python3 + pygi 3.4, and python3 + pygi 3.7.2.

[lp:~pitti/aptdaemon/pygobject-fixes]

871. By Martin Pitt on 2012-11-26

Drop MemoizedTransaction mixin.

This uses internal implementation details of PyGObject (GObject.GObjectMeta)
which stopped working with PyGObject 3.7.2. Replace it with overriding __new__
which does the caching.

Extend ClientTest.test_tid_caching to ensure that different TIDs actually
generate different transactions.

[Michael Vogt (mvo)]

Thanks again for the update. This is looking good now

review: Approve

=== modified file 'aptdaemon/client.py'
--- aptdaemon/client.py	2012-08-09 18:57:09 +0000
+++ aptdaemon/client.py	2012-11-26 10:07:24 +0000
@@ -51,32 +51,7 @@
 __all__ = ("AptTransaction", "AptClient", "get_transaction", "get_aptdaemon")
 
 
-class MemoizedTransaction(type):
-
-    """Metaclass to cache transactions."""
-
-    cache = weakref.WeakValueDictionary()
-
-    def __call__(mcs, *args):
-        tid = args[0]
-        try:
-            return mcs.cache[tid]
-        except KeyError:
-            mcs.cache[tid] = value = \
-                    super(MemoizedTransaction, mcs).__call__(*args)
-            return value
-
-
-class MemoizedMixInMeta(MemoizedTransaction, GObject.GObjectMeta):
-
-    """Helper meta class for merging"""
-
-# This code is used to make meta classes work with Python 2 and 3 at the same
-# time, see http://mikewatkins.ca/2008/11/29/python-2-and-3-metaclasses/
-MemoizedMixIn = MemoizedMixInMeta("MemoizedMixIn", (GObject.GObject,), {})
-
-
-class AptTransaction(MemoizedMixIn):
+class AptTransaction(GObject.Object):
 
     """Represents an aptdaemon transaction.
 
@@ -518,6 +493,16 @@
                                               GObject.TYPE_STRING)),
                     }
 
+    _tid_cache = weakref.WeakValueDictionary()
+
+    def __new__(cls, tid, *args):
+        """Cache transactions with identical tid."""
+        try:
+            return AptTransaction._tid_cache[tid]
+        except KeyError:
+            AptTransaction._tid_cache[tid] = value = \
+                    GObject.Object.__new__(cls, tid, *args)
+            return value
 
     def __init__(self, tid, bus=None):
         GObject.GObject.__init__(self)

=== modified file 'tests/test_client.py'
--- tests/test_client.py	2012-11-26 07:16:42 +0000
+++ tests/test_client.py	2012-11-26 10:07:24 +0000
@@ -116,7 +116,9 @@
         tid = "/meep"
         trans = aptdaemon.client.AptTransaction(tid, None)
         trans2 = aptdaemon.client.AptTransaction(tid, None)
+        trans3 = aptdaemon.client.AptTransaction("/meep2", None)
         self.assertEqual(trans, trans2)
+        self.assertNotEqual(trans, trans3)
 
 
 if __name__ == "__main__":