Repository: cython/cython
Pull request: BUG: prefer sys.path before Cython/Include when searching for pxd files
Fixes #2905 by raising the sys_path resolution into Context.search_include_directories

Also fix a bug in calling `os.path.join` with identical paths which merges them when they are absolute paths but concatenates them when they are relative paths, causing the test which adds a relative path to `sys.path` to fail. This would only occur when searching for `__init__.pxd`.



Problem statement:
BUG: cannot easily override a cython Include
I am trying to vendor `numpy.pxd` (or `numpy/__init__.pxd`) into numpy. It seems the file in `Includes/numpy/__init__.pxd` is seen before mine when I try `cimport numpy`.

To reproduce: copy the `__init__.pxd` from `Includes/numpy` into `site-packages/numpy`, and add garbage in the original file so using it will crash cython (I used `xxxx = xxxxxxxxx`). Then build a project that calls `cimport numpy` such as pandas. If the copy is found, `cythonize` will succeed. If the original version is found, cythonize will crash.

I am willing to work on a PR, but need some direction as to where to look for the problem.

Hints from issue discussion:
I think the problem is that `Includes` is appended to the search path [here](https://github.com/cython/cython/blob/0.29.6/Cython/Compiler/Main.py#L93), then later on `sys.path` is optionally appended to the search path [here](https://github.com/cython/cython/blob/0.29.6/Cython/Utils.py#L136). IMO if `sys_path` is True the precedence should be 
- user provided `include_directories`
- `sys.path`
- `Includes`

instead of the current situation which gives precedence to `Includes`
I agree about the import path order. `sys.path` and `Includes/` should be reversed.
PR welcome.

Commit message: BUG: prefer sys.path before Cython/Include when searching for pxd files

Patch to review:
diff --git a/Cython/Compiler/Main.py b/Cython/Compiler/Main.py
index da4c3cc07f5..bf648a6059d 100644
--- a/Cython/Compiler/Main.py
+++ b/Cython/Compiler/Main.py
@@ -40,6 +40,8 @@
 
 verbose = 0
 
+standard_include_path = os.path.abspath(os.path.normpath(
+        os.path.join(os.path.dirname(__file__), os.path.pardir, 'Includes')))
 
 class Context(object):
     #  This class encapsulates the context needed for compiling
@@ -66,7 +68,7 @@ def __init__(self, include_directories, compiler_directives, cpp=False,
         self.modules = {"__builtin__" : Builtin.builtin_scope}
         self.cython_scope = CythonScope.create_cython_scope(self)
         self.modules["cython"] = self.cython_scope
-        self.include_directories = include_directories
+        self.include_directories = tuple(include_directories)
         self.future_directives = set()
         self.compiler_directives = compiler_directives
         self.cpp = cpp
@@ -75,10 +77,6 @@ def __init__(self, include_directories, compiler_directives, cpp=False,
         self.pxds = {}  # full name -> node tree
         self._interned = {}  # (type(value), value, *key_args) -> interned_value
 
-        standard_include_path = os.path.abspath(os.path.normpath(
-            os.path.join(os.path.dirname(__file__), os.path.pardir, 'Includes')))
-        self.include_directories = include_directories + [standard_include_path]
-
         if language_level is not None:
             self.set_language_level(language_level)
 
@@ -253,8 +251,13 @@ def find_include_file(self, filename, pos):
 
     def search_include_directories(self, qualified_name, suffix, pos,
                                    include=False, sys_path=False):
-        return search_include_directories(
-            tuple(self.include_directories), qualified_name, suffix, pos, include, sys_path)
+        if sys_path:
+            include_dirs = self.include_directories + tuple(sys.path)
+        else:
+            include_dirs = self.include_directories
+        include_dirs = include_dirs + (standard_include_path,)
+        return search_include_directories(include_dirs, qualified_name,
+                                          suffix, pos, include)
 
     def find_root_package_dir(self, file_path):
         return Utils.find_root_package_dir(file_path)
@@ -274,8 +277,10 @@ def c_file_out_of_date(self, source_path, output_path):
             return 1
         for kind, name in self.read_dependency_file(source_path):
             if kind == "cimport":
+                # missing suffix?
                 dep_path = self.find_pxd_file(name, pos)
             elif kind == "include":
+                # missing suffix?
                 dep_path = self.search_include_directories(name, pos)
             else:
                 continue
@@ -602,8 +607,7 @@ def compile(source, options = None, full_module_name = None, **kwds):
 
 
 @Utils.cached_function
-def search_include_directories(dirs, qualified_name, suffix, pos,
-                               include=False, sys_path=False):
+def search_include_directories(dirs, qualified_name, suffix, pos, include=False):
     """
     Search the list of include directories for the given file name.
 
@@ -612,10 +616,7 @@ def search_include_directories(dirs, qualified_name, suffix, pos,
     report an error.
 
     The 'include' option will disable package dereferencing.
-    If 'sys_path' is True, also search sys.path.
     """
-    if sys_path:
-        dirs = dirs + tuple(sys.path)
 
     if pos:
         file_desc = pos[0]
@@ -648,7 +649,18 @@ def search_include_directories(dirs, qualified_name, suffix, pos,
                 path = os.path.join(package_dir, module_filename)
                 if os.path.exists(path):
                     return path
-                path = os.path.join(dirname, package_dir, module_name,
+                # In most cases, dirname and package_dir will be the same.
+                # From the documentation of os.path.join:
+                # " If a component is an absolute path, all previous components
+                # are thrown away and joining continues from the absolute path
+                # component"
+                # So if dirname and package_dir are absolute pathes, one will
+                # be discarded. However what happens when they are relative
+                # single-component paths? They will be concatenated (repeated),
+                # causing rare and hard to debug problems.
+                # path = os.path.join(dirname, package_dir, module_name,
+                #                    package_filename)
+                path = os.path.join(package_dir, module_name,
                                     package_filename)
                 if os.path.exists(path):
                     return path
diff --git a/tests/compile/find_pxd.srctree b/tests/compile/find_pxd.srctree
index 1d7885fe881..75d2765c95b 100644
--- a/tests/compile/find_pxd.srctree
+++ b/tests/compile/find_pxd.srctree
@@ -6,12 +6,13 @@ from Cython.Build import cythonize
 from Cython.Distutils.extension import Extension
 
 import sys
-sys.path.append("path")
+sys.path.insert(0, "path")
 
 ext_modules = [
     Extension("a", ["a.pyx"]),
     Extension("b", ["b.pyx"]),
     Extension("c", ["c.pyx"]),
+    Extension("d", ["d.pyx"]),
 ]
 
 ext_modules = cythonize(ext_modules, include_path=["include"])
@@ -37,3 +38,15 @@ ctypedef int my_type
 
 ######## path/c.pxd ########
 +++syntax error just to show that this file is not actually cimported+++
+
+######## path/numpy/__init__.pxd ########
+
+# gh-2905: This should be found before Cython/Inlude/numpy/__init__.pxd
+
+ctypedef int my_type
+
+######## d.pyx ########
+
+cimport numpy
+
+cdef numpy.my_type foo


Code review comments:
diff_hunk: @@ -253,8 +251,13 @@ def find_include_file(self, filename, pos):
 
     def search_include_directories(self, qualified_name, suffix, pos,
                                    include=False, sys_path=False):
-        return search_include_directories(
-            tuple(self.include_directories), qualified_name, suffix, pos, include, sys_path)
+        if sys_path:
+            include_dirs = self.include_directories + tuple(sys.path)
+        else:
+            include_dirs = self.include_directories
path: Cython/Compiler/Main.py
text: @user1:
It might be cleaner to write this as
```
include_dirs = self.include_directories
if sys_path:
    include_dirs = include_dirs + tuple(sys.path)
```

@author:
adopted

diff_hunk: @@ -40,6 +40,8 @@
 
 verbose = 0
 
+standard_include_path = os.path.abspath(os.path.normpath(
path: Cython/Compiler/Main.py
text: @user1:
I don't think you need `os.path.normpath` here.

@author:
ok

diff_hunk: @@ -648,7 +649,18 @@ def search_include_directories(dirs, qualified_name, suffix, pos,
                 path = os.path.join(package_dir, module_filename)
                 if os.path.exists(path):
                     return path
-                path = os.path.join(dirname, package_dir, module_name,
+                # In most cases, dirname and package_dir will be the same.
+                # From the documentation of os.path.join:
+                # " If a component is an absolute path, all previous components
+                # are thrown away and joining continues from the absolute path
+                # component"
+                # So if dirname and package_dir are absolute pathes, one will
+                # be discarded. However what happens when they are relative
+                # single-component paths? They will be concatenated (repeated),
+                # causing rare and hard to debug problems.
+                # path = os.path.join(dirname, package_dir, module_name,
+                #                    package_filename)
path: Cython/Compiler/Main.py
text: @user1:
It seems to me that this llong comment belongs in the PR discussion, not in the code. There is no point in explaining why the old code was wrong, you need to explain why the new code is right.

@author:
ok

diff_hunk: @@ -274,8 +277,10 @@ def c_file_out_of_date(self, source_path, output_path):
             return 1
         for kind, name in self.read_dependency_file(source_path):
             if kind == "cimport":
+                # missing suffix?
path: Cython/Compiler/Main.py
text: @user1:
No idea. As far as I can tell, this code path is only used by `Cython/Build/BuildExecutable.py`

@user2:
Not sure what you two mean here. What suffix is missing where? The `name` shouldn't have one, and the lookups don't expect one.

@author:
ok, will remove the comments

diff_hunk: @@ -66,7 +68,7 @@ def __init__(self, include_directories, compiler_directives, cpp=False,
         self.modules = {"__builtin__" : Builtin.builtin_scope}
         self.cython_scope = CythonScope.create_cython_scope(self)
         self.modules["cython"] = self.cython_scope
-        self.include_directories = include_directories
+        self.include_directories = tuple(include_directories)
path: Cython/Compiler/Main.py
text: @user1:
Does this really need to be a tuple? It seems like it should rather be a list, at least at this point, but also generally. Make a copy when necessary, but why use a tuple?

@author:
ok, changing to list

@author:
it has to be a tuple to be hashable. Reverting

@user2:
Where do you need it to be hashable?

@author:
`search_include_directories` is marked as `Utils.cached_function` on line 606
```
@user3.cached_function
def search_include_directories(dirs, qualified_name, suffix, pos, include=False):
```

@user1:
Then it's probably enough to convert it into a tuple right before passing it into the function, right?
I mean, the right solution might actually be to convert everything to tuples inside of `cached_function()`, but in any case, we shouldn't use surprising data structures in multiple places just because one place that uses them requires a certain type as a side-effect of an optimisation.

@author:
Would you prefer `args = [tuple(x) for x in args if isinstance(x, list) else x]` in `cached_function`? I reduced the "multiple places to the call to `search_include_directories` and added a comment.