Re: [PATCH v2 2/2] python-cffi: use config_pairs API in ConfigIterator

2022-02-17 Thread Floris Bruynooghe
lgtm i think :)

On Wed 16 Feb 2022 at 22:08 -0400, David Bremner wrote:

> This returns all of the config keys with non-empty values, not just
> those that happen to be stored in the database.
> ---
>  bindings/python-cffi/notmuch2/_build.py   | 16 -
>  bindings/python-cffi/notmuch2/_config.py  | 40 +++
>  bindings/python-cffi/tests/test_config.py | 24 --
>  test/T055-path-config.sh  |  1 -
>  4 files changed, 49 insertions(+), 32 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_build.py 
> b/bindings/python-cffi/notmuch2/_build.py
> index a55b484f..349bb79d 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -97,7 +97,7 @@ ffibuilder.cdef(
>  typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
>  typedef struct _notmuch_directory notmuch_directory_t;
>  typedef struct _notmuch_filenames notmuch_filenames_t;
> -typedef struct _notmuch_config_list notmuch_config_list_t;
> +typedef struct _notmuch_config_pairs notmuch_config_pairs_t;
>  typedef struct _notmuch_indexopts notmuch_indexopts_t;
>  
>  const char *
> @@ -325,18 +325,18 @@ ffibuilder.cdef(
>  notmuch_database_set_config (notmuch_database_t *db, const char *key, 
> const char *value);
>  notmuch_status_t
>  notmuch_database_get_config (notmuch_database_t *db, const char *key, 
> char **value);
> -notmuch_status_t
> -notmuch_database_get_config_list (notmuch_database_t *db, const char 
> *prefix, notmuch_config_list_t **out);
> +notmuch_config_pairs_t *
> +notmuch_config_get_pairs (notmuch_database_t *db, const char *prefix);
>  notmuch_bool_t
> -notmuch_config_list_valid (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_valid (notmuch_config_pairs_t *config_list);
>  const char *
> -notmuch_config_list_key (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_key (notmuch_config_pairs_t *config_list);
>  const char *
> -notmuch_config_list_value (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_value (notmuch_config_pairs_t *config_list);
>  void
> -notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_move_to_next (notmuch_config_pairs_t *config_list);
>  void
> -notmuch_config_list_destroy (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_destroy (notmuch_config_pairs_t *config_list);
>  """
>  )
>  
> diff --git a/bindings/python-cffi/notmuch2/_config.py 
> b/bindings/python-cffi/notmuch2/_config.py
> index 29de6495..603fdcbf 100644
> --- a/bindings/python-cffi/notmuch2/_config.py
> +++ b/bindings/python-cffi/notmuch2/_config.py
> @@ -13,27 +13,42 @@ class ConfigIter(base.NotmuchIter):
>  def __init__(self, parent, iter_p):
>  super().__init__(
>  parent, iter_p,
> -fn_destroy=capi.lib.notmuch_config_list_destroy,
> -fn_valid=capi.lib.notmuch_config_list_valid,
> -fn_get=capi.lib.notmuch_config_list_key,
> -fn_next=capi.lib.notmuch_config_list_move_to_next)
> +fn_destroy=capi.lib.notmuch_config_pairs_destroy,
> +fn_valid=capi.lib.notmuch_config_pairs_valid,
> +fn_get=capi.lib.notmuch_config_pairs_key,
> +fn_next=capi.lib.notmuch_config_pairs_move_to_next)
>  
>  def __next__(self):
> -item = super().__next__()
> -return base.BinString.from_cffi(item)
> -
> +# skip pairs whose value is NULL
> +while capi.lib.notmuch_config_pairs_valid(super()._iter_p):
> +val_p = capi.lib.notmuch_config_pairs_value(super()._iter_p)
> +key_p = capi.lib.notmuch_config_pairs_key(super()._iter_p)
> +if key_p == capi.ffi.NULL:
> +# this should never happen
> +raise errors.NullPointerError
> +key = base.BinString.from_cffi(key_p)
> +capi.lib.notmuch_config_pairs_move_to_next(super()._iter_p)
> +if val_p != capi.ffi.NULL and base.BinString.from_cffi(val_p) != 
> "":
> +return key
> +self._destroy()
> +raise StopIteration
>  
>  class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
> -"""The config key/value pairs stored in the database.
> +"""The config key/value pairs loaded from the database, config file,
> +and and/or defaults.
>  
>  The entries are exposed as a :class:`collections.abc.MutableMapping` 
> object.
>  Note that setting a value to an empty string is the same as deleting it.
>  
> +Mutating (deleting or updating values) in the map persists only in
> +the database, which can be shadowed by config files.
> +
>  :param parent: the parent object
>  :param ptr_name: the name of the attribute on the parent which will
> return the memory pointer.  This allows this 

Re: [PATCH 2/2] python-cffi: use config_pairs API in ConfigIterator

2022-02-16 Thread Floris Bruynooghe
On Fri 11 Feb 2022 at 09:04 -0400, David Bremner wrote:

> This returns all of the config keys with non-empty values, not just
> those that happen to be stored in the database.
> ---
>  bindings/python-cffi/notmuch2/_build.py   | 16 +-
>  bindings/python-cffi/notmuch2/_config.py  | 37 +++
>  bindings/python-cffi/tests/test_config.py | 22 --
>  test/T055-path-config.sh  |  1 -
>  4 files changed, 44 insertions(+), 32 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_build.py 
> b/bindings/python-cffi/notmuch2/_build.py
> index a55b484f..349bb79d 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -97,7 +97,7 @@ ffibuilder.cdef(
>  typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
>  typedef struct _notmuch_directory notmuch_directory_t;
>  typedef struct _notmuch_filenames notmuch_filenames_t;
> -typedef struct _notmuch_config_list notmuch_config_list_t;
> +typedef struct _notmuch_config_pairs notmuch_config_pairs_t;
>  typedef struct _notmuch_indexopts notmuch_indexopts_t;
>  
>  const char *
> @@ -325,18 +325,18 @@ ffibuilder.cdef(
>  notmuch_database_set_config (notmuch_database_t *db, const char *key, 
> const char *value);
>  notmuch_status_t
>  notmuch_database_get_config (notmuch_database_t *db, const char *key, 
> char **value);
> -notmuch_status_t
> -notmuch_database_get_config_list (notmuch_database_t *db, const char 
> *prefix, notmuch_config_list_t **out);
> +notmuch_config_pairs_t *
> +notmuch_config_get_pairs (notmuch_database_t *db, const char *prefix);
>  notmuch_bool_t
> -notmuch_config_list_valid (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_valid (notmuch_config_pairs_t *config_list);
>  const char *
> -notmuch_config_list_key (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_key (notmuch_config_pairs_t *config_list);
>  const char *
> -notmuch_config_list_value (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_value (notmuch_config_pairs_t *config_list);
>  void
> -notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_move_to_next (notmuch_config_pairs_t *config_list);
>  void
> -notmuch_config_list_destroy (notmuch_config_list_t *config_list);
> +notmuch_config_pairs_destroy (notmuch_config_pairs_t *config_list);
>  """
>  )
>  
> diff --git a/bindings/python-cffi/notmuch2/_config.py 
> b/bindings/python-cffi/notmuch2/_config.py
> index 29de6495..d73539c5 100644
> --- a/bindings/python-cffi/notmuch2/_config.py
> +++ b/bindings/python-cffi/notmuch2/_config.py
> @@ -13,27 +13,39 @@ class ConfigIter(base.NotmuchIter):
>  def __init__(self, parent, iter_p):
>  super().__init__(
>  parent, iter_p,
> -fn_destroy=capi.lib.notmuch_config_list_destroy,
> -fn_valid=capi.lib.notmuch_config_list_valid,
> -fn_get=capi.lib.notmuch_config_list_key,
> -fn_next=capi.lib.notmuch_config_list_move_to_next)
> +fn_destroy=capi.lib.notmuch_config_pairs_destroy,
> +fn_valid=capi.lib.notmuch_config_pairs_valid,
> +fn_get=capi.lib.notmuch_config_pairs_key,
> +fn_next=capi.lib.notmuch_config_pairs_move_to_next)
>  
>  def __next__(self):
> -item = super().__next__()
> -return base.BinString.from_cffi(item)
> -
> +# skip pairs whose value is NULL
> +while capi.lib.notmuch_config_pairs_valid (super()._iter_p):

FWIW having spaces between the function name and parentheses is rather
uncommon for python style.  Though of course complaining about style
without using an auto-formatter is pretty meh these days :)

> +val_p = capi.lib.notmuch_config_pairs_value (super()._iter_p)
> +key_p = capi.lib.notmuch_config_pairs_key (super()._iter_p)
> +key = base.BinString.from_cffi(key_p)

does key_p need a NULL check first or can it never be NULL?

> +capi.lib.notmuch_config_pairs_move_to_next (super()._iter_p)
> +if val_p != capi.ffi.NULL and base.BinString.from_cffi(val_p) != 
> "":
> +return key
> +self._destroy()
> +raise StopIteration
>  
>  class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
> -"""The config key/value pairs stored in the database.
> +"""The config key/value pairs loaded from the database, config file,
> +and and/or defaults.
>  
>  The entries are exposed as a :class:`collections.abc.MutableMapping` 
> object.
>  Note that setting a value to an empty string is the same as deleting it.
>  
> +Mutating (deleting or updating values) in the map persists only in
> +the database, which can be shadowed by config files.
> +
>  :param parent: the parent object
>  :param 

Re: Python binding SIGABRT/SIGSEGV

2022-02-16 Thread Floris Bruynooghe
On Thu 10 Feb 2022 at 13:16 +0100, Michael J Gruber wrote:

> Austin Lund venit, vidit, dixit 2022-02-10 06:56:12:
>> I'm clearly doing this python code wrong by not using the iterator correctly:
>> 
>> > import notmuch2
>> > 
>> > d = notmuch2.Database()
>> > m = list(d.messages("since:today"))
>> > p = m[0].path
>> > print(p)
>> 
>> But I seem to be getting a SIGABRT instead of a python stack trace.  Is
>> this the expected behaviour?
>
> You didn't expect it :)
>
> And this can be confusing. d.messages() returns an iterator through
> Message objects whose lifetime depends on the iterator. In contrast,
> thread.get_messages() returns on iterator through OwnedMessage objects
> whose lifetime depends on the thread.
>
> As soon as the iterator is depleted, the returned objects are (possibly)
> gone. (Well, because it's return by reference in Python, and ...)
>
> If you're interested in m[0] only you can "cheat" by not depleting the
> iterator:
>
> mm = next(d.messages("since:today"))
>
> p = mm.path
>
> This never frees the object (I think).
>
> My attempts with notmuch2._message.OwnedMessage (and db as parent)
> failed. There must be a better way, possibly using a context manager or
> search.
>
> I guess usually people just use the iterator in a for loop and do
> something with the message inside the loop (while the iterator is not
> depleted), such as converting it into a proper email.Message object
> (i.e. instantiating a new object from it).

Hum, to be fair I consider this a serious bug in the notmuch2 bindings.
You should not be able to crash whatever you do and not need to
understand the internal notmuch memory model.  I tried to build the
bindings so they would keep alive what they need, but it seems it fails
here.  It would be good to figure out if this can be fixed.

Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

2022-01-11 Thread Floris Bruynooghe
On Sun 09 Jan 2022 at 09:26 -0400, David Bremner wrote:
> One thing I would like to think
> about is the length of time it takes to run the pytests. It is not
> currently the bottleneck in running the parallel tests for me, but it is
> among the slower T*.sh. So it might be nice at some point to split into
> several invocations for pytest, assuming multiple invocations of pytest
> can run in parallel.

There is the pytest-xdist plugin which is widely used and probably will
work fine on the notmuch testsuite (I even contributed to the plugin at
some point).  It will split the workload between however many processes
you choose, running them in parallel.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

2022-01-08 Thread Floris Bruynooghe
On Sat 08 Jan 2022 at 10:03 -0400, David Bremner wrote:

> If we return regular Message objects, python will try to destroy them,
> and the underlying notmuch object, causing e.g. the crash [1].
>
> [1]: id:87sfu6utxg@tethera.net
> ---
>  bindings/python-cffi/notmuch2/_message.py | 4 ++--
>  test/T392-python-cffi-notmuch.sh  | 2 --
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_message.py 
> b/bindings/python-cffi/notmuch2/_message.py
> index a460d8c1..aa1cb875 100644
> --- a/bindings/python-cffi/notmuch2/_message.py
> +++ b/bindings/python-cffi/notmuch2/_message.py
> @@ -371,14 +371,14 @@ class Message(base.NotmuchObject):
>  This method will only work if the message was created from a
>  thread.  Otherwise it will yield no results.
>  
> -:returns: An iterator yielding :class:`Message` instances.
> +:returns: An iterator yielding :class:`OwnedMessage` instances.
>  :rtype: MessageIter
>  """
>  # The notmuch_messages_valid call accepts NULL and this will
>  # become an empty iterator, raising StopIteration immediately.
>  # Hence no return value checking here.
>  msgs_p = capi.lib.notmuch_message_get_replies(self._msg_p)
> -return MessageIter(self, msgs_p, db=self._db)
> +return MessageIter(self, msgs_p, db=self._db, msg_cls=OwnedMessage)
>  
>  def __hash__(self):
>  return hash(self.messageid)
> diff --git a/test/T392-python-cffi-notmuch.sh 
> b/test/T392-python-cffi-notmuch.sh
> index 50012c55..15c8fc6b 100755
> --- a/test/T392-python-cffi-notmuch.sh
> +++ b/test/T392-python-cffi-notmuch.sh
> @@ -24,13 +24,11 @@ show_msgs(thread, 0)
>  EOF
>  
>  test_begin_subtest "recursive traversal of replies (no crash)"
> -test_subtest_known_broken
>  test_python < recurse.py
>  error=$?
>  test_expect_equal "${error}" 0
>  
>  test_begin_subtest "recursive traversal of replies (output)"
> -test_subtest_known_broken
>  test_python < recurse.py
>  tail -n 10 < OUTPUT > OUTPUT.sample
>  cat < EXPECTED

oh, nice debugging!  And yes, this seems like the right fix, glad the
mechanism was at least already in place.

With respect to the docs, they seem clear enough.  Probably I missed
this or I simply didn't realise that the replies iter is basically a
thread owning it.

Only thing I'd do different is write a pytest test for this as well, but
than I wouldn't have written the notmuch tests. So I don't think this
matters that much.

Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH] bindings/python-cffi: search for config by default

2022-01-08 Thread Floris Bruynooghe
On Sat 08 Jan 2022 at 12:19 -0400, David Bremner wrote:

> The previous (pre-0.34.2) constructor searched for a config file but
> only if the database path was not specified, and only to retrieve
> database.path. Neither of the available options (CONFIG.SEARCH or
> CONFIG.NONE) matches this semantics exactly, but CONFIG.SEARCH causes
> less breakage for people who relied on the old behaviour to set their
> database.path [1]. Since it also seems like the friendlier option in
> the long run, this commit switches to CONFIG.SEARCH as default.
>
> [1]: id:87fsqijx7u@metapensiero.it
> ---
>  bindings/python-cffi/notmuch2/_database.py  | 2 +-
>  bindings/python-cffi/tests/test_database.py | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_database.py 
> b/bindings/python-cffi/notmuch2/_database.py
> index 14a8f15c..d7485b4d 100644
> --- a/bindings/python-cffi/notmuch2/_database.py
> +++ b/bindings/python-cffi/notmuch2/_database.py
> @@ -139,7 +139,7 @@ class Database(base.NotmuchObject):
>  path = os.fsencode(path)
>  return path
>  
> -def __init__(self, path=None, mode=MODE.READ_ONLY, config=CONFIG.EMPTY):
> +def __init__(self, path=None, mode=MODE.READ_ONLY, config=CONFIG.SEARCH):
>  if isinstance(mode, str):
>  mode = self.STR_MODE_MAP[mode]
>  self.mode = mode
> diff --git a/bindings/python-cffi/tests/test_database.py 
> b/bindings/python-cffi/tests/test_database.py
> index 9b3219c0..473723d5 100644
> --- a/bindings/python-cffi/tests/test_database.py
> +++ b/bindings/python-cffi/tests/test_database.py
> @@ -13,7 +13,7 @@ import notmuch2._message as message
>  
>  @pytest.fixture
>  def db(maildir):
> -with dbmod.Database.create(maildir.path) as db:
> +with dbmod.Database.create(maildir.path, 
> config=notmuch2.Database.CONFIG.SEARCH) as db:
>  yield db

LGTM
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: Python notmuch2 bridge, something changed in the Database constructor

2022-01-01 Thread Floris Bruynooghe
On Fri 24 Dec 2021 at 09:03 -0400, David Bremner wrote:
>
> @Floris: when I made the change to the Database constructor, I chose the
> default as CONFIG.NONE because this matched the previous semantics
> _except_ for finding the database path. But since I don't want to use
> the ad hoc default_path method anymore, maybe a better default would be
> CONFIG.SEARCH. I guess most users of the python bindings will be OK with
> the the configuration values in .notmuch-config being loaded by default.
> What do you think?

Originally my intention with opening the database was that doing the
same as notmuch itself does by default would be the default and most
natural thing to do (whether or not I succeeded at the time I have no
idea).  So reading .notmuch-config by default and using it correctly
seems fine to me.  Backwards compatibility is always tricky I guess, but
following notmuch's behaviour itself seems reasonable to me.

As a random data point I just checked my own use and I seem to be
passing the database path explicitly.


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH] bindings/python-cffi: add matched property to message objects

2022-01-01 Thread Floris Bruynooghe
On Sat 01 Jan 2022 at 10:42 -0400, David Bremner wrote:

> Existing users of the legacy python bindings use
> message.get_flags(Message.FLAG.MATCH) to determine which messages in a
> thread matched. Since the bindings don't provide get_flags anymore,
> they should provide a property analogous to the existing "excluded"
> property.
> ---
>  bindings/python-cffi/notmuch2/_message.py  | 14 ++
>  bindings/python-cffi/tests/test_message.py |  3 +++
>  bindings/python-cffi/tests/test_thread.py  |  7 +++
>  3 files changed, 24 insertions(+)
>
> diff --git a/bindings/python-cffi/notmuch2/_message.py 
> b/bindings/python-cffi/notmuch2/_message.py
> index 2f232076..a460d8c1 100644
> --- a/bindings/python-cffi/notmuch2/_message.py
> +++ b/bindings/python-cffi/notmuch2/_message.py
> @@ -205,6 +205,20 @@ class Message(base.NotmuchObject):
>  self._msg_p, capi.lib.NOTMUCH_MESSAGE_FLAG_EXCLUDED)
>  return bool(ret)
>  
> +@property
> +def matched(self):
> +"""Indicates whether this message was matched by the query.
> +
> +When a thread is created from a search, some of the
> +messages may not match the original query.  This property
> +is set to *True* for those that do match.
> +
> +:raises ObjectDestroyedError: if used after destroyed.
> +"""
> +ret = capi.lib.notmuch_message_get_flag(
> +self._msg_p, capi.lib.NOTMUCH_MESSAGE_FLAG_MATCH)
> +return bool(ret)
> +
>  @property
>  def date(self):
>  """The message date as an integer.
> diff --git a/bindings/python-cffi/tests/test_message.py 
> b/bindings/python-cffi/tests/test_message.py
> index 532bf921..56701d05 100644
> --- a/bindings/python-cffi/tests/test_message.py
> +++ b/bindings/python-cffi/tests/test_message.py
> @@ -97,6 +97,9 @@ class TestMessage:
>  def test_ghost_no(self, msg):
>  assert not msg.ghost
>  
> +def test_matched_no(self,msg):
> +assert not msg.matched
> +
>  def test_date(self, msg):
>  # XXX Someone seems to treat things as local time instead of
>  # UTC or the other way around.
> diff --git a/bindings/python-cffi/tests/test_thread.py 
> b/bindings/python-cffi/tests/test_thread.py
> index 1f44b35d..fbef73ac 100644
> --- a/bindings/python-cffi/tests/test_thread.py
> +++ b/bindings/python-cffi/tests/test_thread.py
> @@ -57,6 +57,13 @@ def test_iter(thread):
>  def test_matched(thread):
>  assert thread.matched == 1
>  
> +def test_matched_iter(thread):
> +count = 0
> +msgs = list(iter(thread))
> +for msg in msgs:
> +if msg.matched:
> +count += 1
> +assert count == thread.matched
>  
>  def test_authors_type(thread):
>  assert isinstance(thread.authors, notmuch2.BinString)

LGTM
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


[PATCH] Fix error message when using notmuch_status_to_string

2021-11-06 Thread Floris Bruynooghe
The python exception class was incorrectly loading the error message
which resulted in unprintable exception objects.
---
 bindings/python-cffi/notmuch2/_errors.py  | 3 ++-
 bindings/python-cffi/tests/test_errors.py | 8 
 2 files changed, 10 insertions(+), 1 deletion(-)
 create mode 100644 bindings/python-cffi/tests/test_errors.py

diff --git a/bindings/python-cffi/notmuch2/_errors.py 
b/bindings/python-cffi/notmuch2/_errors.py
index f55cc96b..17c3ad9c 100644
--- a/bindings/python-cffi/notmuch2/_errors.py
+++ b/bindings/python-cffi/notmuch2/_errors.py
@@ -83,7 +83,8 @@ class NotmuchError(Exception):
 if self.message:
 return self.message
 elif self.status:
-return capi.lib.notmuch_status_to_string(self.status)
+char_str = capi.lib.notmuch_status_to_string(self.status)
+return capi.ffi.string(char_str).decode(errors='replace')
 else:
 return 'Unknown error'
 
diff --git a/bindings/python-cffi/tests/test_errors.py 
b/bindings/python-cffi/tests/test_errors.py
new file mode 100644
index ..c2519f86
--- /dev/null
+++ b/bindings/python-cffi/tests/test_errors.py
@@ -0,0 +1,8 @@
+from notmuch2 import _capi as capi
+from notmuch2 import _errors as errors
+
+def test_status_no_message():
+exc = errors.NotmuchError(capi.lib.NOTMUCH_STATUS_PATH_ERROR)
+assert exc.status == capi.lib.NOTMUCH_STATUS_PATH_ERROR
+assert exc.message is None
+assert str(exc) == 'Path supplied is illegal for this function'
-- 
2.33.0

___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Fix python exception object string repr

2021-11-06 Thread Floris Bruynooghe


This fixes a bug in the exception class which would produce
unprintable exceptions because it was not converting an error
code correctly to a string.

___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 4/4] python-cffi: switch to notmuch_database_{open,create}_with_config

2021-11-05 Thread Floris Bruynooghe
On Tue 02 Nov 2021 at 21:32 -0300, David Bremner wrote:

> PS: I took the liberty of replying to the list, hope that's OK.

whoops, totally.   I keep forgetting to hit the right reply key.
Personal feature request: notmuch-reply-dwim, bound to r, which replies
to the list for mailing list posts. ;-)


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [Tomi Ollila] [RFC PATCH] python-cffi out-of-tree build

2021-08-10 Thread Floris Bruynooghe
Hi David, everyone,

I had a vague recollection of reading from a single version file before
and indeed, looking through annotations it seems commit
3a42abb456893b71b530f099a1467400f2b0ea71 changed this.  Seems the
concern was that "pip install ." didn't work.  So perhaps that's
something else to check?

And err... thanks for your patience.  I know this is really slow!

Cheers,
Floris


On Mon 02 Aug 2021 at 07:44 -0300, David Bremner wrote:

> Any thoughts on this one?
> From: Tomi Ollila 
> Subject: [RFC PATCH] python-cffi out-of-tree build
> To: notmuch@notmuchmail.org
> Cc: tomi.oll...@iki.fi
> Date: Wed, 17 Feb 2021 23:36:00 +0200
>
> setup.py and _build.py to refer some other files based on directory
> where setup.py is located (os.path.dirname(sys.argv[0]).
>
> Dropped bindings/python-cffi/version.txt and refer ../../version.txt
> instead -- _build.py already refers ../../lib so why have version.txt
> twice (with identical content).
>
> Dropped copying of bindings/python-cffi source files to the build
> directory bindings/python-cffi in configure.
>
> ---
>
> an RFC alternative to id:20210215215410.28668-1-tomi.oll...@iki.fi
>
> I really don't know how this should be done... do they even in e.g.
> https://github.com/pypa/pip/issues/7555 ?
>
> Tested to work in in-tree build, and out-of-tree builds referencing
> .../notmuch/configure with absolute and relative paths, and all
> 3 builds worked.
>
> When doing out-of-tree build, only _capi.abi3.so appeared in
> bindings/python-cffi/build/stage/notmuch2/ -- in case of in-tree
> build all the notmuch2/*.py source files also got copied there...
>
> ... I tried this changei in setup.py:
> .  - packages=setuptools.find_packages(exclude=['tests'])
> .  + packages=setuptools.find_packages(dn0, exclude=['tests'])
> but then build broke.
>
>  bindings/Makefile.local | 5 +++--
>  bindings/python-cffi/notmuch2/_build.py | 6 +-
>  bindings/python-cffi/setup.py   | 9 +++--
>  bindings/python-cffi/version.txt| 1 -
>  configure   | 4 
>  5 files changed, 15 insertions(+), 10 deletions(-)
>  delete mode 100644 bindings/python-cffi/version.txt
>
> diff --git a/bindings/Makefile.local b/bindings/Makefile.local
> index bc960bbc..d5c70bff 100644
> --- a/bindings/Makefile.local
> +++ b/bindings/Makefile.local
> @@ -15,9 +15,10 @@ endif
>  
>  python-cffi-bindings: lib/$(LINKER_NAME)
>  ifeq ($(HAVE_PYTHON3_CFFI),1)
> + test '$(srcdir)' = . && bdir=. || 
> bdir='$(NOTMUCH_SRCDIR)/$(dir)/python-cffi'; \
>   cd $(dir)/python-cffi && \
> - ${PYTHON} setup.py build --build-lib build/stage && \
> - mkdir -p build/stage/tests && cp tests/*.py build/stage/tests
> + ${PYTHON} "$$bdir"/setup.py build --build-lib build/stage && \
> + mkdir -p build/stage/tests && cp "$$bdir"/tests/*.py 
> build/stage/tests
>  endif
>  
>  CLEAN += $(patsubst %,$(dir)/ruby/%, \
> diff --git a/bindings/python-cffi/notmuch2/_build.py 
> b/bindings/python-cffi/notmuch2/_build.py
> index f67b4de6..ad585d21 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -1,5 +1,9 @@
>  import cffi
>  
> +import os
> +import sys
> +
> +dn0 = os.path.dirname(sys.argv[0])
>  
>  ffibuilder = cffi.FFI()
>  ffibuilder.set_source(
> @@ -16,7 +20,7 @@ ffibuilder.set_source(
>  #ERROR libnotmuch  version < 5.1 not supported
>  #endif
>  """,
> -include_dirs=['../../lib'],
> +include_dirs=[dn0 + '/../../lib'],
>  library_dirs=['../../lib'],
>  libraries=['notmuch'],
>  )
> diff --git a/bindings/python-cffi/setup.py b/bindings/python-cffi/setup.py
> index cda52338..5884944b 100644
> --- a/bindings/python-cffi/setup.py
> +++ b/bindings/python-cffi/setup.py
> @@ -1,6 +1,11 @@
>  import setuptools
>  
> -with open('version.txt') as fp:
> +import os
> +import sys
> +
> +dn0 = os.path.dirname(sys.argv[0])
> +
> +with open(dn0 + '/../../version.txt') as fp:
>  VERSION = fp.read().strip()
>  
>  setuptools.setup(
> @@ -12,7 +17,7 @@ setuptools.setup(
>  setup_requires=['cffi>=1.0.0'],
>  install_requires=['cffi>=1.0.0'],
>  packages=setuptools.find_packages(exclude=['tests']),
> -cffi_modules=['notmuch2/_build.py:ffibuilder'],
> +cffi_modules=[dn0 + '/notmuch2/_build.py:ffibuilder'],
>  classifiers=[
>  'Development Status :: 3 - Alpha',
>  'Intended Audience :: Developers',
> diff --git a/bindings/python-cffi/version.txt 
> b/bindings/python-cffi/version.txt
> deleted file mode 100644
> index 8239f42d..
> --- a/bindings/python-cffi/version.txt
> +++ /dev/null
> @@ -1 +0,0 @@
> -0.31.3
> diff --git a/configure b/configure
> index cfa9c09b..7bdd7a13 100755
> --- a/configure
> +++ b/configure
> @@ -74,10 +74,6 @@ if [ "$srcdir" != "." ]; then
>  # Use the same hack to replicate python-cffi source for
>  # out-of-tree builds (again, not ideal).
>  mkdir 

Re: [PATCH 1/1] python/notmuch2: provide binding for database_get_directory()

2021-07-31 Thread Floris Bruynooghe
On Sat 31 Jul 2021 at 00:33 +0200, Ludovic LANGE wrote:
> (Btw, it seems your mail was sent directly to me, not the M/L, was it
> intentional or an accident ? Both are OK for me.)

No, that was accidental.  Apologies!

> While you review the new version of the patch, a few thoughts on your
> answers below:
>
> Le 27/07/2021 à 23:01, Floris Bruynooghe a écrit :
>>>>> +:raises UpgradeRequiredError: The database must be upgraded
>>>>> +   first.
>>>>> +"""
>>>>> +if not hasattr(os, 'PathLike') and isinstance(path, 
>>>>> pathlib.Path):
>>>>> +path = bytes(path)
>>>> I think `path = os.fspath(path)` should handle this fine.  But I don't
>>>> think you even need to do that as `os.fsencode()` will just do the right
>>>> thing already.
>>> This is a construct I saw multiple times in the same file (_database.py)
>>> of the notmuch2 python bindings. Not familiar with it, I guessed I
>>> should do the same also.
>>>
>>> I'm ok to remove it, but for consistency a more general approach should
>>> be taken for the rest of the ~6 times it occurs in this file. I'd be
>>> happy to have more insights on why you (as you seem to be the author of
>>> the new bindings, and of these constructs) had to put them here the
>>> first time ?
>> Heh, I guess I just didn't write that good code and read the docs better
>> this time round?  Pathlib was fairly new back then.  I think the other 6
>> times should probably change because it seems like these are the python
>> APIs to do this with and are less error prone.
>
> OK, that's fine - it's just that I don't have much experience with this
> PathLib API.
>
> I'm OK to remove this construct, no issue, will do it. I'll let you do
> the others or I can do it, as you wish.
>
>
>>>>> +class PurePathIter(FilenamesIter):
>>>>> +"""Iterator for pathlib.PurePath objects."""
>>>>> +
>>>>> +def __next__(self):
>>>>> +fname = super().__next__()
>>>>> +return pathlib.PurePath(os.fsdecode(fname))
>>>> I'm surprised you explicitly need a PurePath here?
>>> Not needed. My reasoning was : a Directory is returning real-paths
>>> (directories, files) on the filesystem, but the API is only directed
>>> towards manipulating the notmuch database, not the underlying filesystem
>>> (otherwise we would have to re-run notmuch to synch the differences). So
>>> by returning a PurePath (as opposed to a concrete Path) it could be a
>>> signal to the user not to mess with the undeerlying filesystem.
>> I like this reasoning :)
>>
>> I wonder if we can go even further and not supply the `.files()` and
>> `.directories()` functions at all.  After all there is `Directory.path`
>> and in python you can just do `[p for p in Direcotry.path.iterdir() if
>> p.is_file()]` o get the equivalent.  OTOH supplying these allows you to
>> verify that notmuch sees the same things.
>>
>> So maybe it makes sense to keep this as PurePath and keep .files() and
>> .directories() but explain in their docstring they are more advanced and
>> point users to the list comprehension instead for most uses.
>>
>> Maybe someone who knows the C API better can comment on whether this
>> seems reasonable or whether I have this completely wrong.
>
> I'm a little embarrassed removing those functions (`.files()` and
> `.directories()`):
>
> * They're part of the C API, and in my vision the python API should be a
> (maybe agnostic) wrapper around the C API, not removing functions. I'm
> OK to be a little more "Pythonic", but I'd prefer not to diverge from
> the C roots.

This I'm not too swayed by, if you want a faithful API then you should
use the notmuch._capi module directly but you have to deal with all the
hurdles.  So I think it's reasonable to interpret things in a suitable
way.

> * In this specific use-case, asking the Database its vision of the
> filesystem is what I want - because I'm dealing with mails (that are in
> some folders) indexed by the database. If there is an inconsistency
> (folders not indexed for whatever reason) between the filesystem and
> notmuch's view ; I prefer to have notmuch's view because after I'll
> query notmuch based on those folders - they're better exist in the database.

Great, I missed this.  I checked the implementation in C and indeed it
queries the database and does not do any filesystem operations.  So yes,
it does make sense to include these.

Slightly off-topic: this is why I was reluctant to work on this API
myself, I haven't used it and don't fully understand it's uses.  So I
appreciate this discussion and you taking the time to explain me this
kind of thing.

> * Also, regarding PurePath, you're initial reaction was sound, and I'm
> OK to remove PurePath and have a (valid, functional) Path instead.
> That's what I put in the second version of the patch.


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 1/1] python/notmuch2: provide binding for database_get_directory()

2021-07-25 Thread Floris Bruynooghe
Hi Ludovic,

Thanks for having a look at this!

On Sun 25 Jul 2021 at 10:16 +0200, Ludovic LANGE wrote:

> diff --git a/bindings/python-cffi/notmuch2/_database.py 
> b/bindings/python-cffi/notmuch2/_database.py
> index 868f4408..e48fa895 100644
> --- a/bindings/python-cffi/notmuch2/_database.py
> +++ b/bindings/python-cffi/notmuch2/_database.py
> @@ -13,6 +13,7 @@ import notmuch2._errors as errors
>  import notmuch2._message as message
>  import notmuch2._query as querymod
>  import notmuch2._tags as tags
> +import notmuch2._directory as directory
>  
>  
>  __all__ = ['Database', 'AtomicContext', 'DbRevision']
> @@ -338,7 +339,46 @@ class Database(base.NotmuchObject):
>  return DbRevision(rev, capi.ffi.string(raw_uuid[0]))
>  
>  def get_directory(self, path):
> -raise NotImplementedError
> +"""Returns a :class:`Directory` from the database for path,
> +
> +:param path: An unicode string containing the path relative to the 
> path
> +  of database (see :attr:`path`), or else should be an absolute
> +  path with initial components that match the path of 'database'.

Instead of a unicode string instead it should accept anything that
`os.fspath` accepts: str, bytes or os.PathLike.  This allows using
e.g. pathlib.Path as well.

> +:returns: :class:`Directory` or raises an exception.
> +:raises: :exc:`FileError` if path is not relative database or 
> absolute
> + with initial components same as database.
> +
> +
> +:raises XapianError: A Xapian exception occurred.
> +:raises LookupError: The directory object referred to by ``pathname``

path or pathname?  choose one ;)

> +   does not exist in the database.
> +:raises FileNotEmailError: The file referreed to by
> +   ``pathname`` is not recognised as an email message.

same

> +:raises UpgradeRequiredError: The database must be upgraded
> +   first.
> +"""
> +if not hasattr(os, 'PathLike') and isinstance(path, pathlib.Path):
> +path = bytes(path)

I think `path = os.fspath(path)` should handle this fine.  But I don't
think you even need to do that as `os.fsencode()` will just do the right
thing already.

> +directory_pp = capi.ffi.new('notmuch_directory_t **')
> +ret = capi.lib.notmuch_database_get_directory(
> +  self._db_p, os.fsencode(path), directory_pp)
> +
> +if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +raise errors.NotmuchError(ret)
> +
> +directory_p = directory_pp[0]
> +if directory_p == capi.ffi.NULL:
> +raise LookupError
> +
> +if os.path.isabs(path):
> +# we got an absolute path
> +abs_dirpath = path
> +else:
> +#we got a relative path, make it absolute
> +abs_dirpath = os.path.abspath(os.path.join(self.get_path(), 
> path))
> +
> +ret_dir = directory.Directory(abs_dirpath, directory_p, self)

I think the code has been trying to use pathlib for path manipulations,
so for consistency it'd be good to keep doing that  This if condition
becomes much simpler:

path = pathlib.Path(path)  # this is a no-op if it already is an instance
ret_dir = directory.Directory(path.absolute(), directory_p, self)

> +return ret_dir
>  
>  def default_indexopts(self):
>  """Returns default index options for the database.
> diff --git a/bindings/python-cffi/notmuch2/_directory.py 
> b/bindings/python-cffi/notmuch2/_directory.py
> new file mode 100644
> index ..1d48aa54
> --- /dev/null
> +++ b/bindings/python-cffi/notmuch2/_directory.py
> @@ -0,0 +1,164 @@
> +import os
> +import pathlib
> +
> +import notmuch2._base as base
> +import notmuch2._capi as capi
> +import notmuch2._errors as errors
> +from ._message import FilenamesIter
> +
> +__all__ = ["Directory"]
> +
> +
> +class PurePathIter(FilenamesIter):
> +"""Iterator for pathlib.PurePath objects."""
> +
> +def __next__(self):
> +fname = super().__next__()
> +return pathlib.PurePath(os.fsdecode(fname))

I'm surprised you explicitly need a PurePath here?


> +class Directory(base.NotmuchObject):
> +"""Represents a directory entry in the notmuch directory
> +
> +Modifying attributes of this object will modify the
> +database, not the real directory attributes.
> +
> +The Directory object is usually derived from another object
> +e.g. via :meth:`Database.get_directory`, and will automatically be
> +become invalid whenever that parent is deleted. You should
> +therefore initialized this object handing it a reference to the
> +parent, preventing the parent from automatically being garbage
> +collected.
> +"""
> +
> +_msg_p = base.MemoryPointer()
> +
> +def __init__(self, path, dir_p, parent):
> +"""
> +:param path:   The absolute path of the directory object.

For consistency I'd require this to 

Re: [PATCH 0/3] python/notmuch2: a few docstrings and collect_tags()

2021-01-11 Thread Floris Bruynooghe
On Thu 07 Jan 2021 at 17:09 +, Michael J. Gruber wrote:
> As for the series: the notmuch based MUA "alot" switched to the new
> python bindings recently. collect_tags() is something I used in a
> feature PR submitted but not merged yet there (while on the old bindings),
> and in my updated feature PR there I directly roll
> notmuch2._tags.ImmutableTagSet(msgs, '_iter_p', 
> notmuch2.capi.lib.notmuch_messages_collect_tags).

You could do this entirely in the public bindings too could you not?
Something like (untested):

def collect_tags(db, query):
tags = set()
for msg in db.messages(query):
tags.update(msg.tags)

anyway, i guess the internal APIs you use won't change before your
patchset here lands so this doesn't matter much.


> I don't know whether this will land in alot, but feature parity of the
> new bindings with the old ones is a good aim

Thanks for contributing this!  I never aimed for full parity because I
didn't feel like making the API decisions for APIs I had no need to use
myself.  But it's great when people needing things can add it!


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 3/3] python/notmuch2: provide binding for collect_tags()

2021-01-11 Thread Floris Bruynooghe
Hi Micahel,

Thanks for adding this feature!

On Wed 06 Jan 2021 at 10:08 +0100, Michael J. Gruber wrote:
> diff --git a/bindings/python-cffi/notmuch2/_query.py 
> b/bindings/python-cffi/notmuch2/_query.py
> index 1db6ec96..a1310944 100644
> --- a/bindings/python-cffi/notmuch2/_query.py
> +++ b/bindings/python-cffi/notmuch2/_query.py
> @@ -2,6 +2,7 @@ from notmuch2 import _base as base
>  from notmuch2 import _capi as capi
>  from notmuch2 import _errors as errors
>  from notmuch2 import _message as message
> +from notmuch2 import _tags as tags
>  from notmuch2 import _thread as thread
>  
>  
> @@ -66,6 +67,17 @@ class Query(base.NotmuchObject):
>  raise errors.NotmuchError(ret)
>  return count_p[0]
>  
> +def collect_tags(self):
> +"""Return the tags of messages matching this query."""
> +msgs_pp = capi.ffi.new('notmuch_messages_t**')
> +ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
> +if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +raise errors.NotmuchError(ret)
> +self._msgs_p = msgs_pp[0]
> +tagset = tags.ImmutableTagSet(
> +self, '_msgs_p', capi.lib.notmuch_messages_collect_tags)
> +return tags.ImmutableTagSet._from_iterable(tagset)
> +
>  def threads(self):
>  """Return an iterator over all the threads found by the query."""
>  threads_pp = capi.ffi.new('notmuch_threads_t **')

How this this look on the C level for the memory allocator?  If the
query gets freed the messages also get freed?  In that case indeed the
messages do need to be kept alive together with the query indeed.
Keeping them as an attribute on the query object seems reasonable I
think, they'd be freed at the same time and I don't think this creates a
circular reference.

There's only a small detail though: to get the correct ObjectDestroyed
exceptions you need Query._msgs_p to be a _base.MemoryPoiniter
descriptor, like Query._query_p is currently.  And then you also need to
set it to None in Query._destroy.

It would also be really good to add at least one happy-path test for
this, ensuring that you get the tags in the query.  You could add some
to TestQuery in test_database.py.  The database is created with unread
and inbox tags i think (see conftest.py:75) so the messages should
already have some tags which is sufficient as you're not testing
notmuch's behaviour itself but only that the bindings give you a right
looking tagset (the tagset itself also has tests already, so no need to
test this extensively, just see if you have some tags).

Otherwise LGTM!

Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: python notmuch2 bindings exclude_tags not added correctly to query

2020-12-24 Thread Floris Bruynooghe
Hi Johannes,

On Wed 23 Dec 2020 at 02:34 +0100, Johannes Larsen wrote:

> A typo in Database._create_query loses the exclude_tag names during the
> string to utf-8 conversion.
>
>
> The problem is fixed by this patch applied to current master (ced341e8):
>
> diff --git i/bindings/python-cffi/notmuch2/_database.py 
> w/bindings/python-cffi/notmuch2/_database.py
> index 5ab0f20a..868f4408 100644
> --- i/bindings/python-cffi/notmuch2/_database.py
> +++ w/bindings/python-cffi/notmuch2/_database.py
> @@ -581 +581 @@ class Database(base.NotmuchObject):
> -tag = str.encode('utf-8')
> +tag = tag.encode('utf-8')

Oops yes, someone else found this a while ago and I started working on a
patch but that ended up in some yak shaving when the tests I wrote for
it ended up accidentally unearthing other unrelated bugs and then I lost
track of this...  Apologies.

Anyway, this fix LGTM even if I do prefer fixes to come with tests :)  But that
shouldn't stop a fix from being merged I guess.

Cheers
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: Adding config cache to notmuch_database_t breaks python-cffi test

2020-12-24 Thread Floris Bruynooghe
On Sun 20 Dec 2020 at 19:22 -0400, David Bremner wrote:
Hi David,

I'm just catching up on some mailing lists...

> David Bremner  writes:
>
>> David Bremner  writes:
>>
>>> I hope Floris (or someone) can tell me what is going on here, I don't
>>> understand the memory management in the CFFI bindings very well.
>>>
>>> As part of my attempt to remodel config handling, I've added a cache
>>> for the configuration information stored in the database. This doesn't
>>> seem to break any other tests, and valgrind doesn't spot any memory
>>> errors.
>>>
>>> Here's the output from pytest
>>
>> Oh, I bet I know what it is. I need to update the cache when calling
>> notmuch_database_set_config. It's just a surprise that nothing in our
>> test suite does a read after write for configuration information in the
>> same database session.
>>
>
> Yep, that was it, sorry for the noise. I guess I kindof panicked at not
> understanding what was going on in the bindings tests.

Yes, this test is checking deletion actually works.  Glad the python
tests helped catch an issue.  :)


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: New Python bindings (notmuch2 module) fail to exclude tags

2020-11-24 Thread Floris Bruynooghe
Hi All,

On Sat 21 Nov 2020 at 18:51 -0300, Jorge P. de Morais Neto wrote:

> Hi Floris.
>
> Em [2020-11-20 sex 19:17:56+0100], Floris Bruynooghe escreveu:
>
>> Looking at the implementation I don't seem much that could have gone
>> wrong.  However I did notice the bindings fail to check the return code
>> in one call where it probably should, you could try with this patch?
>>
>> diff --git a/bindings/python-cffi/notmuch2/_database.py 
>> b/bindings/python-cffi/notmuch2/_database.py
>> index 5ab0f20a..5dbfe68e 100644
>> --- a/bindings/python-cffi/notmuch2/_database.py
>> +++ b/bindings/python-cffi/notmuch2/_database.py
>> @@ -579,7 +579,10 @@ class Database(base.NotmuchObject):
>>  for tag in exclude_tags:
>>  if isinstance(tag, str):
>>  tag = str.encode('utf-8')
>> -capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
>> +ret = capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
>> +if ret not in [capi.lib.NOTMUCH_STATUS_SUCCESS,
>> +   capi.lib.NOTMUCH_STATUS_IGNORED]:
>> +raise errors.NotmuchError(ret)
>>  return querymod.Query(self, query_p)
>>  
>>  def messages(self, query, *,
>>
> After applying your patch, the call to nm_db.count_messages() fails:
> AttributeError: cffi library 'notmuch2._capi' has no function, constant 
> or global variable named 'NOTMUCH_STATUS_IGNORED'
>
> However, I then found the bug.  The patch below fixes it.
>
> $ diff -u notmuch2-orig/_database.py notmuch2/_database.py
> --- notmuch2-orig/_database.py2020-11-21 18:02:17.560240619 -0300
> +++ notmuch2/_database.py 2020-11-21 18:43:44.827879141 -0300
> @@ -578,7 +578,7 @@
>  if exclude_tags is not None:
>  for tag in exclude_tags:
>  if isinstance(tag, str):
> -tag = str.encode('utf-8')
> +tag = tag.encode('utf-8')
>  capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
>  return querymod.Query(self, query_p)
>
>
> However, I think you should *also* add the error checking.  The only
> reason my patch omits error checking is that I don't know how to define
> capi.lib.NOTMUCH_STATUS_IGNORED.

Yup, that was missing from _build.py so wouldn't have been available.

Before fixing this I tried to write a test, but I don't know if this
behaves correctly:

I have a database with 3 messages organised in 2 threads and the
following tags:

msg1: all
  +- msg3: all, spam
msg2: all

I query '*', so all messages with exclude_tags=['spam'].  Querying this
with various flags gives me:

NOTMUCH_EXCLUDE_TRUE: 2 messages, what I expect
NOTMUCH_EXCLUDE_ALL: 2 messages, what I expect
NOTMUCH_EXCLUDE_FLAG: 2 messages, I expected 3
NOTMUCH_EXCLUDE_FLASE: 2 messages, I expected 3

Did I misunderstand the docs?  Are these results correct?


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: notmuch2 (python cffi bindings) segfault gdb logs

2020-11-24 Thread Floris Bruynooghe
Hi Patrick,

On Mon 23 Nov 2020 at 10:36 +, Patrick Totzke wrote:
> I've been complaining about the new (and old) python bindings causing the 
> python interpreter to segfault occasionally. So far I was not able to 
> reproduce this reliably nor provide error traces. This has just changed:
> see below and attached for what I got from gdb.

Your gdb info doesn't say explicitly (or I missed it), but this is
showing a SEGFAULT I guess?

> I hope that whoever is in charge of the bindings can make sense of
> it. I don't have any experience so far with cffi nor gdb and have a
> hard time debugging this. The logs below are my attempt to collect as
> much detail as possible about. Please let me know if I missed
> something.

>From what I can tell we're calling a function to free something which
segfaults, so it probably was freed already and we didn't know.  We need
to find out who freed it before and why we thought it still needed to be
freed.

> (gdb) info threads
>   Id   Target Id Frame 
> * 1Thread 0x77c0e740 (LWP 3614451) "python3" __GI_raise 
> (sig=sig@entry=6)
> at ../sysdeps/unix/sysv/linux/raise.c:50

>From this I gather we only have one thread, could you confirm this?
notmuch2 just isn't thread safe at the moment (I forget whether this was
intentional or by accident, might have been intentional depending on how
threadsafe libnotmuch is).

> Traceback (most recent call first):
>0x7636f040>
>   File "/home/pazz/.local/lib/python3.8/site-packages/notmuch2/_thread.py", 
> line 38, in _destroy
> capi.lib.notmuch_thread_destroy(self._thread_p)
>   File "/home/pazz/.local/lib/python3.8/site-packages/notmuch2/_thread.py", 
> line 34, in __del__
> self._destroy()
>   File "/home/pazz/projects/alot/alot/db/manager.py", line 570, in get_threads
>   

I pulled alot master and this does not match at all.  Could you tell me
which git ref this was using so I can try and see what alot is actually
doing?  (or some other way of sharing the source in this backtrace)



Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


[PATCH] python-cffi: Extract cdefs from notmuch.h

2020-11-22 Thread Floris Bruynooghe
Instead of having all the required cdefs copied in _build.py this uses
the C pre-compiler to extract them from notmuch.h itself.  To do this
we need to indroduce a few more #ifdefs in the header file since the
CFFI parser can not deal with everything the real C parser can deal
with, plus it does not want to see symbols not from notmuch.

The main downside is that we always compile in all symbols of the
library, not just those used.  Furthermore to avoid having lots of
compiler warnings about deprecated symbols this excludes all unused
deprecated functions using even more #ifdefs.

To allow this to work this also makes sure that when using the
Makefile to build notmuch the python bindings find and use the headers
and library from the build directory, before it would still link
against the system-wide libnotmuch.so.5.  I have not tested this with
out-of tree builds.
---
 .gitignore |   2 +
 bindings/Makefile.local|   8 +-
 bindings/python-cffi/README|  63 
 bindings/python-cffi/notmuch2/_build.py| 359 +++--
 bindings/python-cffi/pip-editable-build.sh |  16 +
 lib/notmuch.h  |  20 +-
 6 files changed, 159 insertions(+), 309 deletions(-)
 create mode 100644 bindings/python-cffi/README
 create mode 100755 bindings/python-cffi/pip-editable-build.sh

diff --git a/.gitignore b/.gitignore
index 3edd1768..ddc0f650 100644
--- a/.gitignore
+++ b/.gitignore
@@ -8,6 +8,8 @@
 /.stamps
 /Makefile.config
 /bindings/python-cffi/build/
+/bindings/python-cffi/notmuch2/*.c
+/bindings/python-cffi/notmuch2/*.so
 /lib/libnotmuch*.dylib
 /lib/libnotmuch.so*
 /notmuch
diff --git a/bindings/Makefile.local b/bindings/Makefile.local
index bc960bbc..7523a0b5 100644
--- a/bindings/Makefile.local
+++ b/bindings/Makefile.local
@@ -16,7 +16,13 @@ endif
 python-cffi-bindings: lib/$(LINKER_NAME)
 ifeq ($(HAVE_PYTHON3_CFFI),1)
cd $(dir)/python-cffi && \
-   ${PYTHON} setup.py build --build-lib build/stage && \
+   ${PYTHON} setup.py build_ext \
+--build-lib build/stage \
+--include-dirs ../../lib \
+--library-dirs ../../lib \
+--rpath $(CURDIR)/lib  && \
+   ${PYTHON} setup.py build \
+--build-lib build/stage && \
mkdir -p build/stage/tests && cp tests/*.py build/stage/tests
 endif
 
diff --git a/bindings/python-cffi/README b/bindings/python-cffi/README
new file mode 100644
index ..2f20e17f
--- /dev/null
+++ b/bindings/python-cffi/README
@@ -0,0 +1,63 @@
+notmuch2 - python3 compatible bindings to notmuch
+
+These bindings use CFFI to generate the bindings making it work well
+with both CPython and PyPy.  They try to ensure all operations
+from within Python are safe and handle the memory management themselves.
+
+Building notmuch2
+-
+
+The provided setup.py file can be used to build and install the
+bindings.  You will need python3, setuptools and cffi installed for
+this to work.
+
+By default this will assume that the C compiler can find both
+notmuch.h and libnotmuch.so.5, if these are installed in a system-wide
+location they will normally be found there.  When building notmuch
+with make it will ensure that the C compiler finds the in-tree version
+of notmuch instead of any system-wide one which might be available.
+However the resulting python module is not relocatable and is mainly
+used by the notmuch test suite.  For installing your own notmuch
+python package it is recommended to first install libnotmuch system
+wide and than use setup.py to build and install the python package.
+
+If you really need to customise your build environment, have a look at
+the INCUDE_DIRS, LIBRARY_DIRS and EXTRA_LINK_ARGS variables in
+notmuch2/_build.py.
+
+Documentation
+-
+
+The package has extensive docstrings, these docs are also included in
+the general notmuch sphinx documentation.  See the notmuch `doc/`
+subdirectory for this.
+
+
+Development
+---
+
+An easy way to work on the bindings themselves is to create a
+virtualenv to work in.  Install the python bindings in editable mode
+using:
+
+$ cd bindings/python-cffi
+$ pip install -e .
+
+However as described above this will build against the system-wide
+notmuch installation.  If you need to work on the currently developed
+libnotmuch you can invoke pip with some extra --global-option
+arguments.  This is rather verbose, so the pip-editable-build.sh
+script does this for you:
+
+$ make
+$ cd bindings/python-cffi
+$ ./pip-editable-build.sh
+
+Tests are written using pytest and can be run using:
+
+$ pip install pytest pytest-cov
+$ pytest
+
+Individual tests can be selected:
+
+$ pytest tests/test_database.py::TestCreate::test_create
\ No newline at end of file
diff --git a/bindings/python-cffi/notmuch2/_build.py 
b/bindings/python-cffi/notmuch2/_build.py
index 

python-cffi: experiment with extracting cdefs from notmuch.h

2020-11-22 Thread Floris Bruynooghe
Hi,

This is mostly an experiment because I got tired of copying the
C function defs from notmuch.h (in other words this was yak shaving
while I intended to see if I could get notmuch_database_get_directory
supported).  I'm not convinced this is better that what we had before
to be honest, but am curious what other people think of this approach.
In deltachat we do actually use this mechanism and it works nicely,
but there we don't have to deal with deprecated functions which is
what makes this ugly here.

Independenly maybe the changes to the makefile and README are useful
anyway.  I don't think the way this was build before always used the
right lib version, I think for running T391 we probably want to
compile with an RPATH.

Cheers,
Floris

___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: New Python bindings (notmuch2 module) fail to exclude tags

2020-11-20 Thread Floris Bruynooghe
Hi Jorge,

On Fri 20 Nov 2020 at 12:54 -0300, Jorge P. de Morais Neto wrote:

> Hi.  I am trying to migrate my Python3 script to the new Python bindings
> (notmuch2 module).  However, I cannot obtain a count of messages
> matching a query excluding messages that have an exclude tag.  From the
> command line:
> $ notmuch count 'is:.bf_spam'
> 0
>
> The CLI command correctly counts zero messages having '.bf_spam' tag,
> because all such messages also have the excluded 'spam' tag.  But from
> Python:
>
> $ python3
> Python 3.7.3 (default, Jul 25 2020, 13:03:44) 
> [GCC 8.3.0] on linux
> Type "help", "copyright", "credits" or "license" for more information.
 import notmuch2
 nm_db=notmuch2.Database()
 nm_db.count_messages('is:.bf_spam', exclude_tags=('spam',))
> 379
 nm_db.count_messages('is:.bf_spam', exclude_tags=('spam',), 
 omit_excluded=nm_db.EXCLUDE.FALSE)
> 379
 nm_db.count_messages('is:.bf_spam', exclude_tags=('spam',), 
 omit_excluded=nm_db.EXCLUDE.TRUE)
> 379
 nm_db.count_messages('is:.bf_spam', exclude_tags=('spam',), 
 omit_excluded=nm_db.EXCLUDE.ALL)
> 379
 nm_db.count_messages('is:.bf_spam', exclude_tags=('spam',), 
 omit_excluded=nm_db.EXCLUDE.FLAG)
> 379

Personally I find the description of these flags in notmuch.h not very
clear and don't claim to understand them (this probably explains the
really bad docstring, we should improve those).  But I expected to at
least see the EXCLUDE.TRUE one, the default, to work.

Looking at the implementation I don't seem much that could have gone
wrong.  However I did notice the bindings fail to check the return code
in one call where it probably should, you could try with this patch?

diff --git a/bindings/python-cffi/notmuch2/_database.py 
b/bindings/python-cffi/notmuch2/_database.py
index 5ab0f20a..5dbfe68e 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -579,7 +579,10 @@ class Database(base.NotmuchObject):
 for tag in exclude_tags:
 if isinstance(tag, str):
 tag = str.encode('utf-8')
-capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
+ret = capi.lib.notmuch_query_add_tag_exclude(query_p, tag)
+if ret not in [capi.lib.NOTMUCH_STATUS_SUCCESS,
+   capi.lib.NOTMUCH_STATUS_IGNORED]:
+raise errors.NotmuchError(ret)
 return querymod.Query(self, query_p)
 
 def messages(self, query, *,

To experiment you could also raise the exception for
NOTMUCH_STATUS_IGNORED, even though for the bindings to silently swallow
this is probably the best (this is really the bindings being more
limiting than the C lib, if this bothers ppl we could add another
keyword arg to let this raise as well).

While the above patch is probably good, I feel like it's clutching at
straws and unlikely to solve your problem.  Could you try to reproduce
this in plain C or maybe even in pretend-plain-c by using the
capi.lib.notmuch_* calls directly?


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: Python3 cffi bindings

2020-10-14 Thread Floris Bruynooghe
Hi Gaute,

On Thu 08 Oct 2020 at 10:13 +0200, Gaute Hope wrote:
> I made another attempt at porting lieer to notmuch2, but I am missing the
> get_directory method still. Any plans to look at it?

Would indeed be good to add this sometime.  I'm still curious to how you
use it though to make sure we make a good API.  I only found
https://github.com/gauteh/lieer/blob/394d8c1a574fd57e63390e92a6e73363808ebac5/lieer/local.py#L280
and it seems you only use the `.path` attribute.  Is this correct or did
I miss anything?

Cheers,
Floris

>
> Regards, Gaute
>
> On Sun, Nov 17, 2019 at 6:14 PM Floris Bruynooghe  wrote:
>
>> Hi Gaute,
>>
>> Thanks for trying this out!
>>
>> On Mon 04 Nov 2019 at 11:27 +0100, Gaute Hope wrote:
>> > I just checked out the wip/cffi branch on git.notmuch.org with the
>> > purpose of porting Lieer (https://github.com/gauteh/lieer). There
>> > seems to be some missing functionality: `Database.get_directory()`
>> > specifically.
>>
>> Yeah, I didn't add that yet because I don't fully understand how it
>> should be used.  Specifically I don't know where one might get a
>> pathname from to pass to .get_directory() and thus whether the API would
>> be cleaner to just return a reasonable directory object from whatever
>> location that might be.  Maybe notmuch_database_get_path() is the only
>> entrypoint here and you can get further by listing files and directories
>> from it?  But maybe people then use the filesystem directly to find a
>> directory and create the directories ad-hoc.
>>
>> I grepped lieer but I think you only use it in one place?  And if I
>> understand it correctly you only do this to check if your mailstore/cwd
>> is inside the notmuch database.  I.e. this is equivalent to checking if
>> your mailstore/cwd has notmuch2.Database.path as prefix which you could
>> easily do directly rather than using the FileError exception from
>> .get_directory().
>>
>> So is anyone else aware of some code which uses db.get_directory() to
>> give an idea of how and why this is used?
>>
>> > I also ran into a couple of warning when building
>> > (included below).
>>
>> Thanks for pointing these out.  I guess if the bindings are in the main
>> repo only the latest library version can be supported without any
>> further concerns.
>>
>> > By the way, it does not seem that the API is very far from the
>> > previous python API. If it is close enough, perhaps it is possible to
>> > get away with a bug version bump in the bindings rather than creating
>> > a new package. I understand the need for a new package, but it would
>> > be nice if we could avoid the future confusion of two python binding
>> > packages (if at all possible).
>>
>> While I'm glad to hear that you think a migration wouldn't be to painful
>> for you I am very weary of knowingly breaking APIs.  I'd rather have
>> people have an easy migration rather than unexpected breakage after an
>> upgrade.
>>
>>
>> Cheers,
>> Floris
>>
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 1/2] doc: replace use of environment variables with a generated config

2020-07-14 Thread Floris Bruynooghe
On Sat 11 Jul 2020 at 17:00 +0300, Tomi Ollila wrote:

> On Sat, Jul 11 2020, David Bremner wrote:
>
>> I don't love the use of exec, but it is getting unwieldy to pass
>> configuration options on the sphinx-build command line, and I
>> anticipate further use of conditionals.
>
> Perhaps less "opinions" in commit message.
>
> (and as I think I don't comment 2/2, s/seperate/separate/ there)
>
>
>> ---
>>  configure  |  8 
>>  doc/Makefile.local |  2 +-
>>  doc/conf.py| 11 ---
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 80cbac4f..177432db 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1548,6 +1548,14 @@ NOTMUCH_HAVE_PYTHON3_PYTEST=${have_python3_pytest}
>>  PLATFORM=${platform}
>>  EOF
>>  
>> +cat > sphinx.config <> +# Generate by configure, run from doc/conf.py
>> +EOF
>> +if [ $WITH_EMACS = "1" ]; then
>> +printf "tags.add('WITH_EMACS')\n" >> sphinx.config
>> +fi
>> +printf "rsti_dir = '%s'\n" $(realpath emacs) >> sphinx.config
>> +
>
> perhaps instead of multiple redirections to the file, 
>
> {
> echo "# Generated by configure, run from doc/conf.py"
> echo
> if [ $WITH_EMACS = "1" ]; then
> printf "tags.add('WITH_EMACS')\n"
> fi 
> printf "rsti_dir = '%s'\n" "$(realpath emacs)"
>
> } > sphinx.config
> 
> alternative (someone might think less readable... ;/):
>
>  exec 3>&1 1> sphinx.config
>
>  echo "# Generated by configure, run from doc/conf.py"
>  ...
>
>  exec 1>&3 3>&-

I'd probably prefer this last one, but I don't mind any of the solutions or
even the current one.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH] doc: set up for autoapi / readthedocs compatibility

2020-07-14 Thread Floris Bruynooghe
On Sun 12 Jul 2020 at 09:02 -0300, David Bremner wrote:

> sphinx-autoapi seems nicer conceptually (it parses the docs rather
> than importing them),

TIL about sphinx-autoapi, agree it's nicer conceptually.

> but it also generates a ton of warnings, so
> leave the default as autodoc.
> ---
>
> You can see the results of this (for now) at 
> https://notmuch.readthedocs.io/en/rtdtest/
> We'd presumable want to integrate the whole tree somehow into notmuchmail.org

Saldy it seems to struggle with a fair bit of things.  E.g. it does
manage to create a Database.MODE attribute, but it doesn't figure out
that this is the Mode enum and thus doesn't document the fact you have
two options: MODE.READ_ONLY and MODE.READ_WRITE.  There are obviously a
bunch of other enums where this matters.

It could be that these things are solvable by using more autodoc-style
directives:
https://sphinx-autoapi.readthedocs.io/en/latest/reference/directives.html

But I wonder if the autodoc one will always be better because of the
dynamic nature.  E.g. mapping notmuch2._database.Mode to
notmuch2.Database.MODE on the actual Public API.

I didn't actually try out how much better autodoc does, I should
probably try that too before commenting further.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/2] doc: add new python bindings to main documentatation tree.

2020-07-14 Thread Floris Bruynooghe
Oh, this is very nice!  I've been thinking for a while I should attempt
this.  Great to see it being done!

On Sat 11 Jul 2020 at 10:20 -0300, David Bremner wrote:

> A seperate conf.py and doc directory will be needed if someone wants
> to build the bindings docs separately from notmuch.
> ---
>  configure   | 4 
>  doc/conf.py | 8 
>  doc/index.rst   | 1 +
>  doc/python-bindings.rst | 5 +
>  4 files changed, 18 insertions(+)
>  create mode 100644 doc/python-bindings.rst
>
> diff --git a/configure b/configure
> index 177432db..36fe4a9d 100755
> --- a/configure
> +++ b/configure
> @@ -801,6 +801,7 @@ if [ $have_python3 -eq 1 ]; then
>  if "$python" -c 'import cffi,setuptools; cffi.FFI().verify()' >/dev/null 
> 2>&1; then
>  printf "Yes.\n"
>  have_python3_cffi=1
> +WITH_PYTHON_DOCS=1
>  else
>  printf "No (will not install CFFI-based python bindings).\n"
>  fi
> @@ -1554,6 +1555,9 @@ EOF
>  if [ $WITH_EMACS = "1" ]; then
>  printf "tags.add('WITH_EMACS')\n" >> sphinx.config
>  fi
> +if [ $WITH_PYTHON_DOCS = "1" ]; then
> +printf "tags.add('WITH_PYTHON')\n" >> sphinx.config
> +fi
>  printf "rsti_dir = '%s'\n" $(realpath emacs) >> sphinx.config
>  
>  # Finally, after everything configured, inform the user how to continue.
> diff --git a/doc/conf.py b/doc/conf.py
> index fdff2a2c..94e266af 100644
> --- a/doc/conf.py
> +++ b/doc/conf.py
> @@ -4,6 +4,8 @@
>  import sys
>  import os
>  
> +extensions = [ 'sphinx.ext.autodoc' ]
> +
>  # The suffix of source filenames.
>  source_suffix = '.rst'
>  
> @@ -22,6 +24,9 @@ for pathdir in ['.', '..']:
>  with open(version_file,'r') as infile:
>  version=infile.read().replace('\n','')
>  
> +# for autodoc
> +sys.path.insert(0, os.path.join(location, '..', 'bindings', 'python-cffi', 
> 'notmuch2'))
> +
>  # read generated config
>  for pathdir in ['.', '..']:
>  conf_file = os.path.join(location,pathdir,'sphinx.config')
> @@ -50,6 +55,9 @@ else:
>  # the docstring include files
>  exclude_patterns.append('notmuch-emacs.rst')
>  
> +if not tags.has('WITH_PYTHON'):
> +exclude_patterns.append('python-bindings.rst')
> +
>  # The name of the Pygments (syntax highlighting) style to use.
>  pygments_style = 'sphinx'
>  
> diff --git a/doc/index.rst b/doc/index.rst
> index 4440d93a..a3bf3480 100644
> --- a/doc/index.rst
> +++ b/doc/index.rst
> @@ -26,6 +26,7 @@ Contents:
> man7/notmuch-search-terms
> man1/notmuch-show
> man1/notmuch-tag
> +   python-bindings
>  
>  Indices and tables
>  ==
> diff --git a/doc/python-bindings.rst b/doc/python-bindings.rst
> new file mode 100644
> index ..e1ad26ad
> --- /dev/null
> +++ b/doc/python-bindings.rst
> @@ -0,0 +1,5 @@
> +Python Bindings
> +===
> +
> +.. automodule:: notmuch2
> +   :members:
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 1/2] doc: replace use of environment variables with a generated config

2020-07-14 Thread Floris Bruynooghe
On Sat 11 Jul 2020 at 10:20 -0300, David Bremner wrote:

> I don't love the use of exec, but it is getting unwieldy to pass

It's already a config file written in Python which is a terrible sin.
So no need to apologise, I think it makes sense in this context.

> configuration options on the sphinx-build command line, and I
> anticipate further use of conditionals.
> ---
>  configure  |  8 
>  doc/Makefile.local |  2 +-
>  doc/conf.py| 11 ---
>  3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/configure b/configure
> index 80cbac4f..177432db 100755
> --- a/configure
> +++ b/configure
> @@ -1548,6 +1548,14 @@ NOTMUCH_HAVE_PYTHON3_PYTEST=${have_python3_pytest}
>  PLATFORM=${platform}
>  EOF
>  
> +cat > sphinx.config < +# Generate by configure, run from doc/conf.py

Minor insignificant typo: Generated

> +EOF
> +if [ $WITH_EMACS = "1" ]; then
> +printf "tags.add('WITH_EMACS')\n" >> sphinx.config
> +fi
> +printf "rsti_dir = '%s'\n" $(realpath emacs) >> sphinx.config
> +
>  # Finally, after everything configured, inform the user how to continue.
>  cat <  
> diff --git a/doc/Makefile.local b/doc/Makefile.local
> index 769438ed..598b6028 100644
> --- a/doc/Makefile.local
> +++ b/doc/Makefile.local
> @@ -4,7 +4,7 @@ dir := doc
>  
>  # You can set these variables from the command line.
>  SPHINXOPTS:= -q
> -SPHINXBUILD   = WITH_EMACS=${WITH_EMACS} RSTI_DIR=$(realpath emacs) 
> sphinx-build
> +SPHINXBUILD   = sphinx-build
>  DOCBUILDDIR  := $(dir)/_build
>  
>  # Internal variables.
> diff --git a/doc/conf.py b/doc/conf.py
> index 70987ac5..fdff2a2c 100644
> --- a/doc/conf.py
> +++ b/doc/conf.py
> @@ -22,6 +22,13 @@ for pathdir in ['.', '..']:
>  with open(version_file,'r') as infile:
>  version=infile.read().replace('\n','')
>  
> +# read generated config
> +for pathdir in ['.', '..']:
> +conf_file = os.path.join(location,pathdir,'sphinx.config')
> +if os.path.exists(conf_file):
> +with open(conf_file,'r') as infile:
> +exec(''.join(infile.readlines()))
> +
>  # The full version, including alpha/beta/rc tags.
>  release = version
>  
> @@ -29,12 +36,10 @@ release = version
>  # directories to ignore when looking for source files.
>  exclude_patterns = ['_build']
>  
> -if os.environ.get('WITH_EMACS') == '1':
> +if tags.has('WITH_EMACS'):
>  # Hacky reimplementation of include to workaround limitations of
>  # sphinx-doc
>  lines = ['.. include:: /../emacs/rstdoc.rsti\n\n'] # in the source tree
> -rsti_dir = os.environ.get('RSTI_DIR')
> -# the other files are from the build tree
>  for file in ('notmuch.rsti', 'notmuch-lib.rsti', 'notmuch-show.rsti', 
> 'notmuch-tag.rsti'):
>  lines.extend(open(rsti_dir+'/'+file))
>  rst_epilog = ''.join(lines)
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [RFC PATCH] lib: document new database_open API

2020-07-08 Thread Floris Bruynooghe
On Sat 04 Jul 2020 at 14:01 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>>> + *
>>> + * - in the environment variable NOTMUCH_DATABASE, if non-empty
>>> + *
>>> + * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME
>>> + *   defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to
>>> + *   "default"
>>
>> I like the profile support, is the plan for
>> $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE to be written when creating the
>> database?
>
> Yes, with "NOTMUCH_PROFILE=default" by default.
>
>> It's nice that the environment variable handling is done in the library,
>> should make it more consistent for bindings.  As long as it can be
>> overwritten I guess.
>
> Overwritten how? By passing parameters?

yes, that's what I meant.  Which I think you design here allows.  Just
takes a while to figure out what the right parameter combination
is... ;)

>> The API is rather complex though, perhaps easier when split across
>> several functions?  Maybe a notmuch_database_open_profile(const char
>> *profile, notmuch_database_t**) is useful as the simple one which always
>> does the right thing when called with NULL for profile.  Not sure what
>> other combinations would be needed.
>
> I have no objections to a "do the write thing" wrapper or two. I don't
> think that increases maintence cost too much.
>
> d
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-08 Thread Floris Bruynooghe
On Sun 05 Jul 2020 at 08:17 -0300, David Bremner wrote:

> David Bremner  writes:
>
>> Floris Bruynooghe  writes:
>>
>>> notmuch_database_get_version currently returns and unsigned int and
>>> segfaults on use with a closed db.
>>
>> Yes, the ones without a proper status value are going to be a bit work.
>>
>> In the next series I just posted [1], I started providing status value
>> returning version (see notmuch_message_get_flag_st). We've been through
>> a few of these migrations and it has not been too painful.
>>
>
> I thought of another variation for the boolean valued functions. We
> could embed the boolean values in the notmuch_status_t value by adding
> one or more new status values corresponding to TRUE and FALSE. I'm not
> sure if that would be much simpler, but it would avoid the use of output
> parameters.

This also seems very reasonable.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-08 Thread Floris Bruynooghe
On Sat 04 Jul 2020 at 14:17 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>
>>> - * This function will not return NULL since Notmuch ensures that every
>>> - * message has a unique message ID, (Notmuch will generate an ID for a
>>> - * message if the original file does not contain one).
>>> + * This function will return NULL if triggers an unhandled Xapian
>>> + * exception.
>
>> How much of a departure from the existing API is this?  Will this be
>> possible with all functions?  I had a quick look and tried some other
>> functions that don't return notmuch_status_t:
>
> It's upward compatible in that any code which crashes because it was not
> expecting a NULL pointer, will already be crashing in the same
> circumstances because of an uncaught exception / call to abort.

Oh yes, that is a very good point.  This choice seems very reason then.


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [RFC PATCH] lib: document new database_open API

2020-07-04 Thread Floris Bruynooghe
On Fri 03 Jul 2020 at 10:43 -0300, David Bremner wrote:

> Several aspects of this are potentially controversial:
>
> 1) The use of environment variables as fallback. I understand the
> discomfort with having a library function check the environment, but
> this seems to be functionality people want, and it is better to
> implement it once.
>
> 2) The use of both NULL and "" to do different things for config_path.
>
> 3) The new API is pretty complex, compared to the previous one.
> ---
>
> There is not much code to back this so far. This is just me thinking
> out loud at this point.  The location calculation is done (and also
> easy).  The challenging part is probably updating
> notmuch_database_get_config to do what this comment promises.
>
> I suspect database_create will probably need to be updated to match.
>
> Another question is if we should have an opaque set of options to pass
> to open (in the style notmuch_indexopts_t).  Currently this is just
> future proofing as far as I know.
>
>  lib/notmuch.h | 138 --
>  1 file changed, 111 insertions(+), 27 deletions(-)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index ceb5a018..6a46b80a 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -301,52 +301,136 @@ typedef enum {
>  } notmuch_database_mode_t;
>  
>  /**
> - * Open an existing notmuch database located at 'path'.
> + * Deprecated alias for notmuch_database_open_with_config with
> + * config_path=error_message=NULL
> + * @deprecated Deprecated as of libnotmuch 5.2 (notmuch 0.31)
> + */
> +NOTMUCH_DEPRECATED(5, 2)
> +notmuch_status_t
> +notmuch_database_open (const char *path,
> +notmuch_database_mode_t mode,
> +notmuch_database_t **database);
> +/**
> + * Deprecated alias for notmuch_database_open_with_config with
> + * config_path=NULL
> + *
> + * @deprecated Deprecated as of libnotmuch 5.2 (notmuch 0.31)
> + *
> + */
> +NOTMUCH_DEPRECATED(5, 2)
> +notmuch_status_t
> +notmuch_database_open_verbose (const char *path,
> +notmuch_database_mode_t mode,
> +notmuch_database_t **database,
> +char **error_message);
> +
> +/**
> + * Open an existing notmuch database located at 'database_path', using
> + * configuration in 'config_path'.
> + *
> + * @param[in]database_path
> + * @parblock
> + * Path to existing database.
> + *
> + * A notmuch database is a Xapian database containing appropriate
> + * metadata.
>   *
>   * The database should have been created at some time in the past,
>   * (not necessarily by this process), by calling
> - * notmuch_database_create with 'path'. By default the database should be
> - * opened for reading only. In order to write to the database you need to
> - * pass the NOTMUCH_DATABASE_MODE_READ_WRITE mode.
> + * notmuch_database_create.
> + *
> + * If 'database_path' is NULL, use the location specified
> + *
> + * - in the environment variable NOTMUCH_DATABASE, if non-empty
> + *
> + * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME
> + *   defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to
> + *   "default"

I like the profile support, is the plan for
$XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE to be written when creating the
database?

> + *
> + * If 'database_path' is non-NULL, but does not appear to be a Xapian
> + * database, check for a directory '.notmuch/xapian' below
> + * 'database_path'.
> + *
> + * @endparblock
> + * @param[in]mode
> + * @parblock
> + * Mode to open database
> + *
> + * By default the database will be opened for reading only. In order
> + * to write to the database you need to pass the
> + * #NOTMUCH_DATABASE_MODE_READ_WRITE mode.
> + *
> + * @endparblock
> + * @param[in]  config_path
> + * @parblock
> + * Path to config file.
> + *
> + * Config file is key-value, with mandatory sections. See
> + * notmuch-config(5) for more information. The key-value pair
> + * overrides the corresponding configuration data stored in the
> + * database (see notmuch_database_get_config)
>   *
> - * An existing notmuch database can be identified by the presence of a
> - * directory named ".notmuch" below 'path'.
> + * If config_path is NULL use the path specified
> + *
> + * - in environment variable NOTMUCH_CONFIG, if non-empty
> + *
> + * - by  XDG_CONFIG_HOME/notmuch/ where
> + *   XDG_CONFIG_HOME defaults to "$HOME/.config".
> + *
> + * - by $HOME/.notmuch-config
> + *
> + * If config_path is "" (empty string) then do not
> + * open any configuration file.
> + * @endparblock
> + * @param[in] profile:
> + * @parblock
> + * Name of profile (configuration/database variant).
> + *
> + * If non-NULL, append to the directory / file path determined by
> + * config_path.
> + *
> + * If NULL then use
> + * - environment variable NOTMUCH_PROFILE if defined,
> + * - otherwise "default" for directories and "" (empty string) for paths.
> + *
> + * 

Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-04 Thread Floris Bruynooghe
Nice.

On Mon 29 Jun 2020 at 22:14 -0300, David Bremner wrote:
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index ceb5a018..0dc89547 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -1363,9 +1363,8 @@ notmuch_message_get_database (const notmuch_message_t 
> *message);
>   * message is valid, (which is until the query from which it derived
>   * is destroyed).
>   *
> - * This function will not return NULL since Notmuch ensures that every
> - * message has a unique message ID, (Notmuch will generate an ID for a
> - * message if the original file does not contain one).
> + * This function will return NULL if triggers an unhandled Xapian
> + * exception.
>   */
>  const char *
>  notmuch_message_get_message_id (notmuch_message_t *message);

How much of a departure from the existing API is this?  Will this be
possible with all functions?  I had a quick look and tried some other
functions that don't return notmuch_status_t:

notmuch_database_get_version currently returns and unsigned int and
segfaults on use with a closed db.

notmuch_needs_upgrade returns notmuch_bool_t and seems to return a valid
bool with a closed db.  I'm not sure if this is always the right answer
though.

I wonder if a backwards-compatible errno-style API could work,
notmuch_last_status(notmuch_database_t* database) or so.  This kind of
thing is probably easy to adopt in bindings but harder for direct users
of the API.  It's also an extra API call for everything that doesn't
return notmuch_status_t.  But I'll leave the judgement to you, I'm not
as experienced with the API.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: Usage after database close

2020-06-29 Thread Floris Bruynooghe
On Sun 28 Jun 2020 at 19:11 -0300, David Bremner wrote:
> You need to add a seperate repo for the new style debug symbols in
> Debian:
> $ (git)-[master]-% apt policy libxapian30-dbgsym
> libxapian30-dbgsym:
>   Installed: 1.4.15-1
>   Candidate: 1.4.15-1
>   Version table:
>  *** 1.4.15-1 500
> 500 http://debug.mirrors.debian.org/debian-debug testing-debug/main 
> amd64 Packages
> 500 http://debug.mirrors.debian.org/debian-debug unstable-debug/main 
> amd64 Packages
> 100 /var/lib/dpkg/status

My goodness, I completely missed the dbgsym memo.  Thanks!
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 2/2] bindings/python-cffi: update version from global version.

2020-06-29 Thread Floris Bruynooghe
On Thu 25 Jun 2020 at 10:34 -0300, David Bremner wrote:

> David Bremner  writes:
>
>> Copy machinery from the older python bindings
>
>>  
>> +# get the notmuch version number without importing the notmuch module
>> +version_file = os.path.join(os.path.dirname(__file__),
>> +'notmuch2', 'version.py')
>> +exec(compile(open(version_file).read(), version_file, 'exec'))
>> +assert '__VERSION__' in globals(), \
>> +'Failed to read the notmuch binding version number'
>
> I wrote a cover letter for this, but that seems to have gotten lost. My
> main point was I'm not sure why this is better than Floris's version,
> since they both read a file when setup.py is run. I don't understand (or
> use) pip, so someone else will have to figure this out. If the
> constraint is that the version has to be hardcoded in setup.py then (as
> much as that sounds like a design mistake), we can apply similar sed
> hackery directly to setup.py. Perhaps someone can remember why we didn't
> do that for the old python bindings.

For some reason this is the only mail in this thread I have, so I don't
actually know the patch.

I think it can be simpler though, is it possible to copy the toplevel
version file into bindings/python-cffi/version in the part of the build
that would otherwise do the sed magic?  Then setup.py only needs to look
for the version file in the same directory as itself instead of finding
the toplevel of the repo.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Usage after database close

2020-06-28 Thread Floris Bruynooghe
On Sun 28 Jun 2020 at 13:19 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>> Hi,
>>
>> I started writing some test cases to define better what you can do with
>> a closed database and make sure that the python bindings do not behave
>> unexpectedly here too.
>>
>> One of the first things I tried ends up with xapian calling
>> exit_group(2) directly, terminating the process.  So I'm wondering if
>> I'm approaching this entirely the wrong way.  My understanding is that
>> we should generally be allowed to use anything after the database has
>> been closed, as long as nothing has been destroyed.
>>
>> Below is a minimal reproducible example of what I'm testing so far.  I
>> must admit I'm generally lazy here and usually just test with notmuch
>> that is currently in Debian testing.
>
> Funny that you should mention lazy, that's basically what the problem is
> here ;). notmuch_message_get_message_id is lazily trying to read the
> information from the database. This is a bit surprising here because of
> the query, but that's not really visible once the message object is
> created.
>
> In principle it could be documented what parts of the API can trigger
> access to the database, but I'm not sure about the benefit of the extra
> complexity. It might be safer to assume that only access to already
> fetched information is safe. In particular if you move
>
> messageid = notmuch_message_get_message_id(msg);
>
> before you close the database, then printing it out afterwards works. I
> didn't run it valgrind to make sure, but afaik, that should be perfectly
> legal.

