heads up from the python front

heads up from the python front

[Justus Winter]

Hey everyone :)

after getting to know nmbug a little better (it's actually very nice
to track bugs and patches this way...) I did some work on the python
bindings. tl;dr version: housekeeping, python3.2 support, fixed nasty
bug.

I familiarized myself with nmbug and went through all the threads
tagged with notmuch::python, added and updated few tags here and there
and started working on the open issues.

Python 3.2 support
------------------

I merged the last patch of a patchset[0] I wrote in december that makes
it possible to use the python bindings with both python2.x and
python3.2.

I do not now how complete the port is, most notably the notmuch.py
script does not work with 3.x. But it is complete enough to make
afew[1] work using python3.2 and the vanilla notmuch bindings.

If you want to help out and have a small script that uses the bindings
I'd like to invite you to try to port your script and report any
issues.

Fix random crashes when using the bindings
------------------------------------------

I found a nasty bug I introduced with a patchset[2] that was supposed
to make the bindings more robust. Annotating pointers returned from
libnotmuch functions called using ctypes allows the ctypes framework
to do typechecking. But I accidentally broke the error handling
code. Citing the commit message:

    Before 3434d1940 the return values of libnotmuch functions were
    declared as c_void_p and the code checking for errors compared the
    returned value to None, which is the ctypes equivalent of a NULL
    pointer.

    But said commit wrapped all the data types in python classes and the
    semantic changed in a subtle way. If a function returns NULL, the
    wrapped python value is falsish, but no longer equal to None.

In fact the minimal test case triggering the bug is:

    import os
    import notmuch

    db_path = os.path.expanduser('~/Maildir')

    db_0 = notmuch.Database(db_path, mode=notmuch.Database.MODE.READ_WRITE)
    db_1 = notmuch.Database(db_path, mode=notmuch.Database.MODE.READ_WRITE)

The problem was most apparent when opening the database fails because
it has been locked by someone else. The patch regarding the function
Database.open looks like this:

         res = Database._open(_str(path), mode)
 
-        if res is None:
+        if not res:
             raise NotmuchError(message="Could not open the specified database")
         self._db = res

The old code fails to notify the callee of the error who causes a
segfault later if he uses that database reference which is in fact
NULL.

I feel kind of bad since I severely broke the error handling for all
pythonic notmuch users out there, is there any chance of a bugfix
release? The patch[3] is really simple and I'd say it's trivial to
backport it to the last release. What do you think?

Justus

0: id:1323860305-15802-1-git-send-email-4winter@informatik.uni-hamburg.de
1: https://github.com/teythoon/afew
2: id:1318198374-926-1-git-send-email-4winter@informatik.uni-hamburg.de
3: 8015cbff263606f009b5750d23b28ee332c25db8

Re: heads up from the python front

[David Bremner]

On Sun, 22 Jan 2012 06:01:41 -0000, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> 
> I feel kind of bad since I severely broke the error handling for all
> pythonic notmuch users out there, is there any chance of a bugfix
> release? The patch[3] is really simple and I'd say it's trivial to
> backport it to the last release. What do you think?
> 

I'm not opposed in principle; I tentatively plan on a bugfix release for
Aaron's mml quoting patch, so we can have the two fixes in one release.
It would be good to get a bit more feedback on the patch itself. Can you
post a backported (to branch release) version of the patch to the list?

> 3: 8015cbff263606f009b5750d23b28ee332c25db8

[PATCH] python: fix error handling

[Justus Winter]

Before 3434d1940 the return values of libnotmuch functions were
declared as c_void_p and the code checking for errors compared the
returned value to None, which is the ctypes equivalent of a NULL
pointer.

But said commit wrapped all the data types in python classes and the
semantic changed in a subtle way. If a function returns NULL, the
wrapped python value is falsish, but no longer equal to None.

Backported from master to 0.11.
---
 bindings/python/notmuch/database.py |   16 ++++++++--------
 bindings/python/notmuch/filename.py |    2 +-
 bindings/python/notmuch/message.py  |    6 +++---
 bindings/python/notmuch/tag.py      |    2 +-
 bindings/python/notmuch/thread.py   |    6 +++---
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index 7923f76..0074ba3 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -168,7 +168,7 @@ class Database(object):
 
         res = Database._create(_str(path), Database.MODE.READ_WRITE)
 
