sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 480 forks source link

Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1 - some packages without OptionalExtensions) #29706

Closed mkoeppe closed 4 years ago

mkoeppe commented 4 years ago

This simplifies src/module_list.py a lot. This is preparation for splitting sagelib into separate distutils packages.

Follow-up tickets take care of the remaining packages; in the end, in #29701, src/module_list.py will no longer be used, and in another follow-up ticket, it will disappear completely:

CC: @kiwifb @dimpase @jhpalmieri @tscrim @kliem @roed314 @videlec @vbraun

Component: refactoring

Keywords: sd109

Author: Matthias Koeppe

Branch/Commit: 1bfe30d

Reviewer: Jonathan Kliem, John Palmieri

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

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+This would simplify src/module_list.py a lot.
mkoeppe commented 4 years ago

Branch: u/mkoeppe/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files

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

Commit: f31b6a0

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

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

f31b6a0src/sage/geometry: Move Extension options from src/module_list.py to distutils directives
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from f31b6a0 to 346bd88

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

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

0c0ef43src/sage/libs/ntl: Move Extension options from src/module_list.py to distutils directives
a4d04d3src/sage/modular: Move Extension options from src/module_list.py to distutils directives
17ca1d7src/sage/env.py (cython_aliases): Add aliases for libraries use in module_list.py
346bd88src/sage/modules: Move Extension options from src/module_list.py to distutils directives
mkoeppe commented 4 years ago

Author: Matthias Koeppe

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
-This would simplify src/module_list.py a lot.
+This simplifies src/module_list.py a lot.
+
+
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 This simplifies src/module_list.py a lot.

+Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely.

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

Changed commit from 346bd88 to 70901ea

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

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

70901easrc/sage/env.py (cython_aliases): Update doctest
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-This simplifies src/module_list.py a lot.
+This simplifies `src/module_list.py` a lot. This is preparation for splitting sagelib into separate distutils packages.

 Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely.
dimpase commented 4 years ago
comment:10

Aren't all these extra_compile_args=['-std=c++11'] (and the corresponding cython #-directives obsolete (as we now set this globally for the C++ compiler)?

mkoeppe commented 4 years ago
comment:11

I would guess so but would prefer to do that on a different ticket

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

Changed commit from 70901ea to 10f7542

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

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

eed920esrc/sage/tests: Move Extension options from src/module_list.py to distutils directives
0d25d1dsrc/sage/structure: Move Extension options from src/module_list.py to distutils directives
808f46asrc/sage/stats: Move Extension options from src/module_list.py to distutils directives
10f7542src/sage/schemes: Move Extension options from src/module_list.py to distutils directives
kliem commented 4 years ago
comment:14

Isn't src/sage/geometry/triangulation/base.pyx itself redundant here?

-# distutils: sources = sage/geometry/triangulation/base.pyx sage/geometry/triangulation/functions.cc sage/geometry/triangulation/data.cc sage/geometry/triangulation/triangulations.cc
+# distutils: sources = sage/geometry/triangulation/functions.cc sage/geometry/triangulation/data.cc sage/geometry/triangulation/triangulations.cc

Same thing in

I think you missed this one. At least I don't find the corresponding file in the diffs.

-    Extension('sage.modular.pollack_stevens.dist',
-              sources = ['sage/modular/pollack_stevens/dist.pyx'],
-              libraries = ["gmp", "zn_poly"],
-              extra_compile_args = ["-D_XPG6"]),

I don't understand (this is probably reasonable, but I don't have any knowledge about that)

    # TODO: Remove Cygwin hack by installing a suitable cblas.pc
    if os.path.exists('/usr/lib/libblas.dll.a'):
        aliases["CBLAS_LIBS"] = ['gslcblas']

Everything else looks great to me. However, I didn't test anything yet.

kliem commented 4 years ago

Changed keywords from none to sd109

mkoeppe commented 4 years ago
comment:16

Replying to @kliem:

Isn't src/sage/geometry/triangulation/base.pyx itself redundant here?

-# distutils: sources = sage/geometry/triangulation/base.pyx sage/geometry/triangulation/functions.cc sage/geometry/triangulation/data.cc sage/geometry/triangulation/triangulations.cc
+# distutils: sources = sage/geometry/triangulation/functions.cc sage/geometry/triangulation/data.cc sage/geometry/triangulation/triangulations.cc

Same thing in

  • /src/sage/schemes/hyperelliptic_curves/hypellfrob.pyx
  • sage/stats/distributions/discrete_gaussian_integer.pyx)

