Python3.8 + cythonize #78
Conversation
With some edits: * Relax Cython>=0.28 pin * Cosmetic changes to clarify what ext_modules is
This should bootstrap Cython at setup
|
@kmike @superbobry any chance that this will be merged and released? We urgently need this fix for Snakemake. We are happy to help in any way if there are further showstoppers. |
|
Apparently I'm (half)blind and didn't notice this PR until after I updated the bindings for 3.8+ (and dropped python2 support) in my fork. My changes are simpler(?) but instead of messing with an old submodule commit I made it depend on libdatrie instead (currently at 0.2.12). My plan was to make a new tag (in my fork) and push new packages to a PPA and Gentoo overlay. Does that help at all? |
|
@sarnold Sure go ahead. I am on Arch and I did something similar which has libdatrie 0.2.12-1. I simply added a symlink, instead of relying on the submodule. ❯ ls -l datrie/libdatrie
total 4
lrwxrwxrwx 1 avmo avmo 19 14.01.2020 13:28 datrie -> /usr/include/datrie/However, I wonder how one can support Windows by this approach? Maybe you have a better idea. |
|
I tested your branch yesterday, and also made a (hopefully) one-off github release for python3 only: https://github.com/freepn/datrie/releases/tag/0.8.1 Still I didn't manage to fix the build warnings; I had the pointer type warning fixed yesterday but didn't get it in yet, and I'm not quite sure what the right/pythonic answer is for the collections import (since py27 is special there). Got any ideas for the handling the import issue? Also I have PPA pkgs for xenial, helpful for travis builds, er, here even: |
|
@sarnold @ashwinvis what do you think about a pypi release. Something like |
|
Apart from Python + Cython + packaging knowledge, I have neither the expertise on tries nor the long term motivation to be a package maintainer for datrie. I do like to see this happen, but it would be mistake if I take up this responsibility now, only to abdicate it in the future. |
|
@sarnold I compared your branch, and as suspected you have many instance of |
|
Maybe I should move it under the hood of snakemake. |
|
@ashwinvis is it ok if I fork your repo, merge in your fix and release this as |
|
Or, probably cleaner, I fork this original repo, and you add your PR against my fork? |
|
Sounds good @johanneskoester! |
|
If you are going to require cython as a build time dependency you can drop the bundled c-files all together which will prevent a similar "the c-files are broken" issues in the future. |
Between that and the 0.8.1 tag this package is getting pretty forked. Has anyone tried reaching out to @kmike or @superbobry not through github? |
|
I wasn't able to reach them via mail. From one, I did not find an email, from the other, the published mail address is not existent any more. |
|
ok. Very short term, I think just the "do the cythonization" patch to setup.py will un-block conda-forge so we can get that moving again. |
|
@ashwinvis I am done with the setup of the fork and github actions for testing. Awaiting your PR. Thanks a lot! |
|
conda-forge/datrie-feedstock#5 <- CF pr |
|
@ashwinvis @johanneskoester @sarnold I now have commit rights on this repo and should have pypi permissions soon. Should be able to get this done and release in the next ~24 hours. Going to make another issue to make clear what my plan is and track the work. |
|
@tacaswell datrie-snakemake is now available on pypi, with the fixes of @ashwinvis. I can create a new conda-forge recipe for it, or we keep the old and refer to datrie-snakemake in it. What do you think? |
|
Wait, I have just seen your release plan. Are you one of the maintainers? In any case, I am happy to remove my fork again. |
|
@johanneskoester Yes, @kmike gave me commit rights this morning and I just confirmed I have access to pypi as well. As for conda, |
|
Commits merged in #80. Thank you @ashwinvis you did most of the hard work here! |
|
Awesome @tacaswell! I will remove my fork then! |
|
Done. |
|
Sorry, got distracted with stuff... Feel free to use/fork whatever is useful; I just made a (local) tag so I could have something to package/deploy for now. |
Merges #76 and #69