Force cythonize of *.pyx file

[m-rossi]

Close #36

It seems like cythonize of your *.pyx does not do anything if a *.c file. By using force=True it uses the *.pyx and also fixes the issues on Python 3.10. I also applied this fix as a patch to the conda-forge-recipe in https://github.com/conda-forge/reedsolo-feedstock/pull/6 and will remove it once you released a new version.

[dobairoland]

I can also reproduce the issue and the PR here works for me if cython is installed.

[dcousens]

Anything holding this pull request back?

[lrq3000]

Time. I need to thoroughly test this to ensure it doesn't break installation or usage for anybody where it previously worked.

6 nov. 2022 05:51:23 Daniel Cousens @.***>:

Anything holding this pull request back?

— Reply to this email directly, view it on GitHub[https://github.com/tomerfiliba/reedsolomon/pull/38#issuecomment-1304714632], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAIRFXQDWIQANRGL6F4PN53WG42MXANCNFSM5IUJ7VJQ]. You are receiving this because you are subscribed to this thread.[Image de pistage][https://github.com/notifications/beacon/AAIRFXQIULJRGQUFRONLR43WG42MXA5CNFSM5IUJ7VJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJXCF3CA.gif]

[dcousens]

@lrq3000 fair enough. For what it is worth, I can't actually install this package without this pull request.

I added the following to my requirements.txt to resolve https://github.com/tomerfiliba/reedsolomon/issues/36:

reedsolo@git+https://github.com/m-rossi/reedsolomon.git@63e5bd9fc3ca503990c212eb2c77c10589e6d6c3

~ python --version
Python 3.10.8
~ pip --version
pip 22.3 from /usr/lib/python3.10/site-packages/pip (python 3.10)

[CharlemagneLasse]

Time. I need to thoroughly test this to ensure it doesn't break installation or usage for anybody where it previously worked. 6 nov. 2022 05:51:23 Daniel Cousens @.***>: … Anything holding this pull request back? — Reply to this email directly, view it on GitHub[#38 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAIRFXQDWIQANRGL6F4PN53WG42MXANCNFSM5IUJ7VJQ]. You are receiving this because you are subscribed to this thread.[Image de pistage][https://github.com/notifications/beacon/AAIRFXQIULJRGQUFRONLR43WG42MXA5CNFSM5IUJ7VJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJXCF3CA.gif]

@ lrq3000: Why does this need to be tested so thoroughly that the regression from 027715718902f1e8ef4ad46481a11cef044c4eb6 is still present after years? Why wasn't 027715718902f1e8ef4ad46481a11cef044c4eb6 tested with the same care in the first place?

[lrq3000]

@ lrq3000: Why does this need to be tested so thoroughly that the regression from https://github.com/tomerfiliba/reedsolomon/commit/027715718902f1e8ef4ad46481a11cef044c4eb6 is still present after years? Why wasn't https://github.com/tomerfiliba/reedsolomon/commit/027715718902f1e8ef4ad46481a11cef044c4eb6 tested with the same care in the first place?

@CharlemagneLasse Because there has already been at least 2 PRs attempting to fix this issue before, this is the 3rd attempt. So this time, it needs to be better tested.

[lrq3000]

Also the primary reason is that I don't have the toolchain to test or even just to run the cythonization anymore, which has been made significantly more complicated since Python 3 and the lack of a dedicated python compiler, this is the primary issue holding back the testing of this PR.

[lrq3000]

Ok so this issue seems to affect only those who 1) have Cython installed, 2) have a compiler installed.

The reason why the *.pyx files were not necessarily cythonized and compiled is because the already cythonized creedsolo.c file had some specific optimizations, but anyway with a so wide distribution of Python versions, it should be dropped, and the .pyx should always be cythonized, for better cross-versions compatibility.

However, I cannot test for myself, because I am running on Windows, and since Python 2.7, there is no miniaturized compiler, and I don't have 50 GB to spare JUST to install a Python compiler for this one project (I use mingw toolchain and cygwin for all my compiling needs, except Python for which they are not not compatible). So I'm going to trust this PR is working, and if not, well, another issue is going to be opened anyway.

@CharlemagneLasse So there you have your answer, that's why the previous fixes were not thoroughly tested either last time. If Python wasn't so messy to compile on Windows, this would have been tested.

If this issue still continues, I will consider dropping the cythonized releases once and for all. It would be a shame as they produce an interesting speed boost, but that's not worth the trouble at this point.

[lrq3000]

@m-rossi thank you very much for the PR

[dcousens]

@lrq3000 thanks for maintaining this package, I know OSS can be a thankless endeavour and the effort can routinely outweigh any benefit :yellow_heart:

[lrq3000]

Thank you @dcousens for your kind words!

I could finally install all the toolchain, it's much easier than before, it only "only" weighs about 10GB.

Anyway, I could not reproduce the error here, and I'm sure I also installed the toolchain before (contrary to what I stated above), because 1) I updated the build instructions, 2) building the cythonized extension is necessary to release on pypi.

So I have no idea why this bug appears to some users and not me. Hopefully, it won't happen anymore. But if it does, I will reverse the logic (make the cythonization as an explicit commandline argument, instead of automatic when Cython is detected).