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

Fix for cimport from PEP420 namespace #2918

Open
CnlPepper opened this Issue Apr 11, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@CnlPepper
Copy link
Contributor

commented Apr 11, 2019

We have a project (https://github.com/cherab) that is split into a number of different sub-packages. Different teams/labs own the different sub-packages. We use namespaces to pull the separate packages together to appear as a single package. We have recently tried to make the package installable from pypi, however we have encountered an issue with cython compilation:

pip install cherab
pip install cherab-openadas

The second package (dependent on the first) fails to compile, saying it can not find the pxd files. On investigating, pip appears to set up a PEP420 namespace where there is no __init__.py in the root package. If I manually add an __init__.py to the root of the package and try pip again, the cython compilation for cherab-openadas succeeds. Given this, I had a look through the cython source and noticed that during the pxd search, only paths with __init__.py/pxd files are scanned. I removed the relevant check in Cython.Utils.search_include_directories() (on 0.29.x branch) and attempted the re-installation with pip in a clean environment. The compilation and installation was successful.

Given PEP420, is there any explicit need for the __init__.* check? Could it be removed? On first look, I can't see any reason it would cause a problem.

@mattngc

This comment has been minimized.

Copy link

commented Apr 11, 2019

Thanks for raising @CnlPepper, +1 for fixing this.

@scoder

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

The lookup algorithm in PEP-420 is a bit more complex than just leaving out the __init__.p* check. PR welcome that implements the correct algorithm.
https://www.python.org/dev/peps/pep-0420/#specification

@CnlPepper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I feel I might be a bit out of my depth with this one, but I can have a look. Do you want this patched against 0.29.x or master?

@scoder

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Definitely master.

@CnlPepper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@scoder I'm not 100% sure how to go about adding a test for this feature... I found test/build/package_compilation.srctree which looks like it might be a good place to get inspiration. I've not encountered srctree files before, is there any documentation and how do I run a specific test?

It looks like I just need to patch the Cython.Utils.is_package_dir() function to understand PEP420 namespace folders. Can you think of any reason this might not be a valid approach?

@scoder

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

That's a good place to start, yes. Our "srctree" test files are literally just a bunch of source files stuffed into one, with the commands at the top that get executed. Use "runtests.py -vv [regex]" to run them. See
https://github.com/cython/cython/wiki/HackerGuide#the-test-suite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.