Replace use of module_list and OptionalExtension by extending find_python_sources #29701

mkoeppe · 2020-05-17T16:26:04Z

We add two new features to find_python_sources: finding Cython extensions, and filtering by "distributions".

We remove the use of module_list, finding Cython extensions instead in the source tree. (This is prepared by #29706 and follow-up tickets by moving Extension options to directives in the source files.)

We remove OptionalExtensions as follows. We map installed packages to "distributions" (for example, tdlib -> sage-tdlib) and then filter by distribution.

Follow-up tickets:

as part of Meta-ticket Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705, we will make these "distributions" actually separate distutils packages that provide namespace packages (Modify find_python_sources to support modularization of sagelib by native namespace packages (PEP 420) #28925)
remove the file module_list.py (not done on this ticket to avoid possible merge conflicts), deprecate OptionalExtension (which may be in use by user packages?)

CC: @kiwifb @isuruf @videlec @dcoudert @dimpase @kliem @vbraun

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 55c3fbc

Reviewer: Jonathan Kliem

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

The text was updated successfully, but these errors were encountered:

mkoeppe · 2020-06-03T22:09:56Z

Dependencies: #29411, #29702, #29706, #29720, #29721, #29785, #29786, #29790, #29791, #28925

mkoeppe · 2020-06-03T22:10:32Z

Branch: u/mkoeppe/replace_use_of_module_list_optionalextension

mkoeppe · 2020-06-03T22:48:38Z

Commit: 891d12a

mkoeppe · 2020-06-03T22:48:38Z

Last 10 new commits:

`4365e5d`	`Merge branch 't/29790/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_5__sage_graphs_' into t/29705/META-modularize-sagelib`
`9dc7022`	`Merge branch 't/29706/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files' into t/29791/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_6__last_`
`f78b06d`	`src/module_list.py: Move options for Extensions in sage.libs to distutils directives`
`1b0e29d`	`src/module_list.py: Move options for Extensions in sage.matrix to distutils directives`
`6421e2c`	`src/module_list.py: Move remaining options for Extensions in sage.libs, sage.rings to distutils directives`
`b3d3d2f`	`Merge branch 't/29791/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_6__last_' into t/29705/META-modularize-sagelib`
`2821934`	`Fix sage_setup directives: Use distribution, not package`
`9052db4`	`Merge branch 't/29720/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_2___optionalextensions_' into t/29705/META-modularize-sagelib`
`ff710ee`	`src/sage_setup/optional_extension.py (is_package_installed_and_updated): Factor out from OptionalExtension`
`891d12a`	`src/setup.py: Remove use of module_list.py; filter by distributions`

mkoeppe · 2020-06-03T22:48:38Z

Changed dependencies from #29411, #29702, #29706, #29720, #29721, #29785, #29786, #29790, #29791, #28925 to #29411, #29702, #29706, #29720, #29721, #29785, #29786, #29790, #29791

mkoeppe · 2020-06-03T22:50:55Z

Changed dependencies from #29411, #29702, #29706, #29720, #29721, #29785, #29786, #29790, #29791 to #29702 (#29411), #29706, #29720, #29721, #29785, #29786, #29790, #29791

sagetrac-git · 2020-06-03T22:53:24Z

Changed commit from 891d12a to ec7e9c5

sagetrac-git · 2020-06-03T22:53:24Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`0295c8f`	`src/module_list.py: Move options for Extensions in sage.graphs.graph_decompositions to distutils directives`
`b582789`	`src/sage_setup/find.py: Filter by directive 'sage_setup: distribution = PKG', find Cython modules`
`63c64d5`	Merge branches 't/29720/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_2___optionalextensions_', 't/29721/spkg_configure_m4_for_coxeter3', 't/29785/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_3__get_rid_of_uname_specific_', 't/29786/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_4__sage_rings_', 't/29790/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_5__sage_graphs_' and 't/29791/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files__part_6__last_' into t/29701/replace_use_of_module_list_optionalextension
`ae70c81`	`src/sage_setup/optional_extension.py (is_package_installed_and_updated): Factor out from OptionalExtension`
`ec7e9c5`	`src/setup.py: Remove use of module_list.py; filter by distributions`

mkoeppe · 2020-06-03T22:54:40Z

comment:15

No longer on top of #28925, so no namespace package are involved any more for this ticket.

mkoeppe · 2020-06-03T22:56:38Z

Author: Matthias Koeppe

dimpase · 2020-08-04T22:56:31Z

comment:63

Replying to @mkoeppe:

#30260 is broken. It is confused about whether it wants sage.graphs.distance_regular or sage.graphs.generators.distance_regular

it used to be broken, but IMHO it's fixed by #30240 comment:32

kiwifb · 2020-08-04T22:58:16Z

comment:64

Replying to @dimpase:

Replying to @mkoeppe:

#30260 is broken. It is confused about whether it wants sage.graphs.distance_regular or sage.graphs.generators.distance_regular

it used to be broken, but IMHO it's fixed by #30240 comment:32

Unfortunately Volker merged before that.

mkoeppe · 2020-08-04T23:00:49Z

comment:65

#30240 does not even have #30260 as a dependency...

Ivo-Maffei · 2020-08-05T09:03:23Z

comment:66

Replying to @mkoeppe:

#30240 does not even have #30260 as a dependency...

Should it? #30260 is a later ticket and it depends on #30240, but #30240 doesn't rely on #30260.
I apologise for the confusion but when #30240 went from "positive review" to "needs work" and I forgot to update #30260.

mkoeppe · 2020-08-05T14:30:16Z

comment:67

Replying to @Ivo-Maffei:

Replying to @mkoeppe:

#30240 does not even have #30260 as a dependency...

Should it? #30260 is a later ticket and it depends on #30240, but #30240 doesn't rely on #30260.
I apologise for the confusion but when #30240 went from "positive review" to "needs work" and I forgot to update #30260.

That's alright -- thanks for the fast reaction and fix!

vbraun · 2020-08-08T09:16:50Z

comment:68

sage -t --long --warn-long 42.4 --random-seed=0 src/sage_setup/find.py
**********************************************************************
File "src/sage_setup/find.py", line 215, in sage_setup.find.find_extra_files
Failed example:
    extras["sage/ext/interpreters"]
Expected:
    ['.../src/sage/ext/interpreters/wrapper_cdf.pxd', ...wrapper_cdf.h...]
Got:
    ['/home/release/Sage/src/sage/ext/interpreters/wrapper_cc.pxd',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_cc.pyx',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_cdf.pxd',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_cdf.pyx',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_el.pxd',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_el.pyx',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_py.pxd',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_py.pyx',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_rdf.pxd',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_rdf.pyx',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_rr.pxd',
     '/home/release/Sage/src/sage/ext/interpreters/wrapper_rr.pyx']
**********************************************************************
1 item had failures:
   1 of   7 in sage_setup.find.find_extra_files
    [36 tests, 1 failure, 0.27 s]
----------------------------------------------------------------------
sage -t --long --warn-long 42.4 --random-seed=0 src/sage_setup/find.py  # 1 doctest failed
----------------------------------------------------------------------

vbraun · 2020-08-08T12:23:12Z

comment:69

Might have been from #29950

mkoeppe · 2020-08-08T20:41:37Z

comment:70

Yes, it's #29950 that broke it.

vbraun · 2020-08-10T18:50:17Z

Changed branch from u/mkoeppe/replace_use_of_module_list_optionalextension to 55c3fbc

The implementation of sage.misc.package_dir.read_distributions uses the function `Cython.Utils.open_source_file()` to open a file instead of `io.open()`. This function was introduced in sagemath#29701 (b582789). The only difference between `open_source_file()` and `io.open()` is that the former will open the file as binary to check the encoding. But all our files should be utf-8 so we don't need this trick.

@mkoeppe

…dist The implementation of sage.misc.package_dir.read_distributions uses the function `Cython.Utils.open_source_file()` to open a file instead of `io.open()`. This function was introduced in sagemath#29701 (b582789). The only difference between `open_source_file()` and `io.open()` is that the former will open the file as binary to check the encoding. But all our files should be utf-8 so we don't need this trick. In case I’m mistaken and there is a reason this trick is really necessary (@mkoeppe?) it seems better to include a version of open_source_file(), for two main reasons: (a) we don’t pull the whole of Cython just for a trivial function; (b) changes in Cython don’t affect sagemath (this is not a documented function of Cython, afaict) ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. URL: sagemath#37290 Reported by: Gonzalo Tornaría Reviewer(s): Gonzalo Tornaría, Matthias Köppe

mkoeppe added this to the sage-9.2 milestone May 17, 2020

mkoeppe added t: refactoring labels May 17, 2020

This comment has been minimized.

Sign in to view

mkoeppe changed the title ~~Replace use of OptionalExtension by namespace packages~~ Meta-ticket: Replace use of OptionalExtension by namespace packages May 17, 2020

This comment has been minimized.

Sign in to view

mkoeppe changed the title ~~Meta-ticket: Replace use of OptionalExtension by namespace packages~~ Replace use of module_list and OptionalExtension by extending find_python_sources Jun 3, 2020

This comment has been minimized.

Sign in to view

mkoeppe added the s: needs review label Jun 3, 2020

This comment has been minimized.

Sign in to view

mkoeppe added the s: positive review label Aug 4, 2020

vbraun added s: needs work and removed s: positive review labels Aug 8, 2020

mkoeppe added s: positive review and removed s: needs work labels Aug 8, 2020

vbraun removed the s: positive review label Aug 10, 2020

vbraun closed this as completed in 2c85de4 Aug 10, 2020

mkoeppe mentioned this issue Jan 4, 2024

Compile everything with meson #36524

Open

5 tasks

tornaria mentioned this issue Feb 11, 2024

sagemath-standard: don't require Cython to create sdist #37290

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of module_list and OptionalExtension by extending find_python_sources #29701

Replace use of module_list and OptionalExtension by extending find_python_sources #29701

mkoeppe commented May 17, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mkoeppe commented Jun 3, 2020

This comment has been minimized.

mkoeppe commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

This comment has been minimized.

mkoeppe commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

sagetrac-git mannequin commented Jun 3, 2020

sagetrac-git mannequin commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

This comment has been minimized.

mkoeppe commented Jun 3, 2020

This comment has been minimized.

dimpase commented Aug 4, 2020

kiwifb commented Aug 4, 2020

mkoeppe commented Aug 4, 2020

Ivo-Maffei mannequin commented Aug 5, 2020

mkoeppe commented Aug 5, 2020

vbraun commented Aug 8, 2020

vbraun commented Aug 8, 2020

mkoeppe commented Aug 8, 2020

vbraun commented Aug 10, 2020

Replace use of module_list and OptionalExtension by extending find_python_sources #29701

Replace use of module_list and OptionalExtension by extending find_python_sources #29701

Comments

mkoeppe commented May 17, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mkoeppe commented Jun 3, 2020

This comment has been minimized.

mkoeppe commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

This comment has been minimized.

mkoeppe commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

sagetrac-git mannequin commented Jun 3, 2020

sagetrac-git mannequin commented Jun 3, 2020

mkoeppe commented Jun 3, 2020

This comment has been minimized.

mkoeppe commented Jun 3, 2020

This comment has been minimized.

dimpase commented Aug 4, 2020

kiwifb commented Aug 4, 2020

mkoeppe commented Aug 4, 2020

Ivo-Maffei mannequin commented Aug 5, 2020

mkoeppe commented Aug 5, 2020

vbraun commented Aug 8, 2020

vbraun commented Aug 8, 2020

mkoeppe commented Aug 8, 2020

vbraun commented Aug 10, 2020