Ok, I forgot the "expected behaviour" part of the bug report ;) I think
that this doesn't work is fine and I'm not surprised by and your
description of fetching it first is very reasonable.  However I was
expecting NOTMUCH_STATUS_XAPIAN_EXEPTION instead of bluntly getting
terminated.  This is what the notmuch_database_close() docs say after
all.

I had a little look and this seems to be caused by the
message->doc.termlist_begin() call in
_notmuch_message_ensure_metadata(), I didn't have xapian debug symbols
and am not familiar with xapian to quickly have an idea of whether this
case can be improved or not.  (-dbg debian packages for notmuch and
xapian would be very handy ;))

But part of my question is, *should* this be improved?  Am I
interpreting notmuch's intended API correctly?

> The original motivation (see 7864350c938944276c1a378539da7670c211b9b5)
> to allow long running processes to release the lock on the
> database. This is not a pattern we use in the CLI, so it's not as well
> tested as it could be. In particular the work to export
> notmuch_database_reopen (tests, documentation) has not happened yet.
>
> d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Usage after database close

2020-06-28 Thread Floris Bruynooghe
Hi,

I started writing some test cases to define better what you can do with
a closed database and make sure that the python bindings do not behave
unexpectedly here too.

One of the first things I tried ends up with xapian calling
exit_group(2) directly, terminating the process.  So I'm wondering if
I'm approaching this entirely the wrong way.  My understanding is that
we should generally be allowed to use anything after the database has
been closed, as long as nothing has been destroyed.