I think you missed this one. At least I don't find the corresponding file in the diffs.

-    Extension('sage.modular.pollack_stevens.dist',
-              sources = ['sage/modular/pollack_stevens/dist.pyx'],
-              libraries = ["gmp", "zn_poly"],
-              extra_compile_args = ["-D_XPG6"]),

Quite possible on both of these. If you have time to test and make these fixes, that would be great!

I don't understand (this is probably reasonable, but I don't have any knowledge about that)

    # TODO: Remove Cygwin hack by installing a suitable cblas.pc
    if os.path.exists('/usr/lib/libblas.dll.a'):
        aliases["CBLAS_LIBS"] = ['gslcblas']

I don't understand it either, I only it moved it there from its origin. It's quite likely that it is outdated - this should be addressed on a separate ticket.

kliem commented 4 years ago
comment:17

Ok. I'm testing it now both locally and with github actions.

kliem commented 4 years ago
comment:18

It all worked on my end (sage -t --all --long)

Testing it on github reavealed some strange error with debian buster standard.

https://github.com/kliem/sage-test-27122/runs/722743115

  [dochtml]     File "/sage/local/lib/python3.7/site-packages/sage/matrix/__init__.py", line 2, in <module>
  [dochtml]       import sage.matrix.args
  [dochtml]     File "sage/matrix/args.pyx", line 23, in init sage.matrix.args (build/cythonized/sage/matrix/args.c:21224)
  [dochtml]     File "/sage/local/lib/python3.7/site-packages/sage/matrix/matrix_space.py", line 44, in <module>
  [dochtml]       from . import matrix_modn_sparse
  [dochtml]     File "sage/matrix/matrix_integer_sparse.pxd", line 5, in init sage.matrix.matrix_modn_sparse (build/cythonized/sage/matrix/matrix_modn_sparse.cpp:16301)
  [dochtml]   ImportError: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.26' not found (required by /sage/local/lib/python3.7/site-packages/sage/matrix/matrix_integer_sparse.cpython-

How come numerical, rings etc. wheren't touched?

dimpase commented 4 years ago
comment:19

I also see this kind of strange error on GH Actions on buster. Perhaps the buster image used is faulty?

kliem commented 4 years ago
comment:20

Also happens on ubuntu bionic and linuxmint 19.

Overall the testing experience is awful at the moment and it appears to have nothing to do with this ticket: https://github.com/kliem/sage-test-27122/actions/runs/119764262

Except for ubuntu focal and eoan and debian bullseye and sid (all standard not minimal) nothing passes within 6 hours and many failures.

kliem commented 4 years ago
comment:21

In comparison to almost everything succeeding at 9.1 release.

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 This simplifies `src/module_list.py` a lot. This is preparation for splitting sagelib into separate distutils packages.

-Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely.
+Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
+- #29720: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 2 - OptionalExtensions)

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

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

5867c05src/sage/modular/pollack_stevens/dist.pyx: Add missing distutils directives
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 10f7542 to 5867c05

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

Changed commit from 5867c05 to c536daa

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

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

c536daaRemove self-listing in distutils sources directive
mkoeppe commented 4 years ago
comment:25

Replying to @mkoeppe:

Replying to @kliem:

Isn't src/sage/geometry/triangulation/base.pyx itself redundant here?

-# distutils: sources = sage/geometry/triangulation/base.pyx sage/geometry/triangulation/functions.cc sage/geometry/triangulation/data.cc sage/geometry/triangulation/triangulations.cc
+# distutils: sources = sage/geometry/triangulation/functions.cc sage/geometry/triangulation/data.cc sage/geometry/triangulation/triangulations.cc

Same thing in

  • /src/sage/schemes/hyperelliptic_curves/hypellfrob.pyx
  • sage/stats/distributions/discrete_gaussian_integer.pyx)

I think you missed this one. At least I don't find the corresponding file in the diffs.

-    Extension('sage.modular.pollack_stevens.dist',
-              sources = ['sage/modular/pollack_stevens/dist.pyx'],
-              libraries = ["gmp", "zn_poly"],
-              extra_compile_args = ["-D_XPG6"]),

Quite possible on both of these.

I have made these changes.

mkoeppe commented 4 years ago
comment:26

Replying to @kliem:

How come numerical, rings etc. weren't touched?

