Replace use of module_list and OptionalExtension by extending find_python_sources

mkoeppe commented 4 years ago

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 #29705, we will make these "distributions" actually separate distutils packages that provide namespace packages (#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

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1 +1,4 @@
+Preparation:
+- #28925 Modify clean_stale_files to support modularization of sagelib by namespace packages

+

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
 Preparation:
 - #28925 Modify clean_stale_files to support modularization of sagelib by namespace packages
+- #29702 Move all code from src/setup.py to sage_setup

-

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,16 @@
 Preparation:
-- #28925 Modify clean_stale_files to support modularization of sagelib by namespace packages
-- #29702 Move all code from src/setup.py to sage_setup
+- #28925 Modify `clean_stale_files` to support modularization of `sagelib` by namespace packages
+- #29702 Move all code from `src/setup.py` to `sage_setup`

+Tickets for individual `OptionalExtension`s (see `src/module_list.py`):
+- `sage.graphs.mcqd`
+- `sage.graphs.bliss`
+- `sage.graphs.graph_decompositions.tdlib`
+- `sage.interfaces.primecount` (how come this is not in `sage.libs`, where `primecount.pxd` lives?)
+- `sage.libs.coxeter3.coxeter`
+- `sage.libs.fes`
+- `sage.libs.sirocco`
+- `sage.libs.meataxe`, `sage.matrix.matrix_gfpn_dense`
+
+
+

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -5,7 +5,7 @@
 Tickets for individual `OptionalExtension`s (see `src/module_list.py`):
 - `sage.graphs.mcqd`
 - `sage.graphs.bliss`
-- `sage.graphs.graph_decompositions.tdlib`
+- `sage.graphs.graph_decompositions.tdlib` (used as an example in #28925)
 - `sage.interfaces.primecount` (how come this is not in `sage.libs`, where `primecount.pxd` lives?)
 - `sage.libs.coxeter3.coxeter`
 - `sage.libs.fes`

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,6 @@
 Preparation:
 - #28925 Modify `clean_stale_files` to support modularization of `sagelib` by namespace packages
+- Remove empty  `__init__.py` files to convert packages to native namespace packages
 - #29702 Move all code from `src/setup.py` to `sage_setup`

 Tickets for individual `OptionalExtension`s (see `src/module_list.py`):

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,9 @@
+Currently, a user would install, for example, the optional package `tdlib` and then rebuild `sagelib` so that the `OptionalExtension` `sage.graphs.graph_decompositions.tdlib` is built and installed.
+
+With this ticket, the user would instead install a new optional package `sage_tdlib` (which has `tdlib` as a dependency); this installs the `Extension` `sage.graphs.graph_decompositions.tdlib` (as a namespace package).
+
 Preparation:
-- #28925 Modify `clean_stale_files` to support modularization of `sagelib` by namespace packages
+- #28925 Modify find_python_sources, clean_stale_files to support modularization of sagelib by native namespace packages (PEP 420)
 - Remove empty  `__init__.py` files to convert packages to native namespace packages
 - #29702 Move all code from `src/setup.py` to `sage_setup`

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -3,7 +3,7 @@
 With this ticket, the user would instead install a new optional package `sage_tdlib` (which has `tdlib` as a dependency); this installs the `Extension` `sage.graphs.graph_decompositions.tdlib` (as a namespace package).

 Preparation:
-- #28925 Modify find_python_sources, clean_stale_files to support modularization of sagelib by native namespace packages (PEP 420)
+- #28925 Modify `find_python_sources`, `clean_stale_files` to support modularization of sagelib by native namespace packages (PEP 420)
 - Remove empty  `__init__.py` files to convert packages to native namespace packages
 - #29702 Move all code from `src/setup.py` to `sage_setup`

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -4,6 +4,7 @@

 Preparation:
 - #28925 Modify `find_python_sources`, `clean_stale_files` to support modularization of sagelib by native namespace packages (PEP 420)
+- Map installed packages/features to virtual `sage_packages` (for example, `tdlib` -> `sage_tdlib`), replace `module_list.py` and the `OptionalExtension` mechanism by passing `sage_packages` to `find_python_sources`.
 - Remove empty  `__init__.py` files to convert packages to native namespace packages
 - #29702 Move all code from `src/setup.py` to `sage_setup`

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -4,7 +4,7 @@

 Preparation:
 - #28925 Modify `find_python_sources`, `clean_stale_files` to support modularization of sagelib by native namespace packages (PEP 420)
-- Map installed packages/features to virtual `sage_packages` (for example, `tdlib` -> `sage_tdlib`), replace `module_list.py` and the `OptionalExtension` mechanism by passing `sage_packages` to `find_python_sources`.
+- Map installed packages/features to virtual `distributions` (for example, `tdlib` -> `sage-tdlib`), replace `module_list.py` and the `OptionalExtension` mechanism by passing `distributions` to `find_python_sources`.
 - Remove empty  `__init__.py` files to convert packages to native namespace packages
 - #29702 Move all code from `src/setup.py` to `sage_setup`

mkoeppe commented 4 years ago

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

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,22 +1,7 @@
-Currently, a user would install, for example, the optional package `tdlib` and then rebuild `sagelib` so that the `OptionalExtension` `sage.graphs.graph_decompositions.tdlib` is built and installed.
+Using the extended `find_python_sources` from #28925, 
+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.)