Below is a minimal reproducible example of what I'm testing so far.  I
must admit I'm generally lazy here and usually just test with notmuch
that is currently in Debian testing.

Cheers,
Floris

Here the script:

#!/bin/sh

MAILDIR=$(mktemp --directory)
export MAILDIR
echo $MAILDIR

mkdir $MAILDIR/tmp
mkdir $MAILDIR/new
mkdir $MAILDIR/cur

cat > $MAILDIR/notmuch-config < $MAILDIR/cur/1593342032.M818430P304029Q3.powell <
Date: Sun, 28 Jun 2020 13:00:32 -
From: s...@example.com
To: d...@example.com
Subject: Test mail
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

This is a test mail
EOF

notmuch new

cat >$MAILDIR/test.c <
#include 

int main(int argc, char** argv) {
notmuch_status_t status;
notmuch_database_t* db;
notmuch_message_t* msg;
const char* messageid;
printf("Opening db\n");
status = notmuch_database_open(argv[1], NOTMUCH_DATABASE_MODE_READ_ONLY, 
);
if (status != NOTMUCH_STATUS_SUCCESS) {
return status;
}
printf("Finding msg\n");
status = notmuch_database_find_message(db, "0...@powell.devork.be", );
if (status != NOTMUCH_STATUS_SUCCESS) {
return status;
}
printf("Closing db\n");
status = notmuch_database_close(db);
if (status != NOTMUCH_STATUS_SUCCESS) {
return status;
}
printf("Get messageid\n");
messageid = notmuch_message_get_message_id(msg);
if (messageid == NULL) {
return 1;
}
printf("Messageid: %s\n", messageid);
printf("The end.\n");
}
EOF