-        if res is None:
+        if not res:
             raise NotmuchError(
                 message="Could not create the specified database")
         self._db = res
@@ -188,7 +188,7 @@ class Database(object):
         """
         res = Database._open(_str(path), mode)
 
-        if res is None:
+        if not res:
             raise NotmuchError(message="Could not open the specified database")
         self._db = res
 
@@ -645,7 +645,7 @@ class Query(object):
         self._db = db
         # create query, return None if too little mem available
         query_p = Query._create(db.db_p, _str(querystr))
-        if query_p is None:
+        if not query_p:
             raise NullPointerError
         self._query = query_p
 
@@ -679,7 +679,7 @@ class Query(object):
         self._assert_query_is_initialized()
         threads_p = Query._search_threads(self._query)
 
-        if threads_p is None:
+        if not threads_p:
             raise NullPointerError
         return Threads(threads_p, self)
 
@@ -693,7 +693,7 @@ class Query(object):
         self._assert_query_is_initialized()
         msgs_p = Query._search_messages(self._query)
 
-        if msgs_p is None:
+        if not msgs_p:
             raise NullPointerError
         return Messages(msgs_p, self)
 
@@ -759,7 +759,7 @@ class Directory(object):
     def _assert_dir_is_initialized(self):
         """Raises a NotmuchError(:attr:`STATUS`.NOT_INITIALIZED)
         if dir_p is None"""
-        if self._dir_p is None:
+        if not self._dir_p:
             raise NotmuchError(STATUS.NOT_INITIALIZED)
 
     def __init__(self, path, dir_p, parent):
@@ -920,7 +920,7 @@ class Filenames(object):
     _move_to_next.restype = None
 
     def next(self):
-        if self._files_p is None:
+        if not self._files_p:
             raise NotmuchError(STATUS.NOT_INITIALIZED)
 
         if not self._valid(self._files_p):
@@ -946,7 +946,7 @@ class Filenames(object):
                      # NotmuchError(:attr:`STATUS`.NOT_INITIALIZED)
                      for file in files: print file
         """
-        if self._files_p is None:
+        if not self._files_p:
             raise NotmuchError(STATUS.NOT_INITIALIZED)
 
         i = 0
diff --git a/bindings/python/notmuch/filename.py b/bindings/python/notmuch/filename.py
index a7cd7e6..f7313ec 100644
--- a/bindings/python/notmuch/filename.py
+++ b/bindings/python/notmuch/filename.py
@@ -69,7 +69,7 @@ class Filenames(object):
              reference to it, so we can automatically delete the db object
              once all derived objects are dead.
         """
-        if files_p is None:
+        if not files_p:
             raise NotmuchError(STATUS.NULL_POINTER)
 
         self._files = files_p
diff --git a/bindings/python/notmuch/message.py b/bindings/python/notmuch/message.py
index ce8e718..5540df3 100644
--- a/bindings/python/notmuch/message.py
+++ b/bindings/python/notmuch/message.py
@@ -116,7 +116,7 @@ class Messages(object):
         :TODO: Make the iterator work more than once and cache the tags in
                the Python object.(?)
         """
-        if msgs_p is None:
+        if not msgs_p:
             raise NotmuchError(STATUS.NULL_POINTER)
 
         self._msgs = msgs_p
@@ -321,7 +321,7 @@ class Message(object):
               automatically delete the parent object once all derived
               objects are dead.
         """
-        if msg_p is None:
+        if not msg_p:
             raise NotmuchError(STATUS.NULL_POINTER)
         self._msg = msg_p
         #keep reference to parent, so we keep it alive
@@ -380,7 +380,7 @@ class Message(object):
 
         msgs_p = Message._get_replies(self._msg)
 
-        if msgs_p is None:
+        if not msgs_p:
             return None
 
         return Messages(msgs_p, self)
diff --git a/bindings/python/notmuch/tag.py b/bindings/python/notmuch/tag.py
index 2fb7d32..4881db9 100644
--- a/bindings/python/notmuch/tag.py
+++ b/bindings/python/notmuch/tag.py
@@ -70,7 +70,7 @@ class Tags(object):
         :TODO: Make the iterator optionally work more than once by
                cache the tags in the Python object(?)
         """
