Skip to content

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

Merged
merged 2 commits into from
Sep 3, 2015

Conversation

Sheeo
Copy link
Contributor

@Sheeo Sheeo commented May 22, 2015

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.

@Sheeo Sheeo force-pushed the cffi_build branch 2 times, most recently from d570385 to e8100a8 Compare May 22, 2015 01:34
@Sheeo
Copy link
Contributor Author

Sheeo commented May 22, 2015

I don't know why this fails for pypy. Essentially I followed this

@jdavid
Copy link
Member

jdavid commented May 25, 2015

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
Any reason to support cffi < 1.0?

@carlosmn
Copy link
Member

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?

@jdavid
Copy link
Member

jdavid commented May 25, 2015

Ok, then somebody needs to fix the bug for this PR to be merged.

@Sheeo
Copy link
Contributor Author

Sheeo commented May 26, 2015

Yeah I don't think it's worth removing older PyPy support over this. It should be possible to make the setup.py file compatible with cffi<1.0, which is what I was trying to do. I'll hopefully get back to this soon.

@Julian
Copy link

Julian commented Jun 21, 2015

+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 pypy -c 'import pygit2' takes a full 3 seconds to run, which for my purposes means I'm better off just calling a subprocess unfortunately.

@jdavid
Copy link
Member

jdavid commented Aug 28, 2015

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 BuildWithDLLs? This is about Windows not cffi. And related to this, have you tested your changes on Windows?

@Sheeo
Copy link
Contributor Author

Sheeo commented Aug 28, 2015

@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.
@Sheeo Sheeo force-pushed the cffi_build branch 2 times, most recently from 52fd6f3 to becc265 Compare August 28, 2015 17:47
@Sheeo
Copy link
Contributor Author

Sheeo commented Aug 28, 2015

I've rebased the PR and setup travis to insall cffi >= 1.0.0.

Unfortunately the diff for setup.py ended up being rather subpar, I apologize.

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 cffi >= 1.0 on travis.

@jdavid jdavid merged commit f28a199 into libgit2:master Sep 3, 2015
@jdavid
Copy link
Member

jdavid commented Sep 3, 2015

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.

@olasd
Copy link
Contributor

olasd commented Sep 10, 2015

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.

I did this in #561, and I hope it can get merged in time for the next release :)

@bisho
Copy link
Contributor

bisho commented Sep 21, 2015

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.

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.

6 participants