gcc $MAILDIR/test.c -lnotmuch -o $MAILDIR/test

$MAILDIR/test $MAILDIR
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] python-cffi: read version from notmuch version file

2020-06-23 Thread Floris Bruynooghe
On Tue 23 Jun 2020 at 13:43 +0300, Frank LENORMAND wrote:

> On Tue Jun 23 12:33:36 2020, David Bremner wrote:
>> Frank LENORMAND  writes:
>> > For example, 0.30.1, with the first two numbers coming from the main
>> > repository, and the last one acting as major for the bindings.
>> >
>> > 0.29.3 → 0.29.1
>> > 0.30-rc2 → 0.30.1-rc2
>> > etc.
>> >
>> 
>> I'm mainly interested in supporting two use cases for notmuch: building
>> everything from source, and binary packages of released versions. We've
>> already gone to some trouble to tell Emacs users that try to mix and
>> match versions that they are on their own, and this seems to apply even
>> more strongly to bindings users.
>>
>> With that said, if Floris thinks some hierarchical version is useful,
>> and is willing to maintain it, I can live with it. I would ask that:
>> 
>> 1) You keep the whole "upstream" version number. So the first example
>> would be 0.29.3.1.  0.29.1 is a previous version of notmuch, and that
>> ambiguity can only cause trouble.
>
> The idea was that the bindings will work with the X.Y version they were
> released for, since the last component in X.Y.Z is for minor changes that
> shouldn't affect the API.

Minor nitpicking, but API is not strong enough here, you'd need to
ensure ABI compatibility.

> So we can keep X.Y from NotMuch itself, and append some information that
> hint at the state of the bindings.
[...]
> Or the exact same version number, but then what should happen to it when
> the bindings are modified, but not NotMuch?

If it was bad enough to need a new release then I guess everyone gets
the same version bump as the entire project gets a bugfix release?

I honestly like the simplicity of just having the same version number
and not having to think about maintaining it separately.  It also means
we mostly don't have to worry about how setuptools/pip is going to view
the version number.

The only way I think this could break is if we want to break backwards
compatibility in the bindings, but we're not supposed to do that
(realistically an impossible task in Python if you ask me, but we can
aim for at least avoiding doing this knowingly).

The most likely version number sin is that the python bindings get a new
feature while libnotmuch only gets bugfix.  I also don't think this is
terrible, but that's perhaps unusual and frowned upon.  Maybe this
warrants a README in the bindings to warn the version number just tracks
libnotmuch and as far as python goes can only be used to order the
releases.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] python-cffi: read version from notmuch version file

2020-06-22 Thread Floris Bruynooghe
On Fri 19 Jun 2020 at 15:26 +0300, Frank LENORMAND wrote:

> On Fri Jun 19 12:46:28 2020, Floris Bruynooghe wrote:
>> This keeps it in sync with the main notmuch version which is less
>> confusing to users.
>> ---
>>  bindings/python-cffi/setup.py | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/bindings/python-cffi/setup.py b/bindings/python-cffi/setup.py
>> index 37918e3d..1effcfc6 100644
>> --- a/bindings/python-cffi/setup.py
>> +++ b/bindings/python-cffi/setup.py
>> @@ -1,9 +1,17 @@
>> +import pathlib
>> +
>>  import setuptools
>>  
>>  
>> +THIS_FILE = pathlib.Path(__file__).absolute()
>> +PROJECT_ROOT = THIS_FILE.parent.parent.parent
>> +with open(PROJECT_ROOT.joinpath('version')) as fp:
>> +VERSION = fp.read().strip()
>> +
>> +
>>  setuptools.setup(
>>  name='notmuch2',
>> -version='0.1',
>> +version=VERSION,
>>  description='Pythonic bindings for the notmuch mail database using 
>> CFFI',
>>  author='Floris Bruynooghe',
>>  author_email='f...@devork.be',
>> -- 
>> 2.27.0
>
> It seems that this strategy doesn't work well when the user runs
> `pip install .` in the `bindings/python-cffi` directory.
>
> Apparently all the files are copied to a temporary directory first:
>
> https://travis-ci.com/github/pazz/alot/jobs/351377760#L708-L710
>
> It doesn't happen with the original bindings, probably because the version
> number is stored in `bindings/python/notmuch/version.py`, which is also
> copied when `pip` runs.

Ouch, I only tested pip install -e, which does work.  But indeed a plain
pip install no longer works which is pretty bad.

I guess we could either revert this and do the same sed hackery as the
other bindings, or copy the version file into bindings/python-cffi and
have it loaded in the same way as now.  It would still have to be kept
in sync there sadly.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: python/notmuch2 on PyPI

2020-06-22 Thread Floris Bruynooghe
On Fri 19 Jun 2020 at 09:30 -0300, David Bremner wrote:
> Patrick Totzke  writes:
>> Just to clarify: alot does not, and will not, depend on packages being on 
>> PyPI

Ah, my bad.  I got some github threads mixed up and assumed this had to
do with alot.

> Notmuch as a project does not currently distribute any binary packages,
> whether for linux distros or for PyPi, or fancy new things like flatpaks
> or snaps.

Also fair enough, I think that's a very reasonable stance to take.


I was really hoping to hear from Thore on their motivation and how they
envision this to ideally work.  I'm curious how much work the syncing is
and how to keep it up to date etc.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


python/notmuch2 on PyPI

2020-06-19 Thread Floris Bruynooghe
Hi Thore, notmuch folks,

I noticed that Thore published notmuch2 on PyPI.  I think this is
because alot needs it's users to be able to pull it in as a dependency
using the normal Python mechanisms?

It seems this is currently published from a fork at
https://github.com/weilbith/notmuch2-python-bindings and I wondered if
it was possible to publish this directly from the main notmuch repo or
even integrate this into the normal notmuch release process.  What are
the pros and cons of this?  Is it a bad idea to tie these two publishing
mechanisms too close together?  To difficult to do bugfix releases?  Is
it too hard to make pypi publishing frictionless enough for the main
release process?

An cool stretch goal would be to publish manylinux wheels with the
library included.  But let's not get too hung up on that, small steps
are great.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] python-cffi: read version from notmuch version file

2020-06-19 Thread Floris Bruynooghe
This keeps it in sync with the main notmuch version which is less
confusing to users.
---
 bindings/python-cffi/setup.py | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/bindings/python-cffi/setup.py b/bindings/python-cffi/setup.py
index 37918e3d..1effcfc6 100644
--- a/bindings/python-cffi/setup.py
+++ b/bindings/python-cffi/setup.py
@@ -1,9 +1,17 @@
+import pathlib
+
 import setuptools
 
 
+THIS_FILE = pathlib.Path(__file__).absolute()
+PROJECT_ROOT = THIS_FILE.parent.parent.parent
+with open(PROJECT_ROOT.joinpath('version')) as fp:
+VERSION = fp.read().strip()
+
+
 setuptools.setup(
 name='notmuch2',
-version='0.1',
+version=VERSION,
 description='Pythonic bindings for the notmuch mail database using CFFI',
 author='Floris Bruynooghe',
 author_email='f...@devork.be',
-- 
2.27.0

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


python-cffi: read version number from notmuch

2020-06-19 Thread Floris Bruynooghe
This reads the version from the toplevel notmuch version file.
The main assumption is obviously that setup.py is always in
bindings/python-cffi/setup.py together with the rest of the
notmuch git repo.


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [python-cffi] Version number for the `notmuch2` bindings

2020-06-19 Thread Floris Bruynooghe
On Thu 18 Jun 2020 at 16:56 -0300, David Bremner wrote:

> Frank LENORMAND  writes:
>
>> Hi,
>>
>> The original Python bindings follow the entire repository's version
>> number[1]. The new Python bindings use `0.1`[2].
>>
>> The Debian package[3] follows the same version number as well, but
>> it's starting to confuse maintainers of packages for other environments
>> (e.g. Pypi[4]), who use `0.1` because that's what's in the code.
>
> Floris might have some good reason in mind for the divergence. I will
> say it's a pain in Debian to have different binary packages (.deb's)
> built from the same source with different version numbers. So I'd need
> to be convinced.

There is no good reason, it was overlooked when merging the cffi
bindings into notmuch proper.

Is there any reason we can not directly read the toplevel version file
from inside setup.py instead of having to add sed hackery?

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 2/2] python config access: fix style and KeyError bug

2020-06-15 Thread Floris Bruynooghe
This fixes some minor style/pep8 things and adds tests for the new
config support.  Also fixes a bug where KeyError was never raised
on a missing key.
---
 bindings/python-cffi/notmuch2/_config.py  |  9 ++--
 bindings/python-cffi/tests/test_config.py | 56 +++
 2 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100644 bindings/python-cffi/tests/test_config.py

diff --git a/bindings/python-cffi/notmuch2/_config.py 
b/bindings/python-cffi/notmuch2/_config.py
index 58383c16..29de6495 100644
--- a/bindings/python-cffi/notmuch2/_config.py
+++ b/bindings/python-cffi/notmuch2/_config.py
@@ -4,9 +4,12 @@ import notmuch2._base as base
 import notmuch2._capi as capi
 import notmuch2._errors as errors
 
+
 __all__ = ['ConfigMapping']
 
+
 class ConfigIter(base.NotmuchIter):
+
 def __init__(self, parent, iter_p):
 super().__init__(
 parent, iter_p,
@@ -19,6 +22,7 @@ class ConfigIter(base.NotmuchIter):
 item = super().__next__()
 return base.BinString.from_cffi(item)
 
+
 class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
 """The config key/value pairs stored in the database.
 
@@ -50,11 +54,10 @@ class ConfigMapping(base.NotmuchObject, 
collections.abc.MutableMapping):
 ret = capi.lib.notmuch_database_get_config(self._ptr(), key, val_pp)
 if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
 raise errors.NotmuchError(ret)
-if val_pp[0] == "":
-capi.lib.free(val_pp[0])
-raise KeyError
 val = base.BinString.from_cffi(val_pp[0])
 capi.lib.free(val_pp[0])
+if val == '':
+raise KeyError
 return val
 
 def __setitem__(self, key, val):
diff --git a/bindings/python-cffi/tests/test_config.py 
b/bindings/python-cffi/tests/test_config.py
new file mode 100644
index ..1b2695f5
--- /dev/null
+++ b/bindings/python-cffi/tests/test_config.py
@@ -0,0 +1,56 @@
+import collections.abc
+
+import pytest
+
+import notmuch2._database as dbmod
+
+import notmuch2._config as config
+
+
+class TestIter:
+
+@pytest.fixture
+def db(self, maildir):
+with dbmod.Database.create(maildir.path) as db:
+yield db
+
+def test_type(self, db):
+assert isinstance(db.config, collections.abc.MutableMapping)
+assert isinstance(db.config, config.ConfigMapping)
+
+def test_alive(self, db):
+assert db.config.alive
+
+def test_set_get(self, maildir):
+# Ensure get-set works from different db objects
+with dbmod.Database.create(maildir.path) as db0:
+db0.config['spam'] = 'ham'
+with dbmod.Database(maildir.path) as db1:
+assert db1.config['spam'] == 'ham'
+
+def test_get_keyerror(self, db):
+with pytest.raises(KeyError):
+val = db.config['not-a-key']
+print(repr(val))
+
+def test_iter(self, db):
+assert list(db.config) == []
+db.config['spam'] = 'ham'
+db.config['eggs'] = 'bacon'
+assert set(db.config) == {'spam', 'eggs'}
+assert set(db.config.keys()) == {'spam', 'eggs'}
+assert set(db.config.values()) == {'ham', 'bacon'}
+assert set(db.config.items()) == {('spam', 'ham'), ('eggs', 'bacon')}
+
+def test_len(self, db):
+assert len(db.config) == 0
+db.config['spam'] = 'ham'
+assert len(db.config) == 1
+db.config['eggs'] = 'bacon'
+assert len(db.config) == 2
+
+def test_del(self, db):
+db.config['spam'] = 'ham'
+assert db.config.get('spam') == 'ham'
+del db.config['spam']
+assert db.config.get('spam') is None
-- 
2.27.0

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 1/2] python/notmuch2: add bindings for the database config strings

2020-06-15 Thread Floris Bruynooghe
From: Anton Khirnov 

---
 bindings/python-cffi/notmuch2/_build.py| 17 +
 bindings/python-cffi/notmuch2/_config.py   | 84 ++
 bindings/python-cffi/notmuch2/_database.py | 23 ++
 3 files changed, 124 insertions(+)
 create mode 100644 bindings/python-cffi/notmuch2/_config.py

diff --git a/bindings/python-cffi/notmuch2/_build.py 
b/bindings/python-cffi/notmuch2/_build.py
index 5e1fcac1..f269f2a1 100644
--- a/bindings/python-cffi/notmuch2/_build.py
+++ b/bindings/python-cffi/notmuch2/_build.py
@@ -314,6 +314,23 @@ ffibuilder.cdef(
 notmuch_indexopts_get_decrypt_policy (const notmuch_indexopts_t 
*indexopts);
 void
 notmuch_indexopts_destroy (notmuch_indexopts_t *options);
+
+notmuch_status_t
+notmuch_database_set_config (notmuch_database_t *db, const char *key, 
const char *value);
+notmuch_status_t
+notmuch_database_get_config (notmuch_database_t *db, const char *key, char 
**value);
+notmuch_status_t
+notmuch_database_get_config_list (notmuch_database_t *db, const char 
*prefix, notmuch_config_list_t **out);
+notmuch_bool_t
+notmuch_config_list_valid (notmuch_config_list_t *config_list);
+const char *
+notmuch_config_list_key (notmuch_config_list_t *config_list);
+const char *
+notmuch_config_list_value (notmuch_config_list_t *config_list);
+void
+notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
+void
+notmuch_config_list_destroy (notmuch_config_list_t *config_list);
 """
 )
 
