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

replace loadable_module_extension() by importlib.machinery.EXTENSION_SUFFIXES #33636

Closed
tornaria opened this issue Apr 3, 2022 · 11 comments
Closed

Comments

@tornaria
Copy link
Contributor

tornaria commented Apr 3, 2022

After #33626, 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=True 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=True is not used anywhere, and it's not doctested either.
  • 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).

Depends on #33626

Component: refactoring

Author: Gonzalo Tornaría

Branch/Commit: 780be6a

Reviewer: Matthias Koeppe

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

@tornaria tornaria added this to the sage-9.6 milestone Apr 3, 2022
@tornaria
Copy link
Contributor Author

tornaria commented Apr 3, 2022

Commit: 64dfaf3

@tornaria
Copy link
Contributor Author

tornaria commented Apr 3, 2022

Branch: u/tornaria/33636

@tornaria
Copy link
Contributor Author

tornaria commented Apr 3, 2022

comment:1

Committed the patch above just for convenience. Not sure if this is ready, it's probably desirable to add some direct tests for these options.


New commits:

f93188dTrac #33626: add doctest for coverage of sage_getfile
de413b7Trac #33626: fix sage_getfile by checking all possible EXTENSION_SUFFIXES
c5bb7ffTrac #33626: don't use loadable_module_extension() in sage_setup
fd46c83Trac #33626: fix doctest for loadable_module_extension
64dfaf3Trac #33636: don't use loadable_module_extension() in sage.misc.cython.cython()

@mkoeppe
Copy link
Member

mkoeppe commented Apr 3, 2022

Author: Gonzalo Tornaría

@mkoeppe
Copy link
Member

mkoeppe commented Apr 3, 2022

comment:3

Can you also please deprecate loadable_module_extension?

@mkoeppe
Copy link
Member

mkoeppe commented Apr 3, 2022

comment:4

+1 on adding a doctest for create_local_so_file=True

@mkoeppe
Copy link
Member

mkoeppe commented Jul 23, 2022

Changed branch from u/tornaria/33636 to u/mkoeppe/33636

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Changed commit from 64dfaf3 to 780be6a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

780be6asrc/sage/misc/sageinspect.py: Deprecate loadable_module_extension

@mkoeppe
Copy link
Member

mkoeppe commented Jul 23, 2022

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Jul 28, 2022

Changed branch from u/mkoeppe/33636 to 780be6a

@vbraun vbraun closed this as completed in 977e691 Jul 28, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 25, 2024
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 27, 2024
vbraun pushed a commit to vbraun/sage that referenced this issue May 9, 2024
…e_module_extension`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- Deprecated in sagemath#33636 (2022)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37867
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue May 11, 2024
…e_module_extension`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- Deprecated in sagemath#33636 (2022)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37867
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue May 12, 2024
…e_module_extension`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- Deprecated in sagemath#33636 (2022)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37867
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue May 12, 2024
…e_module_extension`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- Deprecated in sagemath#33636 (2022)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37867
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants