Skip to content

Don't use the deprecated cffi.verify by default#529

Merged
jdavid merged 2 commits into
libgit2:masterfrom
Sheeo:cffi_build
Sep 3, 2015
Merged

Don't use the deprecated cffi.verify by default#529
jdavid merged 2 commits into
libgit2:masterfrom
Sheeo:cffi_build

Conversation

@Sheeo

@Sheeo Sheeo commented May 22, 2015

Copy link
Copy Markdown
Contributor

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

Sheeo commented May 22, 2015

Copy link
Copy Markdown
Contributor Author

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

@jdavid

jdavid commented May 25, 2015

Copy link
Copy Markdown
Member

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
Copy Markdown
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

jdavid commented May 25, 2015

Copy link
Copy Markdown
Member

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

@Sheeo

Sheeo commented May 26, 2015

Copy link
Copy Markdown
Contributor Author

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

Julian commented Jun 21, 2015

Copy link
Copy Markdown

+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

jdavid commented Aug 28, 2015

Copy link
Copy Markdown
Member

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

Sheeo commented Aug 28, 2015

Copy link
Copy Markdown
Contributor Author

@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

Sheeo commented Aug 28, 2015

Copy link
Copy Markdown
Contributor Author

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

jdavid commented Sep 3, 2015

Copy link
Copy Markdown
Member

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

olasd commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

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

bisho commented Sep 21, 2015

Copy link
Copy Markdown
Contributor

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