Skip to content

Extend search path for PyCXX headers #2282

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 1 commit into from
Aug 8, 2013

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Aug 6, 2013

PyCXX headers in Gentoo are installed in /usr/include/pythonX.Y/CXX directory (e.g. /usr/include/python2.7/CXX/Extensions.hxx).
This patch fixes detection of PyCXX in Gentoo:

--- setupext.py
+++ setupext.py
@@ -701,6 +701,7 @@
             # the header files can be found.
             base_include_dirs = [
                 os.path.join(x, 'include') for x in get_base_dirs()]
+            base_include_dirs += [sysconfig.get_python_inc()]
             if has_include_file(base_include_dirs, 'CXX/Extensions.hxx'):
                 return 'Using system CXX (version unknown, no pkg-config info)'
             else:

@mdboom
Copy link
Member

mdboom commented Aug 6, 2013

A future release of PyCXX will include a pkg-config .pc file to resolve this issue in the correct way. You should instead report this bug against Gentoo's PyCXX package, that it needs to be updated. Pass along that both Fedora and Debian have done this.

@mdboom mdboom closed this Aug 6, 2013
@Arfrever
Copy link
Author

Arfrever commented Aug 6, 2013

/usr/include/pythonX.Y is a standard location of headers of Python packages. On my system, 10 packages other than CPython 2.7 itself install headers in /usr/include/python2.7 (e.g. bsddb3 installs /usr/include/python2.7/bsddb3/bsddb.h).

pkg-config is a tool not designed for Python packages and it ignores many aspects of packaging of Python packages (e.g. the fact that given package might be separately installed for many versions of Python).

It does not matter what Fedora or Debian are doing.

@mdboom
Copy link
Member

mdboom commented Aug 6, 2013

In this case, we are talking about C/C++ headers, and pkg-config is the standard method to find such headers... And I think it's the best long term solution, particularly because it removes the burden on packages like matplotlib to special case all of the different places in which these files might be found on different distros. The only alternative (which numpy, for example, takes) is to include a way to get the path to the include files from the Python package. PyCXX, as a project, has decided to take the former approach.

That said, the real bug in matplotlib here is not that it doesn't include the CXX include path correctly (as that's handled by the default include paths in distutils), but that the check is incorrect. I think the fix here is probably to obtain and check in the distutils include paths, not the hardcoded paths that matplotlib normally looks for things in. Expect a fix here shortly.

@mdboom mdboom reopened this Aug 6, 2013
@tomspur
Copy link
Contributor

tomspur commented Aug 7, 2013

On Fedora, we seem to have now a working python3-pycxx-devel, which is not found as python3 support has been disabled the hardcoded way. Could the python3 check please be removed in setupext.py Line 677 and following?
It seems to work so far with matplotlib-1.3.0.

@mdboom
Copy link
Member

mdboom commented Aug 7, 2013

@tomspur: Unfortunately, we still need to be able to handle the case where pycxx is installed from upstream source, until such time as there is an official release we can version check against. The current behavior -- that it builds with the local copy -- you still get something that works in all cases. Doing what you suggests will break things for at least some portion of people.

@tomspur
Copy link
Contributor

tomspur commented Aug 7, 2013

@mdboom python3-pycxx is installed from the upstream source as an previous SCM checkout and shipped as a Fedora package. Don't you know, if the package is installed, if the "import CXX" succeeds later on and then use the bundled copy, if that failed? With the current behavior matplotlib won't use pycxx from upstream source, or do I miss something?

@mdboom
Copy link
Member

mdboom commented Aug 7, 2013

There has been no released version number from the upstream PyCXX project with the patches that are required to build with Python 3. Being able to import it is not enough -- the compilation will fail later, despite that. Fedora is creating their package off of a SCM checkout, and there is no way, as far as I can tell, to version check that. (pkg-config --modversion PyCXX still returns 6.2.4 with Fedora's package). Once we have a version number to check against, the test can be updated. Until then, we're stuck with something that works, but it less than ideal, rather than something that is entirely broken in some cases.

@tomspur
Copy link
Contributor

tomspur commented Aug 7, 2013

Aah, I see. Thanks for the explanation.
Then let's wait for 6.2.5 or so. Thanks!

@zultron
Copy link

zultron commented Aug 7, 2013

Hi fellas,

Would it help to stick e.g. the svn rev into the .pc file? e.g.

$ pkg-config --variable=svn_rev PyCXX
280

Not sure if that makes sense, since the .pc file exists only in RH and Debian pkgs, and we'd have to decide how to handle released versions.

Ahh, if only upstream would cooperate.

@mdboom
Copy link
Member

mdboom commented Aug 7, 2013

The only other way I can think of around this is to do some sort of a test compile (a la autoconf), but those kind of things are really tricky with distutils.

I'm crossing my fingers the PyCXX folks will just get a release out soon -- the patches are already in SVN, which is a start.

mdboom added a commit that referenced this pull request Aug 8, 2013
Extend search path for PyCXX headers
@mdboom mdboom merged commit 87d25ff into matplotlib:v1.3.x Aug 8, 2013
@mdboom mdboom deleted the pycxx-fix-check branch August 7, 2014 13:53
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.

4 participants