sageinspect.sage_getfile(x) incorrect when using an alternate suffix for extension modules

[tornaria]

Python has in importlib.machinery.EXTENSION_SUFFIXES a list of strings representing the recognized file suffixes for extension modules.

Sagemath defines loadable_module_extension() as the first entry in the list, and assumes extension modules use this suffix only.

This is normally ok since python setuptools uses that particular suffix when compiling extension modules.

However, this may not always be true. For example in void linux the extension modules are renamed to use the shortest suffix (which is .so, instead of .cpython-310-x86_64-linux-gnu.so).

I don't know what's the rationale behind that rename, but it's "legal" from pov of python, so it'd be nice if this was supported in sage.

There is one doctest failure on this situation:

**********************************************************************
File "src/sage/misc/sageinspect.py", line 186, in sage.misc.sageinspect.loadable_module_extension
Failed example:
    sage.structure.sage_object.__file__.endswith(loadable_module_extension())
Expected:
    True
Got:
    False
**********************************************************************

Moreover, this causes another (more serious) issue in sage.misc.sageinspect.sage_getfile, namely:

sage: sage.misc.sageinspect.sage_getfile(x)
'/opt/sage/sage-git/develop/pkgs/sagemath-standard/build/lib.linux-x86_64-3.10/sage/symbolic/expression.so'

The expected output is the full path to expression.pyx instead of .so.

It turns out this is handled by a fallback in sage_getfile that is not covered by any doctest.

Purpose of this ticket is
a. add a doctest that exercises the above example so it triggers this incorrect behaviour.
b. fix the incorrect behaviour of sage_getfile
c. look for other possible incorrect uses of loadable_module_extension() which are making the same assumption

Component: porting

Author: Gonzalo Tornaría

Branch/Commit: fd46c83

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33626

[mkoeppe]

comment:1

I think we should deprecate loadable_module_extension and replace all uses by just using importlib.machinery.EXTENSION_SUFFIXES.

[tornaria]

comment:2

This fixes all the doctest failures in the void linux package (where, per distro policy, python extension modules are renamed from %.cpython*.so to just %.so).

After this, loadable_module_extension() is only used (twice) in sage.misc.cython.cython().
For these I have this tentative change:

--- a/src/sage/misc/cython.py
+++ b/src/sage/misc/cython.py
@@ -240,13 +240,20 @@ def cython(filename, verbose=0, compile_message=False,
         # There is already a module here. Maybe we do not have to rebuild?
         # Find the name.
         if use_cache:
-            from sage.misc.sageinspect import loadable_module_extension
-            prev_so = [F for F in os.listdir(target_dir) if F.endswith(loadable_module_extension())]
-            if len(prev_so) > 0:
-                prev_so = prev_so[0]     # should have length 1 because of deletes below
-                if os.path.getmtime(filename) <= os.path.getmtime('%s/%s' % (target_dir, prev_so)):
+            from importlib.machinery import EXTENSION_SUFFIXES
+            for f in os.listdir(target_dir):
+                for suffix in EXTENSION_SUFFIXES:
+                    if f.endswith(suffix):
+                        # use the first matching extension
+                        prev_file = os.path.join(target_dir, f)
+                        prev_name = f[:-len(suffix)]
+                        break
+                else:
+                    # no match, try next file
+                    continue
+                if os.path.getmtime(filename) <= os.path.getmtime(prev_file):
                     # We do not have to rebuild.
-                    return prev_so[:-len(loadable_module_extension())], target_dir
+                    return prev_name, target_dir
 
         # Delete all ordinary files in target_dir
         for F in os.listdir(target_dir):
@@ -410,9 +417,11 @@ def cython(filename, verbose=0, compile_message=False,
 
     if create_local_so_file:
         # Copy module to current directory
-        from sage.misc.sageinspect import loadable_module_extension
-        shutil.copy(os.path.join(target_dir, name + loadable_module_extension()),
-                    os.curdir)
+        from importlib.machinery import EXTENSION_SUFFIXES
+        for ext in EXTENSION_SUFFIXES:
+            path = os.path.join(target_dir, name + ext)
+            if os.path.exists(path):
+                shutil.copy(path, os.curdir)
 
     return name, target_dir
 

Notes:

• The option use_cache is only used in sage.repl.load.load_cython() and I'm not sure there's any doctest that will run this code.
• The option create_local_so_file is not used anywhere, and it's not doctested.
• If both options are given together and the compiled .so file is found in cache, it will not save a copy in the current directory (seems to be a bug).

---

New commits:

f93188d	Trac #33626: add doctest for coverage of sage_getfile
de413b7	Trac #33626: fix sage_getfile by checking all possible EXTENSION_SUFFIXES
c5bb7ff	Trac #33626: don't use loadable_module_extension() in sage_setup
fd46c83	Trac #33626: fix doctest for loadable_module_extension

[tornaria]

Author: Gonzalo Tornaría

[tornaria]

Commit: fd46c83

[tornaria]

Branch: u/tornaria/33626

[tornaria]

comment:3

Follow-up in #33636, since the change in cython() is more complicated, affects untested code, and it's not urgent.

The changes in this ticket seem very simple and clear, are doctested, and fix actual issues.

[mkoeppe]

Reviewer: Matthias Koeppe

[vbraun]

Changed branch from u/tornaria/33626 to fd46c83