search

tomerfiliba-org / reedsolomon

⏳🛡 Pythonic universal errors-and-erasures Reed-Solomon codec to protect your data from errors and bitrot. Includes a future-proof zero-dependencies pure-python implementation 🔮 and an optional speed-optimized Cython/C extension 🚀
http://pypi.python.org/pypi/reedsolo
Other
358 stars 86 forks source link

Allow building/installing without Cython #21

Closed projectgus closed 4 years ago

projectgus commented 4 years ago

Hi,

Thanks for maintaining this project, it's very useful to have!

Noticed that in v1.5.1 building or installing the package will fail if Cython is not installed - setup.py falls back to trying to compile creedsolo.c which doesn't exist in the repo (it's generated by Cython). Even if creedsolo.c was added to the repo, this means the package can't be built on platforms without a C compiler installed.

This PR changes the behaviour so if Cython is not installed, only the reedsolo module is built. This allows just this module to be installed as "pure Python".

PS I saw the note in setup.py about not supporting Python 3.8 - do you have any info about the exact problem? FWIW, on Ubuntu 20.04 amd64 with Python 3.8.2 & Cython 0.29.14 I found the creedsolo module built OK and test_creedsolo.py can pass. I haven't tested it beyond this.

projectgus commented 4 years ago

Hmm, I'm not sure why coveralls says coverage went down as a result of this commit. Any clues?

lrq3000 commented 4 years ago

@projectgus Nice catch! Thanks a lot for your PR!

I indeed forgot that there is no pure python install, although there should be one. Currently the behavior is that the module can be installed without Cython as long as there is a C compiler (using the pretranspiled creedsolo.c), but indeed even that is unnecessary if a C compiler is not installed.

I'll update the PR to combine your changes while still keeping creedsolo.c, as I think it can be useful to maintain an already pretranspiled C file.

lrq3000 commented 4 years ago

PS: Don't worry about coveralls, he's including files it shouldn't in the coverage analysis (temporary files created by travis). I don't know why but I can fix that afterward, that's a separate issue.

lrq3000 commented 4 years ago

Ok it's merged in, thanks a lot! 👍

lrq3000 commented 4 years ago

Ah and thanks for the tip about v3.8, this was an issue with Travis, I'll see if maybe now it's fixed. /EDIT: you're right, Cython works fine now on Python 3.8 and on Travis, support for this version is now declared on pypi and is unit tested on Travis :-)

projectgus commented 4 years ago

Thanks for the super fast response and merge, @lrq3000!

v1.5.3 is working great for our use case now (which is being able to install from pip on installations that don't have a native compiler.)

:raised_hands:

lrq3000 commented 4 years ago

Ah ok great ! Thank you for letting me know this also affected pip install, sorry about that!

projectgus commented 4 years ago

Thank you for letting me know this also affected pip install, sorry about that!

@tomerfiliba No problems. FWIW, the way this works is that if there isn't a wheel matching the platform in the files on pypi, pip essentially downloads the source tarball and runs setup.py build on it (probably there are some subtle differences to directly running setup.py build, but that's the gist of it.)