-
-
Notifications
You must be signed in to change notification settings - Fork 399
Don't use the deprecated cffi.verify by default #529
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
Conversation
d570385
to
e8100a8
Compare
I don't know why this fails for pypy. Essentially I followed this |
It may not be pypy, but an old version of cffi. Look at the Travis logs, with Python 2/3 pip installs cffi 1.0; but for the pypy build it says the cffi requirement is already satisfied, probably by an older version. Locally I have tried your branch with Python 2.7 and older versions of cffi (0.8.6 and 0.9.2) and get an error, though different from the one in Travis. While it works fine with cffi 1.0. Anyway, since cffi is easily installable via pip, we may skip your latest commit and drop support for cffi <1.0 |
Pypy has its cffi module built-in, which means you cannot install it via pip but have to use the version which comes with the interpreter. Only supporting versions >=1.0 leaves out every released version of pypy as well as whoever prefers using the distribution-provided packages. This method new method has also only become available rather recently, as I understand. Are the advantages of moving to this 1.0-only method immediately worth removing pypy support and making everyone update their cffi packages? |
Ok, then somebody needs to fix the bug for this PR to be merged. |
Yeah I don't think it's worth removing older PyPy support over this. It should be possible to make the |
+1 for CFFI >1.0 -- it's not removing PyPy support, generally people I think stay fairly up to date on PyPy versions considering how beneficial the updates typically are. And besides being much nicer to write, there are some things that will help import speed, which seems to be an issue -- for me |
At this point I am willing to merge this PR. If anyone wants to bring back support for older versions they will have time to do so before the next release. Just a question @Sheeo why do you remove |
@jdavid whoops, that's unintended! I was testing on windows yes, but I must've had the dll in my path during testing. Apologies. I'll rebase and reintroduce the function -- at this time I don't have access to a windows environment capable of building it however. If anyone is interested (I may get around to this myself) we could setup windows-based CI on http://www.appveyor.com, with bonus points for uploading windows wheels to github / pypi for release. |
Instead this does what is recommend in the CFFI docs here: https://cffi.readthedocs.org/en/latest/cdef.html?highlight=verify#out-of-line-api This also means building the cffi extension is neatly handled by cffi's setuptools integration itself, so we can delete the code in setup.py that used to do this.
52fd6f3
to
becc265
Compare
I've rebased the PR and setup travis to insall Unfortunately the diff for I'm currently unable to verify whether this actually works on windows, it builds successfully locally and travis is happy for CPython. Travis builds fail for pypy as before, I'm unaware of how to upgrade to pypy versions with embedded |
So it is merged. The minimum required version of PyPy is now 2.6, which is not yet supported by Travis, so I have temporarily disabled PyPy on Travis. There is not yet a release of PyPy3 with the new cffi. |
I did this in #561, and I hope it can get merged in time for the next release :) |
The patch: Sheeo@becc265 works really well. The deprecated cffi.verify() api was delaying the linking to run time, so you get calls to gcc in production, where sometimes gcc is not available. Works well enough for pip installs that have gcc, but when trying to do deploys that just doesn't work. With the diff applied, the linking is done at installation time and the library runs just fine. There are a several related bugs in distros, like https://bugzilla.opensuse.org/show_bug.cgi?id=933421 so I think that diff should be also merged in trunk. |
Instead this does what is recommend in the CFFI docs here: https://cffi.readthedocs.org/en/latest/cdef.html?highlight=verify#out-of-line-api
This also means building the cffi extension is neatly handled by cffi's setuptools integration itself, so we can delete the code in setup.py that used to do this.
This change means it's easier to install pygit2 with the binary extensions bundled. It will fallback to the previous behavior in case the bundled extension was not found.