diff --git a/bindings/python-cffi/notmuch2/_config.py 
b/bindings/python-cffi/notmuch2/_config.py
new file mode 100644
index ..58383c16
--- /dev/null
+++ b/bindings/python-cffi/notmuch2/_config.py
@@ -0,0 +1,84 @@
+import collections.abc
+
+import notmuch2._base as base
+import notmuch2._capi as capi
+import notmuch2._errors as errors
+
+__all__ = ['ConfigMapping']
+
+class ConfigIter(base.NotmuchIter):
+def __init__(self, parent, iter_p):
+super().__init__(
+parent, iter_p,
+fn_destroy=capi.lib.notmuch_config_list_destroy,
+fn_valid=capi.lib.notmuch_config_list_valid,
+fn_get=capi.lib.notmuch_config_list_key,
+fn_next=capi.lib.notmuch_config_list_move_to_next)
+
+def __next__(self):
+item = super().__next__()
+return base.BinString.from_cffi(item)
+
+class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
+"""The config key/value pairs stored in the database.
+
+The entries are exposed as a :class:`collections.abc.MutableMapping` 
object.
+Note that setting a value to an empty string is the same as deleting it.
+
+:param parent: the parent object
+:param ptr_name: the name of the attribute on the parent which will
+   return the memory pointer.  This allows this object to
+   access the pointer via the parent's descriptor and thus
+   trigger :class:`MemoryPointer`'s memory safety.
+"""
+
+def __init__(self, parent, ptr_name):
+self._parent = parent
+self._ptr = lambda: getattr(parent, ptr_name)
+
+@property
+def alive(self):
+return self._parent.alive
+
+def _destroy(self):
+pass
+
+def __getitem__(self, key):
+if isinstance(key, str):
+key = key.encode('utf-8')
+val_pp = capi.ffi.new('char**')
+ret = capi.lib.notmuch_database_get_config(self._ptr(), key, val_pp)
+if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+raise errors.NotmuchError(ret)
+if val_pp[0] == "":
+capi.lib.free(val_pp[0])
+raise KeyError
+val = base.BinString.from_cffi(val_pp[0])
+capi.lib.free(val_pp[0])
+return val
+
+def __setitem__(self, key, val):
+if isinstance(key, str):
+key = key.encode('utf-8')
+if isinstance(val, str):
+val = val.encode('utf-8')
+ret = capi.lib.notmuch_database_set_config(self._ptr(), key, val)
+if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+raise errors.NotmuchError(ret)
+
+def __delitem__(self, key):
+self[key] = ""
+
+def __iter__(self):
+"""Return an iterator over the config items.
+
+:raises NullPointerError: If the iterator can not be created.
+"""
+configlist_pp = capi.ffi.new('notmuch_config_list_t**')
+ret = capi.lib.notmuch_database_get_config_list(self._ptr(), b'', 
configlist_pp)
+if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+raise errors.NotmuchError(ret)
+return ConfigIter(self._parent, configlist_pp[0])
+
+def __len__(self):
+return sum(1 for t in self)
diff --git a/bindings/python-cffi/notmuch2/_database.py 
b/bindings/python-cffi/notmuch2/_database.py
index 95f59ca0..3c06402d 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -7,6 +7,7 @@ import pathlib
 import weakref

python: config API

2020-06-15 Thread Floris Bruynooghe
This is a followup on the patch from Anton Khirnov adding config
API support to the python bindings.

I can not help myself but point out that I did not spot the bug
until not only I had written tests, but until I looked at the
test coverage to see what was not yet executed and added more
tests.  Tests and test coverage is good!


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 1/2] python/notmuch2: do not destroy messages owned by a query

2020-06-15 Thread Floris Bruynooghe
From: Anton Khirnov 

Any messages retrieved from a query - either directly via
search_messages() or indirectly via thread objects - are owned by that
query. Retrieving the same message (i.e. corresponding to the same
message ID / database object) several times will always yield the same
C object.

The caller is allowed to destroy message objects owned by a query before
the query itself - which can save memory for long-lived queries.
However, that message must then never be retrieved again from that
query.

The python-notmuch2 bindings will currently destroy every message object
in Message._destroy(), which will lead to an invalid free if the same
message is then retrieved again. E.g. the following python program leads
to libtalloc abort()ing:

import notmuch2
db   = notmuch2.Database(mode = notmuch2.Database.MODE.READ_ONLY)
t= next(db.threads('*'))
msgs = list(zip(t.toplevel(), t.toplevel()))
msgs = list(zip(t.toplevel(), t.toplevel()))

Fix this issue by creating a subclass of Message, which is used for
"standalone" message which have to be freed by the caller. Message class
is then used only for messages descended from a query, which do not need
to be freed by the caller.
---
 bindings/python-cffi/notmuch2/_database.py |  6 ++--
 bindings/python-cffi/notmuch2/_message.py  | 42 ++
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py 
b/bindings/python-cffi/notmuch2/_database.py
index 95f59ca0..f14eac78 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
   capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
 if ret not in ok:
 raise errors.NotmuchError(ret)
-msg = message.Message(self, msg_pp[0], db=self)
+msg = message.StandaloneMessage(self, msg_pp[0], db=self)
 if sync_flags:
 msg.tags.from_maildir_flags()
 return self.AddedMessage(
@@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
 msg_p = msg_pp[0]
 if msg_p == capi.ffi.NULL:
 raise LookupError
-msg = message.Message(self, msg_p, db=self)
+msg = message.StandaloneMessage(self, msg_p, db=self)
 return msg
 
 def get(self, filename):
@@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
 msg_p = msg_pp[0]
 if msg_p == capi.ffi.NULL:
 raise LookupError
-msg = message.Message(self, msg_p, db=self)
+msg = message.StandaloneMessage(self, msg_p, db=self)
 return msg
 
 @property
diff --git a/bindings/python-cffi/notmuch2/_message.py 
b/bindings/python-cffi/notmuch2/_message.py
index c5fdbf6d..416ce7ca 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -14,7 +14,7 @@ __all__ = ['Message']
 
 
 class Message(base.NotmuchObject):
-"""An email message stored in the notmuch database.
+"""An email message stored in the notmuch database retrieved via a query.
 
 This should not be directly created, instead it will be returned
 by calling methods on :class:`Database`.  A message keeps a
@@ -61,22 +61,10 @@ class Message(base.NotmuchObject):
 
 @property
 def alive(self):
-if not self._parent.alive:
-return False
-try:
-self._msg_p
-except errors.ObjectDestroyedError:
-return False
-else:
-return True
-
-def __del__(self):
-self._destroy()
+return self._parent.alive
 
 def _destroy(self):
-if self.alive:
-capi.lib.notmuch_message_destroy(self._msg_p)
-self._msg_p = None
+pass
 
 @property
 def messageid(self):
@@ -375,6 +363,30 @@ class Message(base.NotmuchObject):
 if isinstance(other, self.__class__):
 return self.messageid == other.messageid
 
+class StandaloneMessage(Message):
+"""An email message stored in the notmuch database.
+
+This subclass of Message is used for messages that are retrieved from the
+database directly and are not owned by a query.
+"""
+@property
+def alive(self):
+if not self._parent.alive:
+return False
+try:
+self._msg_p
+except errors.ObjectDestroyedError:
+return False
+else:
+return True
+
+def __del__(self):
+self._destroy()
+
+def _destroy(self):
+if self.alive:
+capi.lib.notmuch_message_destroy(self._msg_p)
+self._msg_p = None
 
 class FilenamesIter(base.NotmuchIter):
 """Iterator for binary filenames objects."""
-- 
2.27.0

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 2/2] Make messages returned by Thread objects owned

2020-06-15 Thread Floris Bruynooghe
This reverses the logic of StandaloneMessage to instead create a
OwnedMessage.  Only the Thread class allows retrieving messages more
then once so it can explicitly create such messages.

The added test fails with SIGABRT without the fix for the message
re-use in threads being present.
---
 bindings/python-cffi/notmuch2/_database.py  |  6 +--
 bindings/python-cffi/notmuch2/_message.py   | 55 -
 bindings/python-cffi/notmuch2/_thread.py|  8 ++-
 bindings/python-cffi/tests/test_database.py | 11 +
 4 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py 
b/bindings/python-cffi/notmuch2/_database.py
index f14eac78..95f59ca0 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
   capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
 if ret not in ok:
 raise errors.NotmuchError(ret)
-msg = message.StandaloneMessage(self, msg_pp[0], db=self)
+msg = message.Message(self, msg_pp[0], db=self)
 if sync_flags:
 msg.tags.from_maildir_flags()
 return self.AddedMessage(
@@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
 msg_p = msg_pp[0]
 if msg_p == capi.ffi.NULL:
 raise LookupError
-msg = message.StandaloneMessage(self, msg_p, db=self)
+msg = message.Message(self, msg_p, db=self)
 return msg
 
 def get(self, filename):
@@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
 msg_p = msg_pp[0]
 if msg_p == capi.ffi.NULL:
 raise LookupError
-msg = message.StandaloneMessage(self, msg_p, db=self)
+msg = message.Message(self, msg_p, db=self)
 return msg
 
 @property
diff --git a/bindings/python-cffi/notmuch2/_message.py 
b/bindings/python-cffi/notmuch2/_message.py
index 416ce7ca..02de50ad 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -47,9 +47,7 @@ class Message(base.NotmuchObject):
 :type db: Database
 :param msg_p: The C pointer to the ``notmuch_message_t``.
 :type msg_p: 
-
 :param dup: Whether the message was a duplicate on insertion.
-
 :type dup: None or bool
 """
 _msg_p = base.MemoryPointer()
@@ -61,10 +59,22 @@ class Message(base.NotmuchObject):
 
 @property
 def alive(self):
-return self._parent.alive
+if not self._parent.alive:
+return False
+try:
+self._msg_p
+except errors.ObjectDestroyedError:
+return False
+else:
+return True
+
+def __del__(self):
+self._destroy()
 
 def _destroy(self):
-pass
+if self.alive:
+capi.lib.notmuch_message_destroy(self._msg_p)
+self._msg_p = None
 
 @property
 def messageid(self):
@@ -363,30 +373,26 @@ class Message(base.NotmuchObject):
 if isinstance(other, self.__class__):
 return self.messageid == other.messageid
 
-class StandaloneMessage(Message):
-"""An email message stored in the notmuch database.
 
-This subclass of Message is used for messages that are retrieved from the
-database directly and are not owned by a query.
+class OwnedMessage(Message):
+"""An email message owned by parent thread object.
+
+This subclass of Message is used for messages that are retrieved
+from the notmuch database via a parent :class:`notmuch2.Thread`
+object, which "owns" this message.  This means that when this
+message object is destroyed, by calling :func:`del` or
+:meth:`_destroy` directly or indirectly, the message is not freed
+in the notmuch API and the parent :class:`notmuch2.Thread` object
+can return the same object again when needed.
 """
+
 @property
 def alive(self):
-if not self._parent.alive:
-return False
-try:
-self._msg_p
-except errors.ObjectDestroyedError:
-return False
-else:
-return True
-
-def __del__(self):
-self._destroy()
+return self._parent.alive
 
 def _destroy(self):
-if self.alive:
-capi.lib.notmuch_message_destroy(self._msg_p)
-self._msg_p = None
+pass
+
 
 class FilenamesIter(base.NotmuchIter):
 """Iterator for binary filenames objects."""
@@ -690,8 +696,9 @@ collections.abc.ValuesView.register(PropertiesValuesView)
 
 class MessageIter(base.NotmuchIter):
 
-def __init__(self, parent, msgs_p, *, db):
+def __init__(self, parent, msgs_p, *, db, msg_cls=Message):
 self._db = db
+self._msg_cls = msg_cls
 super().__init__(parent, msgs_p,
  fn_destroy=capi.lib.notmuch_messages_destroy,
  fn_valid=capi.lib.notmuch_messages_valid,
@@ -700,4 +707,4 @@ class 

python: Continuing message re-use fix

2020-06-15 Thread Floris Bruynooghe
Hi,

This builds on the patch by Anton Khirnov to fix the message re-use
that is possible when accessing messages from a thread.  I started
with just addressing my own comments on this patch, but evolved it
into switching the logic around and leave the normal Message object
untouched.  Instead I created a new OwnedMessage which is used by
the Thread which does not free itself on __del__().  I think this
is preferable because the other iterators, mainly Database.messages(),
do not allow retrieving messages more than once since the query object
is hidden from the API.

I've left the original commit in this patch series to not alter any
contributions.

Cheers,
Floris


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] Update tox.ini for python3.8 and fix pypy3.6

2020-06-15 Thread Floris Bruynooghe
On Mon 15 Jun 2020 at 07:06 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>>  [testenv]
>>  deps =
>> @@ -14,3 +14,6 @@ commands = pytest --cov={envsitepackagesdir}/notmuch2 
>> {posargs}
>>  
>>  [testenv:pypy35]
>>  basepython = pypy3.5
>> +
>> +[testenv:pypy36]
>> +basepython = pypy3.6
>
> I'm not an expert, but should python 3.7 and python 3.8 have similar
> clauses? Or do they work by default?

They do work by default.  But to be honest these two pypy ones are not
fully standardised and rely on me manually ensuring I have both a
pypy3.5 and pypy3.6 binary on my path.  The default thing that works is
just a "pypy3" which doesn't give you any guarantees over the version
you get (other than not pypy2).
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] Support aborting the atomic context

2020-06-15 Thread Floris Bruynooghe
On Mon 15 Jun 2020 at 07:35 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>> This is an implementation of what was suggested in
>> id:87v9k96xtl@powell.devork.be It closes the database as that is
>> the only safe way to do this afaik.
>>
>> Currently when the database is closed there are still a bunch of
>> operations which can result in segfaults.

I realise that this is perhaps not a very helpful comment.  I'll see if
I can narrow this down into a proper bug report.

>> Yet the API also promises
>> that some operations are still valid, basically those which only
>> access already previously retrieved information.  It would be nice to
>> find a good solution for this in the python bindings, but I don't yet
>> know what this would be.
>
> There is a Xapian method WritableDatabase::cancel_transaction(), but it
> is not currently exposed by notmuch.  I think it could be added, but
> getting the testing working right is not something I want to tackle at
> this point in the release cycle.  I guess this should be upwardly
> compatible, as all code correct for your current implementation should
> still work if we replace notmuch_database_close with
> notmuch_cancel_atomic. Thoughts?

I agree with your reasoning here that such a change would be upwardly
compatible.  Though I can think of some really convoluted scenarios
where this could be false, e.g. after aborting a transaction code could
rely on handling NOTMUCH_STATUS_XAPIAN_EXCEPTION it gets from a the
closed database.  This seems so much a corner case though, that I'd be
willing to ignore it.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: difficulties with notmuch2 python bindings for alot

2020-06-15 Thread Floris Bruynooghe
On Sun 14 Jun 2020 at 19:44 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>> One thing that they encountered and don't yet understand is that they
>> reported issues with leaking filedescriptors.  They used the bindings in
>> a way where I expect it to only call notmuch_database_destroy() when
>> they are done with it.  From reading notmuch.h I think that's correct
>> and there's no need to call notmuch_database_close() first.  Yet someone
>> reported that explicitly calling close helped.  Is the assumption I made
>> of only calling destroy correct?
>
> The first thing destroy does is call close. My read of the
> notmuch_database_close code is that it is idempotent (calling multiple
> times does not change anything).

Thanks for confirming, so that should be fine.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: difficulties with notmuch2 python bindings for alot

2020-06-14 Thread Floris Bruynooghe
Hi Daniel,

On Tue 09 Jun 2020 at 09:19 -0400, Daniel Kahn Gillmor wrote:
> I see over on github that alot is trying to port to the notmuch2
> bindings, and having a few problems with it:
>
>  https://github.com/pazz/alot/pull/1511
>
> alot is an important consumer of the notmuch python bindings, and it
> would be really great to see them successfully transition to the
> notmuch2 module.
>
> Floris, if you (or anyone else with this particular knowledge) has a
> chance to take a look and help them sort out the remaining issues, that
> would be much appreciated!

Thanks for the pointer, I've pinged the issue offering help with the
bindings and had a look through the existing things they discussed.

One thing that they encountered and don't yet understand is that they
reported issues with leaking filedescriptors.  They used the bindings in
a way where I expect it to only call notmuch_database_destroy() when
they are done with it.  From reading notmuch.h I think that's correct
and there's no need to call notmuch_database_close() first.  Yet someone
reported that explicitly calling close helped.  Is the assumption I made
of only calling destroy correct?

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


python: Update tox.ini for python 3.8

2020-06-14 Thread Floris Bruynooghe
This was released a while ago, we should support it.


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] Update tox.ini for python3.8 and fix pypy3.6

2020-06-14 Thread Floris Bruynooghe
Python 3.8 has been released for a while now, make sure we keep
supporting it correctly.

PyPy 3.6 wasn not configured correctly.
---
 bindings/python-cffi/tox.ini | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/bindings/python-cffi/tox.ini b/bindings/python-cffi/tox.ini
index 34148a11..7cf93be0 100644
--- a/bindings/python-cffi/tox.ini
+++ b/bindings/python-cffi/tox.ini
@@ -3,7 +3,7 @@ minversion = 3.0
 addopts = -ra --cov=notmuch2 --cov=tests
 
 [tox]
-envlist = py35,py36,py37,pypy35,pypy36
+envlist = py35,py36,py37,py38,pypy35,pypy36
 
 [testenv]
 deps =
@@ -14,3 +14,6 @@ commands = pytest --cov={envsitepackagesdir}/notmuch2 
{posargs}
 
 [testenv:pypy35]
 basepython = pypy3.5
+
+[testenv:pypy36]
+basepython = pypy3.6
-- 
2.27.0

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] Add missing set methods to tagsets

2020-06-14 Thread Floris Bruynooghe
Even though we use collections.abc.Set which implements all these
methods under their operator names, the actual named variations of
these methods are shockingly missing.  So let's add them manually.
---
 bindings/python-cffi/notmuch2/_tags.py  | 21 +
 bindings/python-cffi/tests/test_tags.py | 62 +
 2 files changed, 83 insertions(+)

diff --git a/bindings/python-cffi/notmuch2/_tags.py 
b/bindings/python-cffi/notmuch2/_tags.py
index 212852a8..3b14c981 100644
--- a/bindings/python-cffi/notmuch2/_tags.py
+++ b/bindings/python-cffi/notmuch2/_tags.py
@@ -110,6 +110,27 @@ class ImmutableTagSet(base.NotmuchObject, 
collections.abc.Set):
 def __eq__(self, other):
 return tuple(sorted(self.iter())) == tuple(sorted(other.iter()))
 
+def issubset(self, other):
+return self <= other
+
+def issuperset(self, other):
+return self >= other
+
+def union(self, other):
+return self | other
+
+def intersection(self, other):
+return self & other
+
+def difference(self, other):
+return self - other
+
+def symmetric_difference(self, other):
+return self ^ other
+
+def copy(self):
+return set(self)
+
 def __hash__(self):
 return hash(tuple(self.iter()))
 
diff --git a/bindings/python-cffi/tests/test_tags.py 
b/bindings/python-cffi/tests/test_tags.py
index f12fa1e6..faf3947b 100644
--- a/bindings/python-cffi/tests/test_tags.py
+++ b/bindings/python-cffi/tests/test_tags.py
@@ -50,6 +50,22 @@ class TestImmutable:
 assert 'unread' in tagset
 assert 'foo' not in tagset
 
+def test_isdisjoint(self, tagset):
+assert tagset.isdisjoint(set(['spam', 'ham']))
+assert not tagset.isdisjoint(set(['inbox']))
+
+def test_issubset(self, tagset):
+assert {'inbox'} <= tagset
+assert {'inbox'}.issubset(tagset)
+assert tagset <= {'inbox', 'unread', 'spam'}
+assert tagset.issubset({'inbox', 'unread', 'spam'})
+
+def test_issuperset(self, tagset):
+assert {'inbox', 'unread', 'spam'} >= tagset
+assert {'inbox', 'unread', 'spam'}.issuperset(tagset)
+assert tagset >= {'inbox'}
+assert tagset.issuperset({'inbox'})
+
 def test_iter(self, tagset):
 expected = sorted(['unread', 'inbox'])
 found = []
@@ -78,18 +94,30 @@ class TestImmutable:
 assert isinstance(common, set)
 assert isinstance(common, collections.abc.Set)
 assert common == {'unread'}
+common = tagset.intersection({'unread'})
+assert isinstance(common, set)
+assert isinstance(common, collections.abc.Set)
+assert common == {'unread'}
 
 def test_or(self, tagset):
 res = tagset | {'foo'}
 assert isinstance(res, set)
 assert isinstance(res, collections.abc.Set)
 assert res == {'unread', 'inbox', 'foo'}
+res = tagset.union({'foo'})
+assert isinstance(res, set)
+assert isinstance(res, collections.abc.Set)
+assert res == {'unread', 'inbox', 'foo'}
 
 def test_sub(self, tagset):
 res = tagset - {'unread'}
 assert isinstance(res, set)
 assert isinstance(res, collections.abc.Set)
 assert res == {'inbox'}
+res = tagset.difference({'unread'})
+assert isinstance(res, set)
+assert isinstance(res, collections.abc.Set)
+assert res == {'inbox'}
 
 def test_rsub(self, tagset):
 res = {'foo', 'unread'} - tagset
@@ -102,6 +130,10 @@ class TestImmutable:
 assert isinstance(res, set)
 assert isinstance(res, collections.abc.Set)
 assert res == {'inbox', 'foo'}
+res = tagset.symmetric_difference({'unread', 'foo'})
+assert isinstance(res, set)
+assert isinstance(res, collections.abc.Set)
+assert res == {'inbox', 'foo'}
 
 def test_rxor(self, tagset):
 res = {'unread', 'foo'} ^ tagset
@@ -109,6 +141,12 @@ class TestImmutable:
 assert isinstance(res, collections.abc.Set)
 assert res == {'inbox', 'foo'}
 
+def test_copy(self, tagset):
+res = tagset.copy()
+assert isinstance(res, set)
+assert isinstance(res, collections.abc.Set)
+assert res == {'inbox', 'unread'}
+
 
 class TestMutableTagset:
 
@@ -175,3 +213,27 @@ class TestMutableTagset:
 msg.tags.to_maildir_flags()
 flags = msg.path.name.split(',')[-1]
 assert 'F' not in flags
+
+def test_isdisjoint(self, tagset):
+assert tagset.isdisjoint(set(['spam', 'ham']))
+assert not tagset.isdisjoint(set(['inbox']))
+
+def test_issubset(self, tagset):
+assert {'inbox'} <= tagset
+assert {'inbox'}.issubset(tagset)
+assert not {'spam'} <= tagset
+assert not {'spam'}.issubset(tagset)
+assert tagset <= {'inbox', 'unread', 'spam'}
+assert tagset.issubset({'inbox', 'unread', 'spam'})
+assert 

python-cffi: add missing tagset methods

2020-06-14 Thread Floris Bruynooghe
This issue was found by alot's porting efforts.  It seems these
were simply missing.


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] Support aborting the atomic context

2020-06-14 Thread Floris Bruynooghe
Since it is possible to use an atomic context to abort a number of
changes support this usage.  Because the only way to actually abort
the transaction is to close the database this must also do so.
---
 bindings/python-cffi/notmuch2/_database.py  | 16 +++-
 bindings/python-cffi/tests/test_database.py |  5 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py 
b/bindings/python-cffi/notmuch2/_database.py
index 95f59ca0..c851f0a5 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -641,6 +641,7 @@ class AtomicContext:
 def __init__(self, db, ptr_name):
 self._db = db
 self._ptr = lambda: getattr(db, ptr_name)
+self._exit_fn = lambda: None
 
 def __del__(self):
 self._destroy()
@@ -656,13 +657,17 @@ class AtomicContext:
 ret = capi.lib.notmuch_database_begin_atomic(self._ptr())
 if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
 raise errors.NotmuchError(ret)
+self._exit_fn = self._end_atomic
 return self
 
-def __exit__(self, exc_type, exc_value, traceback):
+def _end_atomic(self):
 ret = capi.lib.notmuch_database_end_atomic(self._ptr())
 if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
 raise errors.NotmuchError(ret)
 