The present ticket only touches top-level packages that do not have OptionalExtensions. Part 2 will do OptionalExtensions. Part 3 all remaining (mixed) ones. I split it up to reduce the potential for merge conflicts.

kliem commented 4 years ago
comment:27

As far as I can see OptionalExtension is used only in graphs, interfaces, libs and matrix.

Numerical, rings, finite_rings, number_field, padics and polynomial don't seem to have that.

Maybe I'm missing something. We can also split at this point, because we are already touching enough packages.

mkoeppe commented 4 years ago
comment:28

It's quite possible that I misremembered and it's just an arbitrary subset.

kliem commented 4 years ago
comment:30

Works for me.

As it apparently also works on your mac and it passes on cygwin https://github.com/kliem/sage-test-27122/actions/runs/119764261 and it works fine on multiple linux instances, the build process seems to work just the same as before.

kliem commented 4 years ago

Reviewer: Jonathan Kliem

mkoeppe commented 4 years ago
comment:31

Thanks!

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,4 @@

 Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
 - #29720: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 2 - OptionalExtensions)
-
+- #28925 takes care of `sage.numerical.backends`
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,4 @@

 Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
 - #29720: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 2 - OptionalExtensions)
-- #28925 takes care of `sage.numerical.backends`
+- #28925 takes care of `sage.numerical.backends` and `sage.graphs.graph_decompositions`
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,5 @@

 Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
 - #29720: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 2 - OptionalExtensions)
+- #29785: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 3: Get rid of `uname_specific`)
 - #28925 takes care of `sage.numerical.backends` and `sage.graphs.graph_decompositions`
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 This simplifies `src/module_list.py` a lot. This is preparation for splitting sagelib into separate distutils packages.

 Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
-- #29720: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 2 - OptionalExtensions)
-- #29785: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 3: Get rid of `uname_specific`)
+- #29720: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 2 - `OptionalExtension`s)
+- #29785: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 3: Get rid of `uname_specific`)
 - #28925 takes care of `sage.numerical.backends` and `sage.graphs.graph_decompositions`
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,8 @@
 Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
 - #29720: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 2 - `OptionalExtension`s)
 - #29785: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 3: Get rid of `uname_specific`)
-- #28925 takes care of `sage.numerical.backends` and `sage.graphs.graph_decompositions`
+- #29786: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 4: sage.rings)
+- #28925: takes care of `sage.numerical.backends` and `sage.graphs.graph_decompositions`
+- #29721: takes care of `sage.libs.coxeter3`
+
+
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,10 +1,11 @@
 This simplifies `src/module_list.py` a lot. This is preparation for splitting sagelib into separate distutils packages.

-Follow-up tickets will take care of the remaining packages; in the end, `src/module_list.py` might disappear completely:
+Follow-up tickets take care of the remaining packages; in the end, in #29701, `src/module_list.py` will no longer be used, and in another follow-up ticket, it will disappear completely:
 - #29720: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 2 - `OptionalExtension`s)
 - #29785: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 3: Get rid of `uname_specific`)
-- #29786: Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 4: sage.rings)
-- #28925: takes care of `sage.numerical.backends` and `sage.graphs.graph_decompositions`
+- #29786: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 4: `sage.rings`)
+- #29790: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 5: `sage.graphs`)
+- #29791: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 6: last)
 - #29721: takes care of `sage.libs.coxeter3`
+- #29701: takes care of `sage.graphs.graph_decompositions`

-
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,11 +1,11 @@
 This simplifies `src/module_list.py` a lot. This is preparation for splitting sagelib into separate distutils packages.

 Follow-up tickets take care of the remaining packages; in the end, in #29701, `src/module_list.py` will no longer be used, and in another follow-up ticket, it will disappear completely:
-- #29720: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 2 - `OptionalExtension`s)
-- #29785: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 3: Get rid of `uname_specific`)
-- #29786: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 4: `sage.rings`)
-- #29790: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 5: `sage.graphs`)
-- #29791: Move `Extension` options from `src/module_list.py` to "distutils:" directives in the individual files (part 6: last)
+- #29720: Move `Extension` options ... (part 2 - `OptionalExtension`s)
+- #29785: Move `Extension` options ... (part 3: Get rid of `uname_specific`)
+- #29786: Move `Extension` options ... (part 4: `sage.rings`)
+- #29790: Move `Extension` options ... (part 5: `sage.graphs`)
+- #29791: Move `Extension` options ... (part 6: last)
 - #29721: takes care of `sage.libs.coxeter3`
 - #29701: takes care of `sage.graphs.graph_decompositions`