-        if tags_p is None:
+        if not tags_p:
             raise NotmuchError(STATUS.NULL_POINTER)
 
         self._tags = tags_p
diff --git a/bindings/python/notmuch/thread.py b/bindings/python/notmuch/thread.py
index 5058846..594fa52 100644
--- a/bindings/python/notmuch/thread.py
+++ b/bindings/python/notmuch/thread.py
@@ -97,7 +97,7 @@ class Threads(object):
         :TODO: Make the iterator work more than once and cache the tags in
                the Python object.(?)
         """
-        if threads_p is None:
+        if not threads_p:
             raise NotmuchError(STATUS.NULL_POINTER)
 
         self._threads = threads_p
@@ -227,7 +227,7 @@ class Thread(object):
               automatically delete the parent object once all derived
               objects are dead.
         """
-        if thread_p is None:
+        if not thread_p:
             raise NotmuchError(STATUS.NULL_POINTER)
         self._thread = thread_p
         #keep reference to parent, so we keep it alive
@@ -288,7 +288,7 @@ class Thread(object):
 
         msgs_p = Thread._get_toplevel_messages(self._thread)
 
-        if msgs_p is None:
+        if not msgs_p:
             raise NotmuchError(STATUS.NULL_POINTER)
 
         return Messages(msgs_p, self)
-- 
1.7.8.3

Re: [PATCH] python: fix error handling

[Tomi Ollila]

On Sun, 22 Jan 2012 14:09:35 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Before 3434d1940 the return values of libnotmuch functions were
> declared as c_void_p and the code checking for errors compared the
> returned value to None, which is the ctypes equivalent of a NULL
> pointer.
> 
> But said commit wrapped all the data types in python classes and the
> semantic changed in a subtle way. If a function returns NULL, the
> wrapped python value is falsish, but no longer equal to None.
> 
> Backported from master to 0.11.
> ---

LGTM.

Tomi

Re: [PATCH] python: fix error handling

[David Bremner]

On Sun, 22 Jan 2012 14:09:35 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Before 3434d1940 the return values of libnotmuch functions were
> declared as c_void_p and the code checking for errors compared the
> returned value to None, which is the ctypes equivalent of a NULL
> pointer.

Looks good to me as well. I pushed it to branch release. Could you make
a NEWS patch against branch release (i.e. start 0.11.1) ?

d

[PATCH] Add a NEWS section for 0.11.1 and document the python error handling bugfix

[Justus Winter]

---
 NEWS |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/NEWS b/NEWS
index bf21e64..3d2c2a8 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,17 @@
+Notmuch 0.11.1 (2012-mm-dd)
+===========================
+
+Bug-fix release.
+----------------
+
+Fix error handling in python bindings.
+
+  The python bindings in 0.11 failed to detect NULL pointers being
+  returned from libnotmuch functions and thus failed to raise
+  exceptions to indicate the error condition. Any subsequent calls
+  into libnotmuch caused segmentation faults.
+
+
 Notmuch 0.11 (2012-01-13)
 =========================
 
-- 
1.7.8.3

bugfix release (was: heads up from the python front)

[Justus Winter]

Hey David,

Quoting David Bremner (2012-01-22 13:30:35)
>On Sun, 22 Jan 2012 06:01:41 -0000, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
>> 
>> I feel kind of bad since I severely broke the error handling for all
>> pythonic notmuch users out there, is there any chance of a bugfix
>> release? The patch[3] is really simple and I'd say it's trivial to
>> backport it to the last release. What do you think?
>> 
>
>I'm not opposed in principle; I tentatively plan on a bugfix release for
>Aaron's mml quoting patch, so we can have the two fixes in one release.

What about that bugfix release?

Cheers,
Justus