+def __exit__(self, exc_type, exc_value, traceback):
+self._exit_fn()
+
 def force_end(self):
 """Force ending the atomic section.
 
@@ -681,6 +686,15 @@ class AtomicContext:
 if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
 raise errors.NotmuchError(ret)
 
+def abort(self):
+"""Abort the transaction.
+
+Aborting a transaction will not commit any of the changes, but
+will also implicitly close the database.
+"""
+self._exit_fn = lambda: None
+self._db.close()
+
 
 @functools.total_ordering
 class DbRevision:
diff --git a/bindings/python-cffi/tests/test_database.py 
b/bindings/python-cffi/tests/test_database.py
index e3a8344d..aa2cbdc7 100644
--- a/bindings/python-cffi/tests/test_database.py
+++ b/bindings/python-cffi/tests/test_database.py
@@ -127,6 +127,11 @@ class TestAtomic:
 with pytest.raises(errors.UnbalancedAtomicError):
 ctx.force_end()
 
+def test_abort(self, db):
+with db.atomic() as txn:
+txn.abort()
+assert db.closed
+
 
 class TestRevision:
 
-- 
2.27.0

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] Support aborting the atomic context

2020-06-14 Thread Floris Bruynooghe
This is an implementation of what was suggested in
id:87v9k96xtl@powell.devork.be It closes the database as that is
the only safe way to do this afaik.

Currently when the database is closed there are still a bunch of
operations which can result in segfaults.  Yet the API also promises
that some operations are still valid, basically those which only
access already previously retrieved information.  It would be nice to
find a good solution for this in the python bindings, but I don't yet
know what this would be.


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Python 3.x bindings support which versions, exactly?

2020-06-05 Thread Floris Bruynooghe
On Tue 26 May 2020 at 00:41 +0300, Tomi Ollila wrote:

> On Mon, May 25 2020, Floris Bruynooghe wrote:
>
>> In tox.ini the earliest version is 3.5 and if memory serves me right
>> there's a reasonably good reason for that.  I think the notmuch2
>> bindings use some features not yet present in 3.4.  But anything >= 3.5
>> should be supported and if not a bug.  I guess tox.ini should be updated
>> with 3.8 sometime :)
>
> I've been long thinking whether our current python3 check is "strict"
> enough, as it, in configure, is:
>
> if "$python" -c 'import sys; assert sys.version_info >= (3,0)' > /dev/null 
> 2>&1
>
> Perhaps that should be updated to (3,5) ?

If this check is only used to decide to build the python-cffi/notmuch2
bindings, then yes.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: status of the new python bindings

2020-06-02 Thread Floris Bruynooghe
Hi Anton,

Great that you're looking at this API!  My apologies for the late
response, this slipped by me probably as I was bulk marking things as
read when I came back from a few weeks away.

On Thu 07 May 2020 at 15:57 +0200, Anton Khirnov wrote:
> 1) What is the logic behind choosing whether something is exported as
> a property or as a method? E.g. Database.needs_upgrade is a property,
> while Database.revision() is a method. In my own python code, I tend to
> use @property for things that are "cheap" - i.e. do not involve
> (significant) IO or heavy computation and methods for those that do. But
> both of the above attributes involve library calls, presumably(?) of
> similar complexity. Would be nice if this was consistent.

Choosing when something should be a property is sometimes indeed tricky
and in general I agree you're right to expect property calls to be not
too expensive.  But I wouldn't go so far to consider every call into
libnotmuch as expensive.  I think there is also an element of how static
something is.  Usually I expect properties to behave more like
attributes, that is after all the point of them, and thus I don't expect
them to change often without explicitly manipulating them.

So that could justifies why I choose Database.version as a property
while Database.revision() is a method.  The revision changes all the
time as a side-effect of other things.  I think it also justifies
generally why tags are exposed as properties.

Database.needs_upgrade is indeed a more questionable choice, especially
given its naming which is more of a question being asked and thus
probably more suitable as a method.  It does change rarely, but does so
by interaction with another method - Database.upgrade().  So I would
agree that this perhaps wasn't the best choice and maybe that was better
as a method.

> 2) Atomic transactions are now exported as a context manager, which is
> nice and convenient for the usual use cases, but AFAIU does not have the
> same power. E.g. my tagging script does the tagging as a single atomic
> transaction and has a "dry-run" mode in which it omits the end_atomic()
> call, which is documented to throw away all the changes. This seems to
> not be possible with the new bindings.
> Would it be okay to add bindings for explicitly calling
> start/end_atomic()? Or is my approach considered invalid?

This is indeed a good usecase that I overlooked, I stopped reading at
the part where is says being and end atomic must always be used in
pairs...  But yes, we should add support for this too.  What would you
think of a .abort() on the context manager instead of directly exposing
the start/end?  E.g.:

db = notmuch2.Database()
with db.atomic() as txn:
# do stuff
txn.abort()

However if I read the docs correctly the transaction only actually is
aborted once the database is closed?  But when I try this:

with db.atomic():
# do stuff
db.close()

I discover that after calling .close() it's very easy to segfault by
making other calls, e.g. the above currently segfaults.  I thought this
wasn't so bad but perhaps .close() should immediately destroy the
database too.  This would again disallow some funky behaviour that the
raw C API allows I guess where some objects might still be valid to use.
But this is really hard to get right I think and I'd prefer to vote on
the side of safety in the Python bindings.

Still, it could be reasonable to make AtomicContext.abort() also close
the database and then make sure it doesn't call
notmuch_database_end_atomic() on exit.  I tried this locally and it
seems reasonable.  What do you think?


Lastly you can do this today by calling the C API directly:

db = notmuch2.Database()
notmuch2._capi.notmuch_database_being_atomic(db._db_p)

Yes, that uses a lot of internal stuff but it's also pretty advanced
usage.  I'm only suggesting you this to maybe unblock you in the short
term.


> 3) There seem to be no bindings for notmuch_database_set_config().

There are still bits of the API missing indeed.  Would be great to add
them but would be even better to have someone who needs it to validate
the API.  There were recently some good patches posted to expose the
config stuff, would be good to see that merged indeed.

> 4) The setup for building the documentation seems to be missing.

Hmm, yes.  The docstrings are written to work with sphinx autodoc but a
doc directory with sphinx configured never got added.  Perhaps we should
do this so that people can at least build docs locally.

> Anything else of note that remains to be implemented?

Probably.


Thanks for caring!

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Python 3.x bindings support which versions, exactly?

2020-05-25 Thread Floris Bruynooghe
On Sun 24 May 2020 at 16:29 -0300, David Bremner wrote:

> Ralph Seichter  writes:
>
>> Hello,
>>
>> having examined the source code, I assume that Notmuch's Python bindings
>> should support Python 3.x for any given 'x'. However, reading the Travis
>> logs at https://travis-ci.org/notmuch/notmuch and .travis.yml, the CI
>> tests apparently run on Ubuntu 18.04 and use only Python 3.6.
>>
>> While I hope there won't be any problems with Python 3.7, 3.8 and 3.9,
>> I wanted to ask here first if anybody has tested this yet?
>
> the old (modulename = notmuch) bindings actually do have memory
> management issues with python > 3.6 (maybe also with 3.6). The new
> bindings (modulename= notmuch2) should work with any python recent
> 3.x.

In tox.ini the earliest version is 3.5 and if memory serves me right
there's a reasonably good reason for that.  I think the notmuch2
bindings use some features not yet present in 3.4.  But anything >= 3.5
should be supported and if not a bug.  I guess tox.ini should be updated
with 3.8 sometime :)

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: python-cffi and ruby test suites fail in out-of-tree builds

2020-05-23 Thread Floris Bruynooghe
Hi,

On Thu 21 May 2020 at 21:29 -0400, Daniel Kahn Gillmor wrote:

> Hey folks--
>
> I just did a bit of testing and cleanup for out-of-tree builds (see the
> minor patches that should have landed on the list in the last hour or
> two).

It probably is indeed the unfortunate case that copying the python
source is currently the easiest.  I had a quick look and it seemed like
one'd have to dig into the cffi setuptools support to make this work and
I'm not sure how successful that would be, but I admit I didn't feel
like trying.

> For me, "make check" in an out-of-tree build works fine now, with the
> exception of T391-python-cffi.sh and T395-ruby.sh.

I checked out the python one, and maybe that's not too hard.  The
following patch made this work for both in tree and out of tree (on top
of your other patch):

modified   test/T391-python-cffi.sh
@@ -8,7 +8,7 @@ fi
 
 
 test_begin_subtest "python cffi tests"
-pytest_dir=$NOTMUCH_SRCDIR/bindings/python-cffi/build/stage
+pytest_dir=$NOTMUCH_BUILDDIR/bindings/python-cffi/build/stage
 printf "[pytest]\nminversion = 3.0\naddopts = -ra\n" > $pytest_dir/pytest.ini
 test_expect_success "(cd $pytest_dir && ${NOTMUCH_PYTHON} -m pytest 
--log-file=$TMP_DIRECTORY/test.output)"
 test_done



Hope that's helpful.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 2/2] python/notmuch2: add bindings for the database config strings

2020-05-21 Thread Floris Bruynooghe
Thanks for adding more of the API!

This mostly is fine as well, again I'd mainly ask to add tests however.
At least the things which are implemented directly I guess: setitem,
getitem, iter and len.


On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote:

> ---
>  bindings/python-cffi/notmuch2/_build.py| 17 +
>  bindings/python-cffi/notmuch2/_config.py   | 84 ++
>  bindings/python-cffi/notmuch2/_database.py | 23 ++
>  3 files changed, 124 insertions(+)
>  create mode 100644 bindings/python-cffi/notmuch2/_config.py
>
> diff --git a/bindings/python-cffi/notmuch2/_build.py 
> b/bindings/python-cffi/notmuch2/_build.py
> index 5e1fcac1..f269f2a1 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -314,6 +314,23 @@ ffibuilder.cdef(
>  notmuch_indexopts_get_decrypt_policy (const notmuch_indexopts_t 
> *indexopts);
>  void
>  notmuch_indexopts_destroy (notmuch_indexopts_t *options);
> +
> +notmuch_status_t
> +notmuch_database_set_config (notmuch_database_t *db, const char *key, 
> const char *value);
> +notmuch_status_t
> +notmuch_database_get_config (notmuch_database_t *db, const char *key, 
> char **value);
> +notmuch_status_t
> +notmuch_database_get_config_list (notmuch_database_t *db, const char 
> *prefix, notmuch_config_list_t **out);
> +notmuch_bool_t
> +notmuch_config_list_valid (notmuch_config_list_t *config_list);
> +const char *
> +notmuch_config_list_key (notmuch_config_list_t *config_list);
> +const char *
> +notmuch_config_list_value (notmuch_config_list_t *config_list);
> +void
> +notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
> +void
> +notmuch_config_list_destroy (notmuch_config_list_t *config_list);
>  """
>  )
>  
> diff --git a/bindings/python-cffi/notmuch2/_config.py 
> b/bindings/python-cffi/notmuch2/_config.py
> new file mode 100644
> index ..58383c16
> --- /dev/null
> +++ b/bindings/python-cffi/notmuch2/_config.py
> @@ -0,0 +1,84 @@
> +import collections.abc
> +
> +import notmuch2._base as base
> +import notmuch2._capi as capi
> +import notmuch2._errors as errors
> +
> +__all__ = ['ConfigMapping']
> +

Same style thing about 2 blank lines

> +class ConfigIter(base.NotmuchIter):
> +def __init__(self, parent, iter_p):
> +super().__init__(
> +parent, iter_p,
> +fn_destroy=capi.lib.notmuch_config_list_destroy,
> +fn_valid=capi.lib.notmuch_config_list_valid,
> +fn_get=capi.lib.notmuch_config_list_key,
> +fn_next=capi.lib.notmuch_config_list_move_to_next)
> +
> +def __next__(self):
> +item = super().__next__()
> +return base.BinString.from_cffi(item)
> +
> +class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
> +"""The config key/value pairs stored in the database.
> +
> +The entries are exposed as a :class:`collections.abc.MutableMapping` 
> object.
> +Note that setting a value to an empty string is the same as deleting it.
> +
> +:param parent: the parent object
> +:param ptr_name: the name of the attribute on the parent which will
> +   return the memory pointer.  This allows this object to
> +   access the pointer via the parent's descriptor and thus
> +   trigger :class:`MemoryPointer`'s memory safety.
> +"""
> +
> +def __init__(self, parent, ptr_name):
> +self._parent = parent
> +self._ptr = lambda: getattr(parent, ptr_name)
> +
> +@property
> +def alive(self):
> +return self._parent.alive
> +
> +def _destroy(self):
> +pass
> +
> +def __getitem__(self, key):
> +if isinstance(key, str):
> +key = key.encode('utf-8')
> +val_pp = capi.ffi.new('char**')
> +ret = capi.lib.notmuch_database_get_config(self._ptr(), key, val_pp)
> +if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +raise errors.NotmuchError(ret)
> +if val_pp[0] == "":
> +capi.lib.free(val_pp[0])
> +raise KeyError
> +val = base.BinString.from_cffi(val_pp[0])
> +capi.lib.free(val_pp[0])
> +return val
> +
> +def __setitem__(self, key, val):
> +if isinstance(key, str):
> +key = key.encode('utf-8')
> +if isinstance(val, str):
> +val = val.encode('utf-8')
> +ret = capi.lib.notmuch_database_set_config(self._ptr(), key, val)
> +if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +raise errors.NotmuchError(ret)
> +
> +def __delitem__(self, key):
> +self[key] = ""
> +
> +def __iter__(self):
> +"""Return an iterator over the config items.
> +
> +:raises NullPointerError: If the iterator can not be created.
> +"""
> +configlist_pp = capi.ffi.new('notmuch_config_list_t**')
> +ret = 

Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query

2020-05-21 Thread Floris Bruynooghe
Hi Anton,

Thanks for improving the bindings!  Any my apologies for the late
response, I failed to spot this mail the first time round.

Also, this is a pretty serious bug, thanks for finding it.

This looks pretty solid, a few small style comments that aren't very
important notwithstanding.  Though I do think you should be able to add
a test for this in the pytest tests.  I think just triggering the
use-after-free you demonstrate in the commit message is fine as it will
work with your patch.  The fact it will crash rather then fail without
is unfortunate but probably fine.

On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote:

> Any messages retrieved from a query - either directly via
> search_messages() or indirectly via thread objects - are owned by that
> query. Retrieving the same message (i.e. corresponding to the same
> message ID / database object) several times will always yield the same
> C object.
>
> The caller is allowed to destroy message objects owned by a query before
> the query itself - which can save memory for long-lived queries.
> However, that message must then never be retrieved again from that
> query.
>
> The python-notmuch2 bindings will currently destroy every message object
> in Message._destroy(), which will lead to an invalid free if the same
> message is then retrieved again. E.g. the following python program leads
> to libtalloc abort()ing:
>
> import notmuch2
> db   = notmuch2.Database(mode = notmuch2.Database.MODE.READ_ONLY)
> t= next(db.threads('*'))
> msgs = list(zip(t.toplevel(), t.toplevel()))
> msgs = list(zip(t.toplevel(), t.toplevel()))
>
> Fix this issue by creating a subclass of Message, which is used for
> "standalone" message which have to be freed by the caller. Message class
> is then used only for messages descended from a query, which do not need
> to be freed by the caller.
> ---
>  bindings/python-cffi/notmuch2/_database.py |  6 ++--
>  bindings/python-cffi/notmuch2/_message.py  | 42 ++
>  2 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_database.py 
> b/bindings/python-cffi/notmuch2/_database.py
> index 95f59ca0..f14eac78 100644
> --- a/bindings/python-cffi/notmuch2/_database.py
> +++ b/bindings/python-cffi/notmuch2/_database.py
> @@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
>capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
>  if ret not in ok:
>  raise errors.NotmuchError(ret)
> -msg = message.Message(self, msg_pp[0], db=self)
> +msg = message.StandaloneMessage(self, msg_pp[0], db=self)
>  if sync_flags:
>  msg.tags.from_maildir_flags()
>  return self.AddedMessage(
> @@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
>  msg_p = msg_pp[0]
>  if msg_p == capi.ffi.NULL:
>  raise LookupError
> -msg = message.Message(self, msg_p, db=self)
> +msg = message.StandaloneMessage(self, msg_p, db=self)
>  return msg
>  
>  def get(self, filename):
> @@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
>  msg_p = msg_pp[0]
>  if msg_p == capi.ffi.NULL:
>  raise LookupError
> -msg = message.Message(self, msg_p, db=self)
> +msg = message.StandaloneMessage(self, msg_p, db=self)
>  return msg
>  
>  @property
> diff --git a/bindings/python-cffi/notmuch2/_message.py 
> b/bindings/python-cffi/notmuch2/_message.py
> index c5fdbf6d..416ce7ca 100644
> --- a/bindings/python-cffi/notmuch2/_message.py
> +++ b/bindings/python-cffi/notmuch2/_message.py
> @@ -14,7 +14,7 @@ __all__ = ['Message']
>  
>  
>  class Message(base.NotmuchObject):
> -"""An email message stored in the notmuch database.
> +"""An email message stored in the notmuch database retrieved via a query.
>  
>  This should not be directly created, instead it will be returned
>  by calling methods on :class:`Database`.  A message keeps a
> @@ -61,22 +61,10 @@ class Message(base.NotmuchObject):
>  
>  @property
>  def alive(self):
> -if not self._parent.alive:
> -return False
> -try:
> -self._msg_p
> -except errors.ObjectDestroyedError:
> -return False
> -else:
> -return True
> -
> -def __del__(self):
> -self._destroy()
> +return self._parent.alive
>  
>  def _destroy(self):
> -if self.alive:
> -capi.lib.notmuch_message_destroy(self._msg_p)
> -self._msg_p = None
> +pass

I feel like some more description from the commit message could live
here to explain why this isn't destroying the message rather than having
to find it in the commit message.

>  
>  @property
>  def messageid(self):
> @@ -375,6 +363,30 @@ class Message(base.NotmuchObject):
>  if isinstance(other, self.__class__):
>  return self.messageid == other.messageid
>  
> +class 

Re: [PATCH 1/2] Show which notmuch command and version is being used

2019-11-18 Thread Floris Bruynooghe
On Mon 18 Nov 2019 at 07:43 -0400, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>> This add the notmuch version and absolute path of the binary used
>> in the pytest header.  This is nice when running the tests
>> interactively as you get confirmation you're testing the version you
>> thought you were testing.
>> ---
>>  bindings/python-cffi/tests/conftest.py | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/bindings/python-cffi/tests/conftest.py 
>> b/bindings/python-cffi/tests/conftest.py
>> index aa940947..674c7218 100644
>> --- a/bindings/python-cffi/tests/conftest.py
>> +++ b/bindings/python-cffi/tests/conftest.py
>> @@ -10,6 +10,13 @@ import os
>>  import pytest
>>  
>>  
>> +def pytest_report_header():
>> +vers = subprocess.run(['notmuch', '--version'], stdout=subprocess.PIPE)
>> +which = subprocess.run(['which', 'notmuch'], stdout=subprocess.PIPE)
>
> I think it would be better to use "shutil.which()" here, to avoid
> variations in shell builtin vs executable which. If you agree I can make
> the change.

Oh nice, I always forget about shutil.  Please do make the change.

Thanks,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] Move from _add_message to _index_file API

2019-11-17 Thread Floris Bruynooghe
This moves away from the deprecated notmuch_database_add_message API
and instead uses the notmuch_database_index_file API.  This means
instroducing a class to manage the index options and bumping the
library version requirement to 5.1.
---
 bindings/python-cffi/notdb/_build.py| 26 +-
 bindings/python-cffi/notdb/_database.py | 88 -
 bindings/python-cffi/tests/test_database.py | 24 ++
 3 files changed, 131 insertions(+), 7 deletions(-)

diff --git a/bindings/python-cffi/notdb/_build.py 
b/bindings/python-cffi/notdb/_build.py
index 6be7e5b1..642ef77a 100644
--- a/bindings/python-cffi/notdb/_build.py
+++ b/bindings/python-cffi/notdb/_build.py
@@ -12,6 +12,9 @@ ffibuilder.set_source(
 #if LIBNOTMUCH_MAJOR_VERSION < 5
 #error libnotmuch version not supported by notdb
 #endif
+#if LIBNOTMUCH_MINOR_VERSION < 1
+#ERROR libnotmuch  version < 5.1 not supported
+#endif
 """,
 include_dirs=['../../lib'],
 library_dirs=['../../lib'],
@@ -68,6 +71,12 @@ ffibuilder.cdef(
 NOTMUCH_EXCLUDE_FALSE,
 NOTMUCH_EXCLUDE_ALL
 } notmuch_exclude_t;
+typedef enum {
+NOTMUCH_DECRYPT_FALSE,
+NOTMUCH_DECRYPT_TRUE,
+NOTMUCH_DECRYPT_AUTO,
+NOTMUCH_DECRYPT_NOSTASH,
+} notmuch_decryption_policy_t;
 
 // These are fully opaque types for us, we only ever use pointers.
 typedef struct _notmuch_database notmuch_database_t;
@@ -81,6 +90,7 @@ ffibuilder.cdef(
 typedef struct _notmuch_directory notmuch_directory_t;
 typedef struct _notmuch_filenames notmuch_filenames_t;
 typedef struct _notmuch_config_list notmuch_config_list_t;
+typedef struct _notmuch_indexopts notmuch_indexopts_t;
 
 const char *
 notmuch_status_to_string (notmuch_status_t status);
@@ -118,9 +128,10 @@ ffibuilder.cdef(
 notmuch_database_get_revision (notmuch_database_t *notmuch,
const char **uuid);
 notmuch_status_t
-notmuch_database_add_message (notmuch_database_t *database,
-  const char *filename,
-  notmuch_message_t **message);
+notmuch_database_index_file (notmuch_database_t *database,
+ const char *filename,
+ notmuch_indexopts_t *indexopts,
+ notmuch_message_t **message);
 notmuch_status_t
 notmuch_database_remove_message (notmuch_database_t *database,
  const char *filename);
@@ -294,6 +305,15 @@ ffibuilder.cdef(
 notmuch_filenames_move_to_next (notmuch_filenames_t *filenames);
 void
 notmuch_filenames_destroy (notmuch_filenames_t *filenames);
+notmuch_indexopts_t *
+notmuch_database_get_default_indexopts (notmuch_database_t *db);
+notmuch_status_t
+notmuch_indexopts_set_decrypt_policy (notmuch_indexopts_t *indexopts,
+  notmuch_decryption_policy_t 
decrypt_policy);
+notmuch_decryption_policy_t
+notmuch_indexopts_get_decrypt_policy (const notmuch_indexopts_t 
*indexopts);
+void
+notmuch_indexopts_destroy (notmuch_indexopts_t *options);
 """
 )
 
diff --git a/bindings/python-cffi/notdb/_database.py 
b/bindings/python-cffi/notdb/_database.py
index d414082a..8fa631f2 100644
--- a/bindings/python-cffi/notdb/_database.py
+++ b/bindings/python-cffi/notdb/_database.py
@@ -45,6 +45,13 @@ class QueryExclude(enum.Enum):
 ALL = capi.lib.NOTMUCH_EXCLUDE_ALL
 
 
+class DecryptionPolicy(enum.Enum):
+FALSE = capi.lib.NOTMUCH_DECRYPT_FALSE
+TRUE = capi.lib.NOTMUCH_DECRYPT_TRUE
+AUTO = capi.lib.NOTMUCH_DECRYPT_AUTO
+NOSTASH = capi.lib.NOTMUCH_DECRYPT_NOSTASH
+
+
 class Database(base.NotmuchObject):
 """Toplevel access to notmuch.
 
@@ -332,7 +339,17 @@ class Database(base.NotmuchObject):
 def get_directory(self, path):
 raise NotImplementedError
 
-def add(self, filename, *, sync_flags=False):
+def default_indexopts(self):
+"""Returns default index options for the database.
+
+:raises ObjectDestoryedError: if used after destroyed.
+
+:returns: :class:`IndexOptions`.
+"""
+opts = capi.lib.notmuch_database_get_default_indexopts(self._db_p)
+return IndexOptions(self, opts)
+
+def add(self, filename, *, sync_flags=False, indexopts=None):
 """Add a message to the database.
 
 Add a new message to the notmuch database.  The message is
@@ -346,6 +363,11 @@ class Database(base.NotmuchObject):
 :param sync_flags: Whether to sync the known maildir flags to
notmuch tags.  See :meth:`Message.flags_to_tags` for
details.
+:type sync_flags: bool
+:param indexopts: The indexing options, see
+   :meth:`default_indexopts`.  Leave as `None` to use the
+   default options configured in the database.
+:type 

Adressing deprecation warnings

2019-11-17 Thread Floris Bruynooghe
This addresses the deprecation warning pointed out in another
thread on this list.  It's also a nice self-contained example
of adding an API including a new object.