-With this ticket, the user would instead install a new optional package `sage_tdlib` (which has `tdlib` as a dependency); this installs the `Extension` `sage.graphs.graph_decompositions.tdlib` (as a namespace package).
+We remove `OptionalExtension`s as follows. We map installed packages to "distributions" (for example, `tdlib` -> `sage-tdlib`) and then filter by distribution.

-Preparation:
-- #28925 Modify `find_python_sources`, `clean_stale_files` to support modularization of sagelib by native namespace packages (PEP 420)
-- Map installed packages/features to virtual `distributions` (for example, `tdlib` -> `sage-tdlib`), replace `module_list.py` and the `OptionalExtension` mechanism by passing `distributions` to `find_python_sources`.
-- Remove empty  `__init__.py` files to convert packages to native namespace packages
-- #29702 Move all code from `src/setup.py` to `sage_setup`
+(In a follow-up ticket, part of Meta-ticket #29705, we will make these "distributions" actually separate distutils packages.)

-Tickets for individual `OptionalExtension`s (see `src/module_list.py`):
-- `sage.graphs.mcqd`
-- `sage.graphs.bliss`
-- `sage.graphs.graph_decompositions.tdlib` (used as an example in #28925)
-- `sage.interfaces.primecount` (how come this is not in `sage.libs`, where `primecount.pxd` lives?)
-- `sage.libs.coxeter3.coxeter`
-- `sage.libs.fes`
-- `sage.libs.sirocco`
-- `sage.libs.meataxe`, `sage.matrix.matrix_gfpn_dense`
-
-
-

mkoeppe commented 4 years ago

Branch: u/mkoeppe/replace_use_of_module_list_optionalextension

mkoeppe commented 4 years ago

Commit: 891d12a

mkoeppe commented 4 years ago

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 commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,6 @@
-Using the extended `find_python_sources` from #28925, 
-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 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 `OptionalExtension`s as follows. We map installed packages to "distributions" (for example, `tdlib` -> `sage-tdlib`) and then filter by distribution.

mkoeppe commented 4 years ago

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

mkoeppe commented 4 years ago

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 891d12a to ec7e9c5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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 commented 4 years ago

comment:15

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

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -4,5 +4,9 @@

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

-(In a follow-up ticket, part of Meta-ticket #29705, we will make these "distributions" actually separate distutils packages.)
+Follow-up tickets:
+- as part of Meta-ticket #29705, we will make these "distributions" actually separate distutils packages
+- 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?)

+
+

mkoeppe commented 4 years ago

Author: Matthias Koeppe

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 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 `OptionalExtension`s as follows. We map installed packages to "distributions" (for example, `tdlib` -> `sage-tdlib`) and then filter by distribution.

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -5,7 +5,7 @@
 We remove `OptionalExtension`s 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 #29705, we will make these "distributions" actually separate distutils packages
+- as part of Meta-ticket #29705, we will make these "distributions" actually separate distutils packages that provide namespace packages (#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?)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from ec7e9c5 to 25a2340

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

`38b6bcf`	`Merge tag '9.2.beta0' into t/29411/make_sagelib_a_script_package`
`f9a30f6`	`build/pkgs/sagelib/spkg-install: Fix up error exits`
`00a1d57`	`Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`25a2340`	`Merge branch 't/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup' into t/29701/replace_use_of_module_list_optionalextension`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 25a2340 to 2d866a3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

2d866a3 src/sage/graphs/graph_decompositions/tdlib.pyx: Fixup

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

`b1b3787`	`sage.env.cython_aliases: Fix for systems without zlib pc`
`802356a`	`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_`
`3beb5b7`	`Merge branch '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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 2d866a3 to 3beb5b7

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 3beb5b7 to 7f8850a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

7f8850a src/module_list.py: Document that it is obsolete

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`5db5318`	`Trac #29345: remove "break" statements from AC_SEARCH_LIBS.`
`e810ad1`	`Trac #29345: don't use sage's config.status for the lrcalc build.`
`93c9921`	`Trac #29345: replace the function that populates the CVXOPT_* variables.`
`0e66a0a`	`Trac #29345: add Dima's SPKG patches for ksh compatibility.`
`df3f05e`	`build/make/Makefile.in [SCRIPT_PACKAGE_templ]: cd into the SPKG directory; adjust spkg-install scripts`
`5372065`	`Merge branch 't/29793/script_packages_should_cd_into_the_spkg_directory' into t/29411/make_sagelib_a_script_package`
`c166b97`	`Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`cc30471`	`build/bin/write-dockerfile.sh: Do not ADD removed file src/Makefile.in`
`8a41326`	`Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`26a85a4`	`Merge branch 't/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup' into t/29701/replace_use_of_module_list_optionalextension`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 7f8850a to 26a85a4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

`5d5803e`	`src/sage/graphs/planarity.pyx: Add forgotten distutils directive`
`a0be9b6`	`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/29701/replace_use_of_module_list_optionalextension`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 26a85a4 to a0be9b6

mkoeppe commented 4 years ago

comment:25

Tests run at https://github.com/mkoeppe/sage/actions/runs/128112212

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from a0be9b6 to 14396af

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

14396af Merge tag '9.2.beta2' into t/29701/replace_use_of_module_list_optionalextension

mkoeppe commented 4 years ago

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

mkoeppe commented 4 years ago

comment:27

Merged in latest beta. Needs review

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 14396af to d62da15

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

d62da15 sage_setup.command.sage_build: Add the extensions to the distribution

kliem commented 4 years ago

comment:29

I'm a bit confused:

find_python_sources gives you all sources including all with optional packages if distributions=None.

However, if distributions is sage-tdlib, you get only one source file.

Don't we want find_python_sources to contain everything except for not installed optional packages?

mkoeppe commented 4 years ago

comment:30

Replying to @kliem:

find_python_sources gives you all sources including all with optional packages if distributions=None.

However, if distributions is sage-tdlib, you get only one source file.

That's right. This is a feature for modularization as in #29864 (Modularization of sagelib: Break out a separate package sage-tdlib).

Don't we want find_python_sources to contain everything except for not installed optional packages?

For this, one passes distributions=['', 'sage-tdlib']. The empty string designates all source files without a sage_setup: distribution directive. This list of distributions is computed in src/setup.py.

kliem commented 4 years ago

comment:31

I see.

I would suggest updating the benchmark. Also I can confirm this, it is not really meaningful, as this is not the normal use case, isn't it. The normal use case would be with distributions=[''] and this takes about 500 ms and not 20 ms. Still no problem I think, but much longer.

kliem commented 4 years ago

comment:32

Otherwise the source code looks fine. Do we have any test runs? Should I run them?

mkoeppe commented 4 years ago

comment:33

I have tested the branch of this ticket on macOS and also as part of #29950 and other tickets.

mkoeppe commented 4 years ago

comment:34

Replying to @kliem:

I would suggest updating the benchmark.

Good idea, will do

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

06a3609 sage_setup.find.find_python_sources: Add benchmark doctest

sagemath / sage

Replace use of module_list and OptionalExtension by extending find_python_sources #29701