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

Automatically add include_dirs for externs #1654

Merged
merged 1 commit into from Jun 19, 2017

Conversation

jdemeyer
Copy link
Contributor

Consider the following situation: some package Foo has installed files foo.pxd and header.h in .../python/site-packages/foo. The foo.pxd file contains cdef extern from "header.h" where header.h is installed in .../python/site-packages/foo/header.h

When another package Bar wants to cimport foo.foo, the cdef extern from "header.h" will become #include "header.h" in the C file. The problem is now that the C compiler will not be able to find this file.

The solution is obvious: whenever we look for a cdef extern from file, we add its directory to the include_dirs of the extension.

@embray
Copy link
Contributor

embray commented Apr 11, 2017

Makes perfect sense to me.

@jdemeyer
Copy link
Contributor Author

We're assuming in SageMath (https://trac.sagemath.org/ticket/22728) that this will be merged.

@jdemeyer
Copy link
Contributor Author

An alternative solution to the problem would be to keep the Extension but replace filenames in the Cython-generated .c files. That is, Cython could turn

cdef extern from "foo.h":
    ...

into

#include "/path/to/foo.h"

Personally, I'm not convinced that this is a better solution, because I think it would be the only place in Cython where the code generation depends on data coming from the Extension object. For that reason, it might be harder to implement also.

@robertwb robertwb added this to the 0.26 milestone Jun 16, 2017
@robertwb
Copy link
Contributor

Yes, this looks good.

@robertwb robertwb merged commit cfa48ae into cython:master Jun 19, 2017
@scoder
Copy link
Contributor

scoder commented Aug 11, 2017

I had to apply the change in e9194f7 in order to avoid writing absolute path names into the C file. Hope it still works for everyone.

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.

None yet

4 participants