vbraun commented 4 years ago

Changed branch from u/mkoeppe/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files to c536daa

vbraun commented 4 years ago

Changed commit from c536daa to none

vbraun commented 4 years ago
comment:40

on OSX:

[sagelib-9.2.beta0] cd . && export                                    \
[sagelib-9.2.beta0]         SAGE_ROOT=/doesnotexist                               \
[sagelib-9.2.beta0]         SAGE_SRC=/doesnotexist                                \
[sagelib-9.2.beta0]         SAGE_SRC_ROOT=/doesnotexist                           \
[sagelib-9.2.beta0]         SAGE_DOC_SRC=/doesnotexist                            \
[sagelib-9.2.beta0]         SAGE_BUILD_DIR=/doesnotexist                          \
[sagelib-9.2.beta0]         SAGE_PKGS=/Users/buildbot-sage/slave/sage_git/build/build/pkgs                \
[sagelib-9.2.beta0]     && sage-python -u setup.py --no-user-cfg build install
[sagelib-9.2.beta0] /Users/buildbot-sage/slave/sage_git/build/src/bin/sage-env: line 130: cd: /doesnotexist: No such file or directory
[sagelib-9.2.beta0] Warning: overwriting SAGE_ROOT environment variable:
[sagelib-9.2.beta0] Old SAGE_ROOT=/doesnotexist
[sagelib-9.2.beta0] New SAGE_ROOT=
[sagelib-9.2.beta0] ************************************************************************
[sagelib-9.2.beta0] Traceback (most recent call last):
[sagelib-9.2.beta0]   File "setup.py", line 72, in <module>
[sagelib-9.2.beta0]     from module_list import ext_modules, library_order
[sagelib-9.2.beta0]   File "/Users/buildbot-sage/slave/sage_git/build/src/module_list.py", line 80, in <module>
[sagelib-9.2.beta0]     aliases = cython_aliases()
[sagelib-9.2.beta0]   File "/Users/buildbot-sage/slave/sage_git/build/src/sage/env.py", line 395, in cython_aliases
[sagelib-9.2.beta0]     aliases[var + "CFLAGS"] = pkgconfig.cflags(lib).split()
[sagelib-9.2.beta0]   File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py", line 144, in cflags
[sagelib-9.2.beta0]     _raise_if_not_exists(package)
[sagelib-9.2.beta0]   File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py", line 103, in _raise_if_not_exists
[sagelib-9.2.beta0]     raise PackageNotFoundError(package)
[sagelib-9.2.beta0] pkgconfig.pkgconfig.PackageNotFoundError: zlib not found
[sagelib-9.2.beta0] ************************************************************************
[sagelib-9.2.beta0] Error building the Sage library
[sagelib-9.2.beta0] ************************************************************************
[sagelib-9.2.beta0] Please email sage-devel (http://groups.google.com/group/sage-devel)
[sagelib-9.2.beta0] explaining the problem and including the relevant part of the log file
[sagelib-9.2.beta0]   /Users/buildbot-sage/slave/sage_git/build/logs/pkgs/sagelib-9.2.beta0.log
[sagelib-9.2.beta0] Describe your computer, operating system, etc.
[sagelib-9.2.beta0] ************************************************************************
mkoeppe commented 4 years ago

Changed branch from c536daa to u/mkoeppe/c536daa32dcc69adbc7b37d9a170e22fd733bca8

kliem commented 4 years ago

Commit: b1b3787

kliem commented 4 years ago
comment:42

That fix looks plausible to me.


Last 10 new commits:

17ca1d7src/sage/env.py (cython_aliases): Add aliases for libraries use in module_list.py
346bd88src/sage/modules: Move Extension options from src/module_list.py to distutils directives
70901easrc/sage/env.py (cython_aliases): Update doctest
eed920esrc/sage/tests: Move Extension options from src/module_list.py to distutils directives
0d25d1dsrc/sage/structure: Move Extension options from src/module_list.py to distutils directives
808f46asrc/sage/stats: Move Extension options from src/module_list.py to distutils directives
10f7542src/sage/schemes: Move Extension options from src/module_list.py to distutils directives
5867c05src/sage/modular/pollack_stevens/dist.pyx: Add missing distutils directives
c536daaRemove self-listing in distutils sources directive
b1b3787sage.env.cython_aliases: Fix for systems without zlib pc
mkoeppe commented 4 years ago
comment:45

Thanks!