This patch is against the wip/cffi branch, it does not rely
on my other patch against this branch which renames things.
Conflicts with that renaming patch should be trivial if any.


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Python3 cffi bindings

2019-11-17 Thread Floris Bruynooghe
Hi Gaute,

Thanks for trying this out!

On Mon 04 Nov 2019 at 11:27 +0100, Gaute Hope wrote:
> I just checked out the wip/cffi branch on git.notmuch.org with the
> purpose of porting Lieer (https://github.com/gauteh/lieer). There
> seems to be some missing functionality: `Database.get_directory()`
> specifically.

Yeah, I didn't add that yet because I don't fully understand how it
should be used.  Specifically I don't know where one might get a
pathname from to pass to .get_directory() and thus whether the API would
be cleaner to just return a reasonable directory object from whatever
location that might be.  Maybe notmuch_database_get_path() is the only
entrypoint here and you can get further by listing files and directories
from it?  But maybe people then use the filesystem directly to find a
directory and create the directories ad-hoc.

I grepped lieer but I think you only use it in one place?  And if I
understand it correctly you only do this to check if your mailstore/cwd
is inside the notmuch database.  I.e. this is equivalent to checking if
your mailstore/cwd has notmuch2.Database.path as prefix which you could
easily do directly rather than using the FileError exception from
.get_directory().

So is anyone else aware of some code which uses db.get_directory() to
give an idea of how and why this is used?

> I also ran into a couple of warning when building
> (included below).

Thanks for pointing these out.  I guess if the bindings are in the main
repo only the latest library version can be supported without any
further concerns.

> By the way, it does not seem that the API is very far from the
> previous python API. If it is close enough, perhaps it is possible to
> get away with a bug version bump in the bindings rather than creating
> a new package. I understand the need for a new package, but it would
> be nice if we could avoid the future confusion of two python binding
> packages (if at all possible).

While I'm glad to hear that you think a migration wouldn't be to painful
for you I am very weary of knowingly breaking APIs.  I'd rather have
people have an easy migration rather than unexpected breakage after an
upgrade.


Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Rename and small tweaks to wip/cffi

2019-11-17 Thread Floris Bruynooghe
Hi,

This is a patch against the wip/cffi branch.  It does the rename
which was discussed before and it adds a small improvment (to me)
to the test runner to show which notmuch version is being tested.

Let me know if this the right or wrong way to contribute to this
branch.

Cheers,
Floris


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 2/2] Rename package to notmuch2

2019-11-17 Thread Floris Bruynooghe
s/python-cffi/notdb/_errors.py 
b/bindings/python-cffi/notmuch2/_errors.py
similarity index 99%
rename from bindings/python-cffi/notdb/_errors.py
rename to bindings/python-cffi/notmuch2/_errors.py
index 924e722f..1c88763b 100644
--- a/bindings/python-cffi/notdb/_errors.py
+++ b/bindings/python-cffi/notmuch2/_errors.py
@@ -1,4 +1,4 @@
-from notdb import _capi as capi
+from notmuch2 import _capi as capi
 
 
 class NotmuchError(Exception):
diff --git a/bindings/python-cffi/notdb/_message.py 
b/bindings/python-cffi/notmuch2/_message.py
similarity index 99%
rename from bindings/python-cffi/notdb/_message.py
rename to bindings/python-cffi/notmuch2/_message.py
index 9b2b037f..bb561426 100644
--- a/bindings/python-cffi/notdb/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -4,10 +4,10 @@ import os
 import pathlib
 import weakref
 
-import notdb._base as base
-import notdb._capi as capi
-import notdb._errors as errors
-import notdb._tags as tags
+import notmuch2._base as base
+import notmuch2._capi as capi
+import notmuch2._errors as errors
+import notmuch2._tags as tags
 
 
 __all__ = ['Message']
diff --git a/bindings/python-cffi/notdb/_query.py 
b/bindings/python-cffi/notmuch2/_query.py
similarity index 93%
rename from bindings/python-cffi/notdb/_query.py
rename to bindings/python-cffi/notmuch2/_query.py
index 613aaf12..1db6ec96 100644
--- a/bindings/python-cffi/notdb/_query.py
+++ b/bindings/python-cffi/notmuch2/_query.py
@@ -1,8 +1,8 @@
-from notdb import _base as base
-from notdb import _capi as capi
-from notdb import _errors as errors
-from notdb import _message as message
-from notdb import _thread as thread
+from notmuch2 import _base as base
+from notmuch2 import _capi as capi
+from notmuch2 import _errors as errors
+from notmuch2 import _message as message
+from notmuch2 import _thread as thread
 
 
 __all__ = []
diff --git a/bindings/python-cffi/notdb/_tags.py 
b/bindings/python-cffi/notmuch2/_tags.py
similarity index 99%
rename from bindings/python-cffi/notdb/_tags.py
rename to bindings/python-cffi/notmuch2/_tags.py
index a25a2264..fe422a79 100644
--- a/bindings/python-cffi/notdb/_tags.py
+++ b/bindings/python-cffi/notmuch2/_tags.py
@@ -1,8 +1,8 @@
 import collections.abc
 
-import notdb._base as base
-import notdb._capi as capi
-import notdb._errors as errors
+import notmuch2._base as base
+import notmuch2._capi as capi
+import notmuch2._errors as errors
 
 
 __all__ = ['ImmutableTagSet', 'MutableTagSet', 'TagsIter']
diff --git a/bindings/python-cffi/notdb/_thread.py 
b/bindings/python-cffi/notmuch2/_thread.py
similarity index 97%
rename from bindings/python-cffi/notdb/_thread.py
rename to bindings/python-cffi/notmuch2/_thread.py
index e1ef6b07..a754749f 100644
--- a/bindings/python-cffi/notdb/_thread.py
+++ b/bindings/python-cffi/notmuch2/_thread.py
@@ -1,11 +1,11 @@
 import collections.abc
 import weakref
 
-from notdb import _base as base
-from notdb import _capi as capi
-from notdb import _errors as errors
-from notdb import _message as message
-from notdb import _tags as tags
+from notmuch2 import _base as base
+from notmuch2 import _capi as capi
+from notmuch2 import _errors as errors
+from notmuch2 import _message as message
+from notmuch2 import _tags as tags
 
 
 __all__ = ['Thread']
