Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to notmuch2 module #320

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mjg
Copy link
Contributor

@mjg mjg commented Nov 26, 2021

Here are some contributions on the way to using notmuch2:

I based my work on @GuillaumeSeren `s branch and added my work on top, resulting in a passing test suite. The test suite does not cover all code paths. The last commit in this PR covers changes which are not caught by the suite, and I have not run this branch against my "real" mail database yet.

Also, I use a generator expression at some point, and this may fail on older python versions - I'm not sure which ones afew wants to support.

So, basically: do not merge yet ;)

GuillaumeSeren and others added 6 commits November 26, 2021 22:30
Signed-off-by: Guillaume Seren <guillaumeseren@gmail.com>
Signed-off-by: Guillaume Seren <guillaumeseren@gmail.com>
Signed-off-by: Guillaume Seren <guillaumeseren@gmail.com>
283ee15 ("Upgrade Database to notmuch2 python module afewmail#278", 2020-11-11)
adjusted Databse partially to notmuch2. There, open() does not have a
create argument any more. So far, only afew's test suite needs to create
a db. So, remove the argument from afew's Database class and use
notmuch2's create() directly in the test suite.
Besides the obvious call signature changes, filenames are Posix.Path
objects now, searches do not return an email.Message object, and
non-existing headers throw a LookupError.
Not caught by the test suite.
@mjg mjg force-pushed the migrate-to-notmuch2-module branch 2 times, most recently from f8db509 to 88d675e Compare November 26, 2021 23:26
python3-notmuch2 is part of ubuntu 21.04 and later, so we need to bump
that, too.
@mjg mjg force-pushed the migrate-to-notmuch2-module branch from 88d675e to 6e98c68 Compare November 26, 2021 23:29
Comment on lines +92 to +97
ret = set()
for msg in db.open().messages('folder:{}'.format(folder)):
with open(msg.path) as f:
ret.add((os.path.basename(msg.messageid),
email.message_from_file(f).get_payload()))
return ret

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: would it be interesting to run this method asynchronously to speed things up? I believe this will speed things up when someone has a bunch of emails to process. Perhaps having a private method that handles the asynchronous call and calls add? I've done something similar here: https://github.com/afewmail/afew/pull/316/files#r744197423

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just in the test suite, which has 10 e-mails or so.

That mail mover test suite is not exactly "pretty" anyways, the proposed changes are the minimal ones to let it pass with the notmuch2 bindings. I would refactor the e-mail comparisons quite a bit, for example.

@benmezger
Copy link

benmezger commented Nov 27, 2021 via email

@GuillaumeSeren
Copy link
Collaborator

Hey @mjg !
Thank you to work on this.

This is a big subject as the eco-system is gona migrate to the new python module soon (notmuch, alot and afew),
also as you have seen in some old issue the notmuch package of your distro may stop shipping the old python module,
due to security concern, so we have to have ready.

But I didn't merge it right away because many distro still ship older version of notmuch (0.2x I think),
and the newer CFFI module is in 0.3x so we still have some time.

Is there something going on with the ci runners ?

Maybe there is an issue un GH now ?

@mjg
Copy link
Contributor Author

mjg commented Sep 13, 2022

I see that lieer is trying to still support 18LTS versions of ubuntu by catering for both versions of the notmuch bindings. Is that something you would prefer, too? In this case this PR needs to be done differently, of course. (Also, I haven't looked into it since.)

@GuillaumeSeren
Copy link
Collaborator

Hey @mjg,
I didn't know this lieer project it looks very cool.

If we can refactor the branch to able to support both, it would nice because we could merge now,
and start testing in the next release.
But it could also lead to more issue and complexity just to support old version.

In the end it depends on the implementation.

@hbog
Copy link

hbog commented Sep 22, 2023

Is there something I can do to help to get this merged ? The current notmuch bindings bother me by causing segmentation faults is certain cases.

@GuillaumeSeren
Copy link
Collaborator

GuillaumeSeren commented Nov 21, 2023

Hey @hbog ,
if you want to help you can try this branch, but I don't think it is ready yet (if you have problem with main branch open a different issue).

@mjg
Copy link
Contributor Author

mjg commented Feb 28, 2024

What would help (without having to port any bindings) is adding more tests so that at least each filter is covered.
As a second step, if we still want to support the legacy bindings, some refactoring should be done in order to isolate any uses of the notmuch module. Then we can reimplement that interface for notmuch2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants