Skip to content
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

Tweak include/lib dir detection in ffi.py #382

Merged
merged 1 commit into from
Jul 4, 2014
Merged

Tweak include/lib dir detection in ffi.py #382

merged 1 commit into from
Jul 4, 2014

Conversation

jasperla
Copy link
Contributor

@jasperla jasperla commented Jul 3, 2014

This is similar to what's done in setup.py by using sane defaults,
instead of just failing if git2.h is located in /usr/local .

@carlosmn
Copy link
Member

carlosmn commented Jul 3, 2014

Won't this fail to find a system-provided libgit2?

@jasperla
Copy link
Contributor Author

jasperla commented Jul 3, 2014

Then https://github.com/libgit2/pygit2/blob/master/setup.py#L57 would fail too, no?

@jasperla
Copy link
Contributor Author

jasperla commented Jul 3, 2014

It seems Travis is having issues with this too. The problem I was trying to solve is that on OpenBSD git2.h lives in /usr/local/include . ffi.py would fail to find it unless I set LIBGIT2 for compilation and installation. I hoped there was a way it would either figure it out by itself or perhaps take it from setup.py ?

@carlosmn
Copy link
Member

carlosmn commented Jul 3, 2014

Travis is having issues because you've removed the logic that sets the correct path in case that there is a LIBGIT2 set. You're setting the list to empty instead of leaving that bit of logic as it was.

@carlosmn
Copy link
Member

carlosmn commented Jul 3, 2014

If your includes live under /usr/local/include, I don't see why you would need any changes. With the current code, in the absence of LIBGIT2, libgit2_path is set to /usr/local/ and that is joined with include, which would already be setting the default include path to /usr/local/include.

@carlosmn
Copy link
Member

carlosmn commented Jul 3, 2014

Sorry, I was looking at setup.py instead of ffi.py since they have essentially have the same block. Bu the solution would seem to be the same. If there is no path, set the path to /usr/local or /usr and the rest of the logic would work.

@jasperla
Copy link
Contributor Author

jasperla commented Jul 3, 2014

Here's the build log from my system: https://gist.github.com/jasperla/288c643e16fc3b59b64c (without the proposed patch)
Anything you spot wrong in there?

@carlosmn
Copy link
Member

carlosmn commented Jul 3, 2014

What I'm thinking of is something like this, which lets the current logic do the work and adds the fallback we have in setup.

diff --git a/pygit2/ffi.py b/pygit2/ffi.py
index 0dba2f3..b776dec 100644
--- a/pygit2/ffi.py
+++ b/pygit2/ffi.py
@@ -112,11 +112,13 @@ with codecs.open(decl_path, 'r', 'utf-8') as header:

 # if LIBGIT2 exists, set build and link against that version
 libgit2_path = getenv('LIBGIT2')
+if not libgit2_path:
+    libgit2_path = '/usr/local'
+
 include_dirs = []
 library_dirs = []
-if libgit2_path:
-    include_dirs = [path.join(libgit2_path, 'include')]
-    library_dirs = [path.join(libgit2_path, 'lib')]
+include_dirs = [path.join(libgit2_path, 'include')]
+library_dirs = [path.join(libgit2_path, 'lib')]

 C = ffi.verify("#include <git2.h>", libraries=["git2"],
                include_dirs=include_dirs, library_dirs=library_dirs)

@jasperla
Copy link
Contributor Author

jasperla commented Jul 3, 2014

That worked better indeed.

@jdavid jdavid merged commit b0bf223 into libgit2:master Jul 4, 2014
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.

3 participants