diff --git a/bindings/python-cffi/setup.py b/bindings/python-cffi/setup.py
index 7baf63cf..37918e3d 100644
--- a/bindings/python-cffi/setup.py
+++ b/bindings/python-cffi/setup.py
@@ -2,7 +2,7 @@ import setuptools
 
 
 setuptools.setup(
-name='notdb',
+name='notmuch2',
 version='0.1',
 description='Pythonic bindings for the notmuch mail database using CFFI',
 author='Floris Bruynooghe',
@@ -10,7 +10,7 @@ setuptools.setup(
 setup_requires=['cffi>=1.0.0'],
 install_requires=['cffi>=1.0.0'],
 packages=setuptools.find_packages(exclude=['tests']),
-cffi_modules=['notdb/_build.py:ffibuilder'],
+cffi_modules=['notmuch2/_build.py:ffibuilder'],
 classifiers=[
 'Development Status :: 3 - Alpha',
 'Intended Audience :: Developers',
diff --git a/bindings/python-cffi/tests/test_base.py 
b/bindings/python-cffi/tests/test_base.py
index b6d3d62c..d3280a67 100644
--- a/bindings/python-cffi/tests/test_base.py
+++ b/bindings/python-cffi/tests/test_base.py
@@ -1,7 +1,7 @@
 import pytest
 
-from notdb import _base as base
-from notdb import _errors as errors
+from notmuch2 import _base as base
+from notmuch2 import _errors as errors
 
 
 class TestNotmuchObject:
diff --git a/bindings/python-cffi/tests/test_database.py 
b/bindings/python-cffi/tests/test_database.py
index 02de0f41..e3a8344d 100644
--- a/bindings/python-cffi/tests/test_database.py
+++ b/bindings/python-cffi/tests/test_database.py
@@ -5,10 +5,10 @@ import pathlib
 
 import pytest
 
-import notdb
-import notdb._errors as errors
-import notdb._database as dbmod
-import notdb._message as message
+import notmuch2
+import notmuch2._errors as errors
+import notmuch2._database as dbmod
+import notmuch2._

[PATCH 1/2] Show which notmuch command and version is being used

2019-11-17 Thread Floris Bruynooghe
This add the notmuch version and absolute path of the binary used
in the pytest header.  This is nice when running the tests
interactively as you get confirmation you're testing the version you
thought you were testing.
---
 bindings/python-cffi/tests/conftest.py | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/bindings/python-cffi/tests/conftest.py 
b/bindings/python-cffi/tests/conftest.py
index aa940947..674c7218 100644
--- a/bindings/python-cffi/tests/conftest.py
+++ b/bindings/python-cffi/tests/conftest.py
@@ -10,6 +10,13 @@ import os
 import pytest
 
 
+def pytest_report_header():
+vers = subprocess.run(['notmuch', '--version'], stdout=subprocess.PIPE)
+which = subprocess.run(['which', 'notmuch'], stdout=subprocess.PIPE)
+return ['{} ({})'.format(vers.stdout.decode(errors='replace').strip(),
+ which.stdout.decode(errors='replace').strip())]
+
+
 @pytest.fixture(scope='function')
 def tmppath(tmpdir):
 """The tmpdir fixture wrapped in pathlib.Path."""
-- 
2.24.0

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: python CFFI bindings integration into notmuch build/test

2019-11-17 Thread Floris Bruynooghe
On Sat 16 Nov 2019 at 10:51 -0500, David Bremner wrote:

> Floris Bruynooghe  writes:
>> Anyway, this looks good.  Would you like some changes, e.g. the rename
>> to notmuch2 or so as patches?  What's the next step.
>
> If you could look at the rename that would be great. Patches on top of
> wip/cffi, or a ref for me to pull both sound fine.
>
>> Kind of unrelated, but in my attempt to run the full notmuch test suite
>> (cd tests; make test) I somehow ended up with lots of weird tags in my
>> notmuch database and made the emacs UI pretty horrible - though it seems
>> I haven't lost any email.  I'm sure that was my fault somehow even
>> though I was just trying to follow the readme.  But if anyone has any
>> hints how to recover from this that could save me some time :)
>
> Hmm. I've never encountered that, but maybe it has to do with killing
> the environment? The test harness uses the environment variable
> NOTMUCH_CONFIG to point to a test database, and if that was deleted, it
> would look in ~/.notmuch-config.

Yeah, I probably managed to make it use ~/.notmuch-config somehow.

> Still that would really only be tags from your pytest tests, and you'd
> probably recognize those?

The were a bunch of weird characters that looked like unicode
test-cases, the pytest tests don't try to test notmuch only the bindings
so doesn't really make up weird strings IIRC.  I recovered with
something like "notmuch dump | cleanup_tags.py | notmuch restore", oh
well.

I still have a bunch of non-existing messages with weird unicode message
IDs in them, but no associated filename just an empty string.  AFAIK
notmuch_database_remove_message() needs a filename though, I've tried
removing the empty file which does not fail but doesn't clear those
database entries.  The messages are not ghosts either.  Any idea how I
might be able to clear those bad messages from the database?

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: python CFFI bindings integration into notmuch build/test

2019-11-14 Thread Floris Bruynooghe
On Thu 14 Nov 2019 at 23:24 +0100, Floris Bruynooghe wrote:

> On Thu 14 Nov 2019 at 22:20 +0200, Tomi Ollila wrote:
>> In git://notmuchmail.org/git/notmuch David has ref origin/wip/cffi
>> which contains related changes -- You can fetch the code while waiting
>> for more collaboration instructions from David.
>
> Aha, thanks!

So I can run the tests using `make` and `cd tests;
./T-391-pytest-cffi.sh`.  I can also run the tests still individually
using something like:

make
set PATH (pwd) $PATH  # I use fish
pushd bindings/python-cffi
pew workon notmuch  # how I manage python virtualenvs
pip install -e .
pip install pytest pytest-cov
pytest test/test_base.py

And all works.  I added a little function to the conftest.py to show
which notmuch it's testing with:

def pytest_report_header():
vers = subprocess.run(['notmuch', '--version'],
  capture_output=True, text=True)
which = subprocess.run(['which', 'notmuch'],
   capture_output=True, text=True)
return ['{} ({})'.format(vers.stdout.strip(), which.stdout.strip())]

Maybe you find that useful too.

Anyway, this looks good.  Would you like some changes, e.g. the rename
to notmuch2 or so as patches?  What's the next step.



Kind of unrelated, but in my attempt to run the full notmuch test suite
(cd tests; make test) I somehow ended up with lots of weird tags in my
notmuch database and made the emacs UI pretty horrible - though it seems
I haven't lost any email.  I'm sure that was my fault somehow even
though I was just trying to follow the readme.  But if anyone has any
hints how to recover from this that could save me some time :)

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: python CFFI bindings integration into notmuch build/test

2019-11-14 Thread Floris Bruynooghe
On Thu 14 Nov 2019 at 22:20 +0200, Tomi Ollila wrote:
> In git://notmuchmail.org/git/notmuch David has ref origin/wip/cffi
> which contains related changes -- You can fetch the code while waiting
> for more collaboration instructions from David.

Aha, thanks!

Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: python CFFI bindings integration into notmuch build/test

2019-11-14 Thread Floris Bruynooghe
Hi,

Thanks for carrying on with this!

I'm a little confused with how to follow this, is the current state of
the code somewhere in a repo/branch where I can try things out and make
changes from?

On Tue 05 Nov 2019 at 22:22 -0400, David Bremner wrote:

> Tomi Ollila  writes:
>
>
>> alternative:
>>
>>...
>>print('Invoking: {}'.format(' '.join(cmd)))
>> 
>>def preexec_fn(): os.environ['NOTMUCH_CONFIG'] = str(cfg_fname) 
>>
>>proc = subprocess.run(cmd, timeout=5, preexec_fn=preexec_fn)
>>...
>>
>> The unix fork ... (here preexec_fn called in child) ... exec model is
>> superior to any other alternative ! =D
>
> I don't consider myself a good judge of python style, so I'll defer to
> Floris on that one.

I'd have gone with what David wrote to be fair, mostly because it's
pretty simple to see what's going on where for the preexec_fn I'd have
to look at the docs.  Not sure if this has much to do with Python style
though, more with how much you like Unix tricks.


Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Python3 cffi bindings

2019-10-25 Thread Floris Bruynooghe
On Tue 22 Oct 2019 at 13:32 -0300, David Bremner wrote:

> David Bremner  writes:
>
>> The LD_LIBRARY_PATH is already set by the test harness, as is PATH (to
>> find notmuch). It looks like your function notmuch is not respecting
>> PATH (see attached log). if I hack something like
>>
>> diff --git a/bindings/python-cffi/tests/conftest.py 
>> b/bindings/python-cffi/tests/conftest.py
>> index 1b7bbc35..ac17397c 100644
>> --- a/bindings/python-cffi/tests/conftest.py
>> +++ b/bindings/python-cffi/tests/conftest.py
>> @@ -31,7 +31,7 @@ def notmuch(maildir):
>>  accidentally do this in the unittests.
>>  """
>>  cfg_fname = maildir.path / 'notmuch-config'
>> -cmd = ['notmuch'] + list(args)
>> +cmd = ['../../notmuch'] + list(args)
>>  print('Invoking: {}'.format(' '.join(cmd)))
>>  proc = subprocess.run(cmd,
>>
>
> I think I figured it out. Your 'run' function completely overrides the
> environment. But just adding PATH back seems to do the trick. I'm not
> sure if this is the most idomatic change, but it works:
>
> diff --git a/bindings/python-cffi/tests/conftest.py 
> b/bindings/python-cffi/tests/conftest.py
> index 1b7bbc35..6a81aa18 100644
> --- a/bindings/python-cffi/tests/conftest.py
> +++ b/bindings/python-cffi/tests/conftest.py
> @@ -33,9 +33,11 @@ def notmuch(maildir):
>  cfg_fname = maildir.path / 'notmuch-config'
>  cmd = ['notmuch'] + list(args)
>  print('Invoking: {}'.format(' '.join(cmd)))
>proc = subprocess.run(cmd,
>timeout=5,
> -  env={'NOTMUCH_CONFIG': str(cfg_fname)})
> +  
> env={'PATH':os.environ["PATH"],'NOTMUCH_CONFIG': str(cfg_fname)})
>  proc.check_returncode()
>  return run
>  

This seems reasonable, perhaps even a "env = os.environ.copy();
env['NOTMUCH_CONFIG'] = src(cfg_fname)" is better here so that
LD_LIBRARY_PATH and anything else is kept around.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Python3 cffi bindings

2019-10-17 Thread Floris Bruynooghe
On Mon 14 Oct 2019 at 09:42 -0300, David Bremner wrote:

> David Bremner  writes:
>
>> The shim in
>> T391-python-cffi.sh doesn't work for me, it doesn't manage to set
>> PYTHONPATH so that notdb is importable.

Ah yes, I tested this shim while activating a venv with the extension
installed using `pip -e .`.

> I should have mentioned that if I manually set python path with
> something like
>
> $ PYTHONPATH=`pwd`/build/lib.linux-x86_64-3.7:$PYTHONPATH pytest-3
>
> it works OK.  Is there a simple/reliable way of calculating the path
> lib.linux-x86_64-3.7?

It is possible to run this without installing, but it does need a build
step since cffi (in the mode used - which is the recommended mode) needs
to build an extension module.  I did something like this, using my
debian testing system-installed python

$ export PYTHONPATH=$(pwd)/bindings/python-cffi
$ pushd bindings/python-cffi
$ python3 notdb/_build.py  # creates notdb/_capi.cpython-37m-x86_64-linux-gnu.so
$ popd
$ pushd test
$ ./T391-pytest.sh

Does that more or less work?  One problem with this is that it will pick
up the system-wide installed notmuch though.  I guess the way to change
this is by tweaking CFLAGS=-I... LDFLAGS=-L... or so when building?  But
than you also have the whole RPATH/LD_LIBRARY_PATH stuff going on as
well.  Does notmuch abstract any of this away already for it's test
suite?

Cheers,
Floris

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Python3 cffi bindings

2019-10-09 Thread Floris Bruynooghe
On Tue 08 Oct 2019 at 19:24 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>> Anyway, I found the code, checked things work, updated tests on new
>> python versions, added a very basic intergration with the test
>> framework and squashed the commits.  Otherwise the attached patch
>> is just a plain dump of the current state so interested people have
>> at least a copy of the code again which can be made to work.
>
> I think you missed the attachement. Other than that, sounds interesting ;).

I used git send-email as per the contributing nodes, so it was supposed
to be a followup email.  It does seem like I got a message back saying
the patch mail is being held in the moderator queue as it's too large...
Perhaps you have moderation powers?

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Python3 cffi bindings

2019-10-08 Thread Floris Bruynooghe
Hi all,

IIRC there was a thread in August about another attempt at bringing
the CFFI-based bindings on board as a Python3-only version.  I
believe there was a desire to re-name things but my searching-fu is
failing me and I can no longer find the email thread.

Anyway, I found the code, checked things work, updated tests on new
python versions, added a very basic intergration with the test
framework and squashed the commits.  Otherwise the attached patch
is just a plain dump of the current state so interested people have
at least a copy of the code again which can be made to work.

IIRC this probably wants to be renamed to "notmuch2" instead of
"notdb".  Otherwise I'm pretty sure this doesn't cover all the
current features either.

So maybe this can be used as a start to figure out how to merge
this if there's still an interest in this.

Cheers,
Floris


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: segfault using python bindings

2019-08-26 Thread Floris Bruynooghe
On Wed 21 Aug 2019 at 12:02 -0400, Daniel Kahn Gillmor wrote:

> On Tue 2019-08-20 19:20:30 +0200, Floris Bruynooghe wrote:
>> For path series what did you have in mind?  One single patch with the
>> whole lot?  The original history at https:://github/flub/notdb?
>> Something in between?
>
> I would be happy with a series of patches, but the cited github history
> has only one commit in the first place :P

hah!  Perhaps the actual history is somewhere in a hg repo, but I can't
even find that.  Not that it matters much.

>> I also recall some comments about the naming not being liked much
>> (notdb).  I'm open to some bikeshedding on the naming if it bothers
>> people.  I was only aiming for something short but under a different
>> import name, which are probably still useful features to keep.
>
> fwiw, "notdb" doesn't really resonate with me, but i wouldn't block you
> if you decided to stick with it.
>
> I'd generally prefer something like "notmuch2" because it makes it much
> clearer that it is associated with notmuch, and that it is the successor
> to the original notmuch bindings.

>From the ensuing conversation I'm concluding that "notmuch2" is the most
liked option, so I'll go with that (and yes, I share many of the
disappointed sentiments raised on that naming convention, but it's how
things are in python).


I'll try and send a first patch series in the coming week.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: segfault using python bindings

2019-08-20 Thread Floris Bruynooghe
On Thu 15 Aug 2019 at 09:28 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>> On Wed 14 Aug 2019 at 16:20 -0300, David Bremner wrote:
>>>
>>> Can you remind me what the percieved blockers are for merging into the
>>> main notmuch tree? I'm less hung up on python2 compatibility than I used
>>> to be, fwiw. I'd be ok with shipping the old python2 bindings in contrib
>>> for a bit for those who still need/want them, but concentrate our
>>> maintenance effort on the cffi bindings.
>>
>> IIRC it was mostly about how to support transitioning from one API to
>> the other since currently there's no compatibility.  I guess there's
>> nothing stopping one from using both APIs in one codebase though, AFAIK
>> Xapian handles the required locking.  But some of the discussions
>> suggested being able to create a new Message object from an old one etc,
>> allowing you to do more mixing during a transition period.  This is the
>> part that I said is possible but a lot of work and questionable if no
>> one thought they'd be using it.
>>
>
> Ah right. Well, given the impending removal of python2 from various
> places (e.g. debian testing), I don't think we should be that
> fussy/ambitious.
>
> I'd propose
>
> - add the new cffi based bindings, using a distinct name (as you already
>   have).
>
> - deprecate the old ones
>
> - port any internal dependencies to the new bindings
>
> - finally drop the old bindings or move them to contrib/ for people slow
>   in switching.
>
> I think for the first step we only need a reasonable looking patch
> (series?) from you.

Sounds reasonable, just a quick note that I'm on holiday at the moment
and generally won't be too quick.  But I guess there's no rush.  I was
also trying to improve the docs but got sidetracked at some point.
There should be already reasonable good docstrings though.

For path series what did you have in mind?  One single patch with the
whole lot?  The original history at https:://github/flub/notdb?
Something in between?

I also recall some comments about the naming not being liked much
(notdb).  I'm open to some bikeshedding on the naming if it bothers
people.  I was only aiming for something short but under a different
import name, which are probably still useful features to keep.

Cheers,
Floris

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: segfault using python bindings

2019-08-15 Thread Floris Bruynooghe
On Wed 14 Aug 2019 at 16:20 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>> These are at https://github.com/flub/notmuch/tree/cffi/bindings/python-cffi
>>
>> I'm not really convinced of the way forward last time it was discussed
>> on how to get them merged into notmuch itself so have failed to put in
>> the not insignificant effort.
>>
>> I've since wondered if just getting them standalone on pypi is perhaps a
>> useful service in the mean time as it's relatively little effort.  And
>> if there eventually is a desire again to get them merged in some way
>> that could still be done.
>
> Can you remind me what the percieved blockers are for merging into the
> main notmuch tree? I'm less hung up on python2 compatibility than I used
> to be, fwiw. I'd be ok with shipping the old python2 bindings in contrib
> for a bit for those who still need/want them, but concentrate our
> maintenance effort on the cffi bindings.

IIRC it was mostly about how to support transitioning from one API to
the other since currently there's no compatibility.  I guess there's
nothing stopping one from using both APIs in one codebase though, AFAIK
Xapian handles the required locking.  But some of the discussions
suggested being able to create a new Message object from an old one etc,
allowing you to do more mixing during a transition period.  This is the
part that I said is possible but a lot of work and questionable if no
one thought they'd be using it.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: segfault using python bindings

2018-11-16 Thread Floris Bruynooghe
On Fri 16 Nov 2018 at 07:15 -0500, Daniel Kahn Gillmor wrote:

> On Fri 2018-11-16 06:27:12 -0400, David Bremner wrote:
>> Floris Bruynooghe  writes:
>>
>>> These are at https://github.com/flub/notmuch/tree/cffi/bindings/python-cffi
>>>
>>> I'm not really convinced of the way forward last time it was discussed
>>> on how to get them merged into notmuch itself so have failed to put in
>>> the not insignificant effort.
>>>
>>> I've since wondered if just getting them standalone on pypi is perhaps a
>>> useful service in the mean time as it's relatively little effort.  And
>>> if there eventually is a desire again to get them merged in some way
>>> that could still be done.
>>
>> What effort are you referring to specifically? Integration with the
>> notmuch test suite?
>
> My recollection is that the main question was about supporting the old
> python interface with the new bindings, so that consumers would have a
> smooth upgrade path.  Is that not right?

That's indeed what I was referring to, integration with the test suite
is fine as was discussed last time imho.

> Floris, i really appreciate the work you put in here, and i'd love to
> see notmuch be able to adopt it directly.   can we figure out what is
> needed to take these changes?

Thanks.  I think mainly when the technical approach was discussed [0] no
actual users of the current Python API weighed in with if they'd be
interested in a migration of the API and if so, how it might work for
them.  So while the gradual approach described there is technically
somewhat nice I have no idea if anyone would benefit from it, or whether
the benefits outweigh all the work involved.

As I was recently thinking however, maybe there's nothing wrong with new
bindings being published as a 3rd party package on pypi.  It'd make it
more discoverable and if people start to adopt it maybe there'd be more
demand for integrating it back with more clarity over how smooth a
transition path needs to be.

Also lastly an apology.  I could have done more to move this forward,
but I simply haven't found^Wmade the time for it.

Cheers,
Floris


[0] id:py3imv2cjp28@devork.be
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: segfault using python bindings

2018-11-16 Thread Floris Bruynooghe
On Fri 16 Nov 2018 at 06:29 -0400, David Bremner wrote:

> Brian May  writes:
>
>> Floris Bruynooghe  writes:
>>
>>> I've since wondered if just getting them standalone on pypi is perhaps a
>>> useful service in the mean time as it's relatively little effort.  And
>>> if there eventually is a desire again to get them merged in some way
>>> that could still be done.
>>
>> Standalone on pypi would be my preferred option.
>>
>> It is defacto Python standard to refer to all dependancies in something
>> like requirements.txt or Pipfile from pypi.
>
> As I mentioned last time this was discussed, the python bindings are
> currently more or less a core part of notmuch as both the test
> suite and developement need them.

Sure, I think pypi publishing is orthogonal to this however.  Either or
both versions of the bindings could be published on pypi in addition to
being in the main repo.  As Brian mentions it would improve
discoverability and improves integration on the python side.  There's
even tooling to bundle the library these days with the manylinux1
wheels.  So there's no need to stop anyone who'd like to do this.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: segfault using python bindings

2018-11-15 Thread Floris Bruynooghe
Hi,

On Sun 11 Nov 2018 at 16:16 -0400, David Bremner wrote:

> David Čepelík  writes:
>
>> Hello Notmuch devs,
>>
>> I'm facing an issue trying to use the Python bindings. This trivial
>> piece of code segfaults:
>>
>> import notmuch
>
> I don't remember the details [1], but there are known conflicts between
> recent versions of python3 and the way the notmuch python bindings
> manage memory. So it could be that. There was also an initiative to
> rewrite at (python3 only?) version of the bindings that did not have
> this problem. I haven't heard much about that recently.

These are at https://github.com/flub/notmuch/tree/cffi/bindings/python-cffi

I'm not really convinced of the way forward last time it was discussed
on how to get them merged into notmuch itself so have failed to put in
the not insignificant effort.

I've since wondered if just getting them standalone on pypi is perhaps a
useful service in the mean time as it's relatively little effort.  And
if there eventually is a desire again to get them merged in some way
that could still be done.


Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 2/2] test: pytest runner for the test suite

2018-04-13 Thread Floris Bruynooghe
On Sun 08 Apr 2018 at 19:14 -0300, David Bremner wrote:

> Floris Bruynooghe <f...@devork.be> writes:
>
>> This series looks good to me, would be great to have!  Do you want to
>> commit them this or should I just incorporate it and submit together
>> with tests once actual tests exist.  You could always commit with a ``def
>> test_dummy(): assert True`` or something if you like.
>>
>
> For now, why don't you just incorporate them. Maybe you'll discover some
> issues with them as you work up some real tests.

Sure, that works.

> BTW, it seems like a
> reasonable plan to get a set of unit tests for the existing bindings
> first to help with migration. Is that what you had in mind?

Yes that's what I had in mind.  To safely swap out ctypes for cffi
adding tests to ensure the existing API behaviour remains is a must.

Which reminds me that it's probably worth calling this out explicitly
again: this works implies that cffi will become an external dependency
[0] for the existing bindings.  Just want to make sure this doesn't
become an issue further down the line.

Cheers,
Floris

[0] Technically only on cpython, it's bundled with pypy.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 2/2] test: pytest runner for the test suite

2018-04-08 Thread Floris Bruynooghe
This series looks good to me, would be great to have!  Do you want to
commit them this or should I just incorporate it and submit together
with tests once actual tests exist.  You could always commit with a ``def
test_dummy(): assert True`` or something if you like.

Thanks!
Floris


On Sat 07 Apr 2018 at 18:39 -0300, David Bremner wrote:

> The 'test_subtest_known_broken' should be removed when there are
> actual tests to run.
>
> Based on a function from Tomi [1]
>
> [1]: id:m2r2nq23r9@guru.guru-group.fi
> ---
>  test/T391-pytest.sh | 14 ++
>  1 file changed, 14 insertions(+)
>  create mode 100755 test/T391-pytest.sh
>
> diff --git a/test/T391-pytest.sh b/test/T391-pytest.sh
> new file mode 100755
> index ..9ac7aabe
> --- /dev/null
> +++ b/test/T391-pytest.sh
> @@ -0,0 +1,14 @@
> +#!/usr/bin/env bash
> +test_description="python bindings (pytest)"
> +. $(dirname "$0")/test-lib.sh || exit 1
> +
> +test_require_external_prereq ${NOTMUCH_PYTHON}
> +
> +for bin in ${NOTMUCH_PYTEST_PYTHONS}; do
> +test_begin_subtest "pytest ($bin)"
> +  test_subtest_known_broken
> +   
> PYTHONPATH="$NOTMUCH_SRCDIR/bindings/python${PYTHONPATH:+:$PYTHONPATH}" \
> + test_expect_success "$bin -m pytest 
> $NOTMUCH_SRCDIR/bindings/python"
> +done
> +
> +test_done
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


experimenting with pytest tests

2018-04-07 Thread Floris Bruynooghe
Hi,

From another conversation on this list I've dug up my earlier attempts
at integrating a pytest run into the notmuch testing suite.  I'd be
grateful for some guidance on whether this is the right way to go about
things.

Here's the diff of configure:

diff --git a/configure b/configure
index c5e2ffed..8aa57b83 100755
--- a/configure
+++ b/configure
@@ -567,6 +567,28 @@ if [ $have_python -eq 0 ]; then
 errors=$((errors + 1))
 fi
 
+check_python() {
+local bin=$1
+local var=$(echo $bin | tr -d '.')
+printf "Checking for $bin (with: pytest)... "
+if command -v $bin > /dev/null; then
+if $bin -c 'import pytest' >/dev/null 2>&1; then
+eval have_$var=1
+eval $var=$bin
+printf "Yes.\n"
+else
+printf "No (skipping $bin tests).\n"
+fi
+else
+printf "No (skipping $bin tests).\n"
+fi
+}
+
+check_python python2.7
+check_python python3.5
+check_python python3.6
+check_python pypy3.5
+
 printf "Checking for valgrind development files... "
 if pkg-config --exists valgrind; then
 printf "Yes.\n"
@@ -1209,6 +1231,10 @@ NOTMUCH_HAVE_MAN=$((have_sphinx))
 
 # Name of python interpreter
 NOTMUCH_PYTHON=${python}
+NOTMUCH_PYTHON27=${python27-}
+NOTMUCH_PYTHON35=${python35-}
+NOTMUCH_PYTHON36=${python36-}
+NOTMUCH_PYPY35=${pypy35-}


And I was then also trying to introduce a test/T391-pytest.sh file.
What I'm trying to do in this file, but it doesn't currently work, is to
have one test, with 4 subtests where each subtest is a pytest run with a
particular python version.  But if the NOTMUCH_PYTHON27 (etc) is not
found in sh.config then the subtest should be skipped.  I'm not really
familiar enough with test-lib.sh to do this correctly without a bunch of
more looking into it, but maybe someone else knows how to do this?
Anyway, here my failing attempt:

#!/usr/bin/env bash
test_description="python unittests"
. ./test-lib.sh || exit 1


test_require_external_prereq "${NOTMUCH_PYTHON27}" && {
test_begin_subtest "${NOTMUCH_PYTHON27}"
(
cd "$NOTMUCH_SRCDIR/bindings/python"
PYTHONPATH=".${PYTHONPATH:+:$PYTHONPATH}" \
$NOTMUCH_PYTHON27 -m pytest
)
test_expect_equal $? 0
}


test_require_external_prereq ${NOTMUCH_PYTHON35} && {
test_begin_subtest "${NOTMUCH_PYTHON35}"
(
cd "$NOTMUCH_SRCDIR/bindings/python"
PYTHONPATH=".${PYTHONPATH:+:$PYTHONPATH}" \
$NOTMUCH_PYTHON35 -m pytest
)
test_expect_equal $? 0
}


test_done


Any tips on whether this is the right direction?

Many thanks,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: New Python bindings (was: Crash with Python bindings)

2018-03-28 Thread Floris Bruynooghe
On Wed, Mar 28 2018, David Bremner wrote:

> Brian May  writes:
>
>> I can into this thread late. However, my priorities for python bindings
>> would be:
>
> [...]
>> * Packages should be available from pypi.python.org
>>
>
> We tried this before, and it didn't work out very well. Bindings tend to
> depend on a strict matching of versions with the underlying library, so
> distributing them seperately doesn't really make sense to me. You need
> the underlying libraries, so why not get the matching bindings from the
> same place?  We found that the situation was exacerbated by the fact
> that no-one cared about updating the bindings on pypi.

I did build a

#if LIBNOTMUCH_MAJOR_VERSION < 5
#error libnotmuch version not supported by notdb
#endif

into my current bindings which kind of allows you to do this to some,
hopefully reasonable, level.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: New Python bindings (was: Crash with Python bindings)

2018-03-28 Thread Floris Bruynooghe
On Wed, Mar 28 2018, Brian May wrote:

> Justus Winter  writes:
>
>>> This is exactly what I have fixed in my alternative bindings which I
>>> created around the end of last year [0].  So we do have an idea of how
>>> to fix this, at the time I said I do believe that it's possible to also
>>> do this for the existing bindings even though it is a lot of work.
>>> After some talking between dkg and me we got to a way forward which
>>> proposed this, but I must admit that after messing a little with getting
>>> a pytest run integrated into the notmuch test-suite instead of using tox
>>> I lost momentum on the project and didn't advance any further.
>>
>> I'm sorry that I didn't speak up when you announced your work.  I'm
>> actually excited about a new set of bindings for Python.  I agree with
>> using cffi.  I briefly looked at the code, and I believe it is much
>> nicer than what we currently have.
>
> I can into this thread late. However, my priorities for python bindings
> would be:
>
> * I don't care about anything less then Python 3.5.
> * Reliable Python 3.6 support important.
> * Packages should be available from pypi.python.org

Well, the 1st two should already be covered on my
https://github.com/flub/notmuch/tree/cffi branch.  There's obviously
still room for improvement.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: New Python bindings

2018-03-28 Thread Floris Bruynooghe
On Wed, Mar 28 2018, Justus Winter wrote:

> Floris Bruynooghe <f...@devork.be> writes:
>
>> On Wed, Mar 21 2018, Justus Winter wrote:
>>>
>>> Floris Bruynooghe <f...@devork.be> writes:
>>>
>>>> This is exactly what I have fixed in my alternative bindings which I
>>>> created around the end of last year [0].  So we do have an idea of how
>>>> to fix this, at the time I said I do believe that it's possible to also
>>>> do this for the existing bindings even though it is a lot of work.
>>>> After some talking between dkg and me we got to a way forward which
>>>> proposed this, but I must admit that after messing a little with getting
>>>> a pytest run integrated into the notmuch test-suite instead of using tox
>>>> I lost momentum on the project and didn't advance any further.
>>>
>>> I'm sorry that I didn't speak up when you announced your work.  I'm
>>> actually excited about a new set of bindings for Python.  I agree with
>>> using cffi.  I briefly looked at the code, and I believe it is much
>>> nicer than what we currently have.
>>
>> Nice to hear, thanks!
>
> Thanks for all the work :)
>
>>> I trust that it works fine with Python 3, does it?
>>
>> The version I made so far *only* works on Python 3.  Mostly because it
>> was easier, but it also allows some API niceties.
>
> Reasonable choice.  Which versions of Python 3 are supported?  I am also
> writing bindings and I am wondering which versions to target.

Personally I consider python3.5, pypy3.5 and python3.6 the ones to
target if I have no other constraints, which was the case here.  For
upstreaming into notmuch proper there are naturally other constraints
;-)  But unless you need something specific I think 3.4 is when py3k
became the better version than 2.7, everything below that is probably
not worth it.  All IMHO obviously.

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: pytest integration for the notmuch test suite

2018-03-26 Thread Floris Bruynooghe
On Sun, Mar 25 2018, David Bremner wrote:

> Here's one approach. A given pytest "file" can be embedded in a normal
> (for us) test script.  As I write this, it occurs to me you might be
> thinking of embedding unit tests in the bindings source files; that
> would be easy to add, something along the lines of
>
> test_begin_subtest "python bindings embedded unit tests"
> test_expect_success "${NOTMUCH_PYTEST} 
> ${NOTMUCH_SRCDIR}/bindings/python/notmuch"

I was trying to construct something where a full pytest run on one
python version was one subtest.  For granularity I think treating an
entire pytest run as a subtest with just checking the return code should
be sufficient,
e.g. `python2.7 -m pytest ${NOTMUCH_SRCDIR}/bindings/python/notmuch`.

But the whole test in this case would be this same subtest but once with
python2.7, python3.5, python3.6 etc.

What do you think of this?

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 1/3] configure: check for pytest binary

2018-03-26 Thread Floris Bruynooghe
On Sun, Mar 25 2018, David Bremner wrote:

> This is to support future use of pytest in the test suite

Thanks for having a go at this!

> ---
>  configure | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/configure b/configure
> index b177b141..ab45878d 100755
> --- a/configure
> +++ b/configure
> @@ -62,6 +62,7 @@ CXXFLAGS=${CXXFLAGS:-\$(CFLAGS)}
>  LDFLAGS=${LDFLAGS:-}
>  XAPIAN_CONFIG=${XAPIAN_CONFIG:-}
>  PYTHON=${PYTHON:-}
> +PYTEST=${PYTEST:-}
>  
>  # We don't allow the EMACS or GZIP Makefile variables inherit values
>  # from the environment as we do with CC and CXX above. The reason is
> @@ -118,6 +119,8 @@ Other environment variables can be used to control 
> configure itself,
>   library. [$XAPIAN_CONFIG]
>   PYTHON  Name of python command to use in
>   configure and the test suite.
> +PYTEST   Name of pytest command to use in
> +the test suite.
>  
>  Additionally, various options can be specified on the configure
>  command line.
> @@ -571,6 +574,24 @@ if [ $have_python -eq 0 ]; then
>  errors=$((errors + 1))
>  fi
>  
> +pytest=""
> +if [ $have_python -eq 1 ]; then
> +printf "Checking for pytest... "
> +have_pytest=0
> +
> +for name in ${PYTEST} pytest-3 pytest pytest-2; do

This is kind of not granular enough I think.  It would be better to
invoke pytest as "pythonX.Y -m pytest" which is the safe way to execute
it on all python versions.

> +if command -v $name > /dev/null; then
> +have_pytest=1
> +pytest=$name
> +printf "Yes (%s).\n" $pytest
> +break
> +fi
> +done
> +if [ $have_pytest -eq 0 ]; then
> +printf "No (some tests may be skipped).\n"
> +fi

The other thing I was trying to achieve was to be able to run the
unittest for each python version, so say 2.7, 3.5 & 3.6 are supported
then I was trying to find all of those instead of just one.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: New Python bindings (was: Crash with Python bindings)

2018-03-26 Thread Floris Bruynooghe
On Wed, Mar 21 2018, Justus Winter wrote:
>
> Floris Bruynooghe <f...@devork.be> writes:
>
>> This is exactly what I have fixed in my alternative bindings which I
>> created around the end of last year [0].  So we do have an idea of how
>> to fix this, at the time I said I do believe that it's possible to also
>> do this for the existing bindings even though it is a lot of work.
>> After some talking between dkg and me we got to a way forward which
>> proposed this, but I must admit that after messing a little with getting
>> a pytest run integrated into the notmuch test-suite instead of using tox
>> I lost momentum on the project and didn't advance any further.
>
> I'm sorry that I didn't speak up when you announced your work.  I'm
> actually excited about a new set of bindings for Python.  I agree with
> using cffi.  I briefly looked at the code, and I believe it is much
> nicer than what we currently have.

Nice to hear, thanks!

> I trust that it works fine with Python 3, does it?

The version I made so far *only* works on Python 3.  Mostly because it
was easier, but it also allows some API niceties.

> The testsuite cannot depend on pulling stuff from the net simply because
> build servers typically do not have access to it.  That is a hard
> requirement.

Sure I understand that.  See another part of this thread on this though.


Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Crash with Python bindings

2018-03-18 Thread Floris Bruynooghe
Daniel Kahn Gillmor <d...@fifthhorseman.net> writes:

> On Fri 2018-03-16 19:30:37 +0100, Floris Bruynooghe wrote:
>> If someone can hook pytest runs with various python versions into the
>> notmuch test suit I'd be very much obliged and probably have another go
>> at this as it's still an interesting problem and gives a nice way
>> forward.
>
> I don't really know what this request means -- so maybe that means that
> i'm not the right person for the task, which is fine.
>
> but it's also possible that the right person for the task *also* doesn't
> know what you're asking for, so if you could elaborate a bit further
> i think that would be super helpful :)

Fair enough :)
Here a somewhat more long-form version of this:

Before even attempting to refactor the existing bindings to use cffi as
a backend instead off ctypes and/or adding the changes needed to track
the lifetime of objects correctly I would like to be able to write full
unitttest-level tests for the bindings to be able to guarantee that no
user-level APIs are broken.  In my version of the bindings I did this
the traditional Python way: using pytest for writing unittest and using
tox to invoke the tests for the various supported versions of python.

One of the feedback items I got from the patch I sent last time was that
the project would be reluctant to adopt this and would like to avoid
virtualenv and pip with their behaviour of downloading things over the
network.  Instead wishing for it to use a system python which should
have the available tools already installed (i.e. pytest).  And all this
just integrated in the existing test suite.

So my last attempt at this looks like I made a test/T391-pytest.sh file
with the idea of running a subtest for each python version, with the
subtest being a ``pythonX.Y -m pytest bindings/python/tests/`` so it'd
run the entire test.  To be nice this also needs to be hooked up so that
the subtests get skipped when a python version is not available, or is
missing pytest itself.

So while trying to figure this out is where I got distracted last time
and started working more on other things.


Kind Regards,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Crash with Python bindings

2018-03-16 Thread Floris Bruynooghe
Hi all,

David Bremner  writes:

> "W. Trevor King"  writes:
>
>> you can avoid the abort (which happens when q.__del__ is called after
>> db.__del__).  We could make that sort of cleanup easier with context
>> managers for Query objects (we have them for databases since [3]), and
>> they look like the only object that keep an internal database
>> reference:
>>
>>   with Database() as db:
>> with Query(db, "*") as q:
>>   # do something with q
>> db.close()
>>
>
> I'm reminded [1] that this problem still exists. If noone has any idea
> of a fix, should we document one of the workarounds?

This is exactly what I have fixed in my alternative bindings which I
created around the end of last year [0].  So we do have an idea of how
to fix this, at the time I said I do believe that it's possible to also
do this for the existing bindings even though it is a lot of work.
After some talking between dkg and me we got to a way forward which
proposed this, but I must admit that after messing a little with getting
a pytest run integrated into the notmuch test-suite instead of using tox
I lost momentum on the project and didn't advance any further.

If someone can hook pytest runs with various python versions into the
notmuch test suit I'd be very much obliged and probably have another go
at this as it's still an interesting problem and gives a nice way
forward.

Cheers,
Floris

[0] https://github.com/flub/notmuch/tree/cffi/bindings/python-cffi
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


LIBNOTMUCH_MINOR_VERSION for 0.26

2018-01-15 Thread Floris Bruynooghe
Hi,

I was looking to update my bindings with the latest release and am
somewhat confused on the value of the LIBNOTMUCH_MINOR_VERSION value.
It seems to be suggested in e.g. the documentation of
notmuch_database_index_file that for notmuch 0.26 this should be 1,
however the header still seems to include it as 0.

Was this accidentally not updated or is there another explanation that I
haven't figured out yet?

Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Update on python-cffi bindings

2017-12-29 Thread Floris Bruynooghe
Hi all,

Daniel Kahn Gillmor <d...@fifthhorseman.net> writes:
> On Thu 2017-12-21 12:30:39 +0100, Floris Bruynooghe wrote:
>> The API changes a lot and there is no easy migration.  And history has
>> shown that's a terrible way to get something new adopted.  Last time I
>> suggested a possible multi-tiered approach (maybe not as explicit):
>>
>> 1 I think it's possible to move the memory management technique to the
>>   existing API without API breakage.  It would still allow users to call
>>   functions in the wrong order etc, but that's not any regression.
>>
>> 2 It's probably possible to either switch the existing API to use CFFI
>>   or create a drop-in replacement for it based on CFFI.  The benefit
>>   here is two-fold: users get better PyPy performance and I believe it
>>   becomes easier to maintain the bindings, e.g. all you need to do to
>>   call notmuch_database_get_path is
>>   capi.ffi.string(capi.lib.notmuch_database_get_path(dbptr)) (see
>>   
>> https://github.com/flub/notmuch/blob/cffi/bindings/python-cffi/notdb/_database.py#L263)
>>   for an actual example).  But this really depends on what everyone else
>>   here that maintains the Python bindings here thinks.  I'd encourage to
>>   have a look at the CFFI implementation to get an idea of this.
>
> these two steps on their own seem like they give us:
>
>   * better and safer memory management
>
>   * better performance on PyPy
>
>   * arguably, easier maintenance of the bindings.
>
> These seem like unambiguous wins, and there is no downside -- people
> using the notmuch module can just upgrade to the new version and it's
> done.  I'd love to see these changes happen.
>
> The only thing it doesn't do is provide more idiomatic bindings for new
> users, which you describe as:
>
>> 3 As last step I still think providing the more idiomatic bindings is
>>   useful, especially for new users.  It does take the burden of
>>   correctly calling the C functions somewhat more.  This could then
>>   either treat a notmuch_cffi as a lower level API which more directly
>>   maps the C API or it could call C directly as it does now.  I'm not
>>   currently sure on which is more feasible or better here.
>>
>>   An additional thing that could be done here to ease migration is to
>>   allow creating the new objects from the old ones allowing a codebase
>>   to gradually adopt the newer API where appropriate.  E.g.:
>>
>>  db = notmuch.Database(...)
>>  msg = db.find_message(...)
>>  new_db = notdb.Database.from_notmuch(db)
>>  new_msg = notdb.Message.from_notmuch(msg)
>>  print('Tags not on msg: {}'.format(new_db.tags - new_msg.tags))
>>
>>   This would rely on the existing API to migrate to CFFI, otherwise it
>>   could still be possible but would be very hairy.
>
> I'm not sure i understand this last sentence.  Are you saying that step
> 3 depends on step 2 happening?

The mixing of a new API with the existing API would be possible if we
can successfully migrate the existing notmuch API to use CFFI.  If not
it may theoretically be possible but could be a lot of pain.

> I'm not sure about the name "notdb"

I'm in no way attached to this name, just needed something to work with.
Naming things is hard, if anyone has a different idea please share.

> but i don't mind the idea of there
> being some other "more-pythonic" notmuch bindings.  New users would
> likely prefer it, and that'd be fine.
>
>> - For exposing completely new APIs, sure building the
>>   notdb.ImmutableTagSet and MutableTagSet was not straight forward,
>>   likewise for the PropertiesMap.  Many other things are much easier
>>   though.  One possible nice way to alleviate this with the idea of the
>>   existing notmuch API being the lower-level layer nearly mimicking the
>>   C-API directly.  That way adding new APIs there is more or less
>>   straight forward and there is less time pressure to add them to the
>>   higher level API.  Especially if mixing the APIs is supported.
>
> I think this is in line with the approach described above.  I like it.

I would really like to hear from some of the existing python binding
users on all of the above and what they thing of this approach.  Is this
work worth it to you?  Will it make you more or less likely to want to
migrate to more pythonic bindings?

In particular I'm not that convinced in the suitability of the existing
library as a low-level binding as it currently sits somewhere above
basic wrapping of libnotmuch.  Anyway, I would appreciate any feedback
on the proposed APIs and way forward.


Cheers,
Floris
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


  1   2   >