Opened 2 years ago
Closed 23 months ago
#29706 closed enhancement (fixed)
Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1  some packages without OptionalExtensions)
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  refactoring  Keywords:  sd109 
Cc:  fbissey, dimpase, jhpalmieri, tscrim, ghkliem, roed, vdelecroix, vbraun  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Jonathan Kliem, John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  1bfe30d (Commits, GitHub, GitLab)  Commit:  1bfe30dffe9128f58c9110c2ebb594e52a7c40f5 
Dependencies:  Stopgaps: 
Description (last modified by )
This simplifies src/module_list.py
a lot. This is preparation for splitting sagelib into separate distutils packages.
Followup tickets take care of the remaining packages; in the end, in #29701, src/module_list.py
will no longer be used, and in another followup ticket, it will disappear completely:
 #29720: Move
Extension
options ... (part 2 OptionalExtension
s)  #29785: Move
Extension
options ... (part 3: Get rid ofuname_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
Change History (57)
comment:1 Changed 2 years ago by
 Cc roed vdelecroix vbraun added
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 Changed 2 years ago by
 Branch set to u/mkoeppe/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files
comment:4 Changed 2 years ago by
 Commit set to f31b6a09b38cd9b6e3b9070ab5e20cb605866812
comment:5 Changed 2 years ago by
 Commit changed from f31b6a09b38cd9b6e3b9070ab5e20cb605866812 to 346bd8887cc45d189a17174107291c72e8b9ad6a
Branch pushed to git repo; I updated commit sha1. New commits:
0c0ef43  src/sage/libs/ntl: Move Extension options from src/module_list.py to distutils directives

a4d04d3  src/sage/modular: Move Extension options from src/module_list.py to distutils directives

17ca1d7  src/sage/env.py (cython_aliases): Add aliases for libraries use in module_list.py

346bd88  src/sage/modules: Move Extension options from src/module_list.py to distutils directives

comment:6 Changed 2 years ago by
 Description modified (diff)
 Status changed from new to needs_review
 Summary changed from Move Extension options from src/module_list.py to "distutils:" directives in the individual files to Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1)
comment:7 Changed 2 years ago by
 Description modified (diff)
comment:8 Changed 2 years ago by
 Commit changed from 346bd8887cc45d189a17174107291c72e8b9ad6a to 70901ea558546af579693dc17775774922ec7e93
Branch pushed to git repo; I updated commit sha1. New commits:
70901ea  src/sage/env.py (cython_aliases): Update doctest

comment:9 Changed 2 years ago by
 Description modified (diff)
comment:10 Changed 2 years ago by
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)?
comment:11 Changed 2 years ago by
I would guess so but would prefer to do that on a different ticket
comment:12 Changed 2 years ago by
 Commit changed from 70901ea558546af579693dc17775774922ec7e93 to 10f754224ab62457353dc4385468ef47af413db5
Branch pushed to git repo; I updated commit sha1. New commits:
eed920e  src/sage/tests: Move Extension options from src/module_list.py to distutils directives

0d25d1d  src/sage/structure: Move Extension options from src/module_list.py to distutils directives

808f46a  src/sage/stats: Move Extension options from src/module_list.py to distutils directives

10f7542  src/sage/schemes: Move Extension options from src/module_list.py to distutils directives

comment:13 Changed 2 years ago by
 Summary changed from Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1) to Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1  packages without OptionalExtensions)
comment:14 followup: ↓ 16 Changed 2 years ago by
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"]),
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.
comment:15 Changed 2 years ago by
 Keywords sd109 added
comment:16 in reply to: ↑ 14 ; followup: ↓ 25 Changed 2 years ago by
Replying to ghkliem:
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.ccSame 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.
comment:17 Changed 2 years ago by
Ok. I'm testing it now both locally and with github actions.
comment:18 followup: ↓ 26 Changed 2 years ago by
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/sagetest27122/runs/722743115
[dochtml] File "/sage/local/lib/python3.7/sitepackages/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/sitepackages/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_64linuxgnu/libstdc++.so.6: version `GLIBCXX_3.4.26' not found (required by /sage/local/lib/python3.7/sitepackages/sage/matrix/matrix_integer_sparse.cpython
How come numerical
, rings
etc. wheren't touched?
comment:19 Changed 2 years ago by
I also see this kind of strange error on GH Actions on buster. Perhaps the buster image used is faulty?
comment:20 Changed 2 years ago by
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/sagetest27122/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.
comment:21 Changed 2 years ago by
In comparison to almost everything succeeding at 9.1 release.
comment:22 Changed 2 years ago by
 Description modified (diff)
comment:23 Changed 2 years ago by
 Commit changed from 10f754224ab62457353dc4385468ef47af413db5 to 5867c054f9605ecb740120ef9e22d5e3c9b6a5e2
Branch pushed to git repo; I updated commit sha1. New commits:
5867c05  src/sage/modular/pollack_stevens/dist.pyx: Add missing distutils directives

comment:24 Changed 2 years ago by
 Commit changed from 5867c054f9605ecb740120ef9e22d5e3c9b6a5e2 to c536daa32dcc69adbc7b37d9a170e22fd733bca8
Branch pushed to git repo; I updated commit sha1. New commits:
c536daa  Remove selflisting in distutils sources directive

comment:25 in reply to: ↑ 16 Changed 2 years ago by
Replying to mkoeppe:
Replying to ghkliem:
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.ccSame 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.
comment:26 in reply to: ↑ 18 Changed 2 years ago by
Replying to ghkliem:
How come
numerical
,rings
etc. weren't touched?
The present ticket only touches toplevel packages that do not have OptionalExtension
s.
Part 2 will do OptionalExtension
s. Part 3 all remaining (mixed) ones. I split it up to reduce the potential for merge conflicts.
comment:27 Changed 2 years ago by
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.
comment:28 Changed 2 years ago by
It's quite possible that I misremembered and it's just an arbitrary subset.
comment:29 Changed 2 years ago by
 Summary changed from Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1  packages without OptionalExtensions) to Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1  some packages without OptionalExtensions)
comment:30 Changed 2 years ago by
 Reviewers set to Jonathan Kliem
 Status changed from needs_review to positive_review
Works for me.
As it apparently also works on your mac and it passes on cygwin https://github.com/kliem/sagetest27122/actions/runs/119764261 and it works fine on multiple linux instances, the build process seems to work just the same as before.
comment:31 Changed 2 years ago by
Thanks!
comment:32 Changed 2 years ago by
 Description modified (diff)
comment:33 Changed 2 years ago by
 Description modified (diff)
comment:34 Changed 2 years ago by
 Description modified (diff)
comment:35 Changed 2 years ago by
 Description modified (diff)
comment:36 Changed 2 years ago by
 Description modified (diff)
comment:37 Changed 2 years ago by
 Description modified (diff)
comment:38 Changed 2 years ago by
 Description modified (diff)
comment:39 Changed 2 years ago by
 Branch changed from u/mkoeppe/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files to c536daa32dcc69adbc7b37d9a170e22fd733bca8
 Resolution set to fixed
 Status changed from positive_review to closed
comment:40 Changed 2 years ago by
 Commit c536daa32dcc69adbc7b37d9a170e22fd733bca8 deleted
 Resolution fixed deleted
 Status changed from closed to new
on OSX:
[sagelib9.2.beta0] cd . && export \ [sagelib9.2.beta0] SAGE_ROOT=/doesnotexist \ [sagelib9.2.beta0] SAGE_SRC=/doesnotexist \ [sagelib9.2.beta0] SAGE_SRC_ROOT=/doesnotexist \ [sagelib9.2.beta0] SAGE_DOC_SRC=/doesnotexist \ [sagelib9.2.beta0] SAGE_BUILD_DIR=/doesnotexist \ [sagelib9.2.beta0] SAGE_PKGS=/Users/buildbotsage/slave/sage_git/build/build/pkgs \ [sagelib9.2.beta0] && sagepython u setup.py nousercfg build install [sagelib9.2.beta0] /Users/buildbotsage/slave/sage_git/build/src/bin/sageenv: line 130: cd: /doesnotexist: No such file or directory [sagelib9.2.beta0] Warning: overwriting SAGE_ROOT environment variable: [sagelib9.2.beta0] Old SAGE_ROOT=/doesnotexist [sagelib9.2.beta0] New SAGE_ROOT= [sagelib9.2.beta0] ************************************************************************ [sagelib9.2.beta0] Traceback (most recent call last): [sagelib9.2.beta0] File "setup.py", line 72, in <module> [sagelib9.2.beta0] from module_list import ext_modules, library_order [sagelib9.2.beta0] File "/Users/buildbotsage/slave/sage_git/build/src/module_list.py", line 80, in <module> [sagelib9.2.beta0] aliases = cython_aliases() [sagelib9.2.beta0] File "/Users/buildbotsage/slave/sage_git/build/src/sage/env.py", line 395, in cython_aliases [sagelib9.2.beta0] aliases[var + "CFLAGS"] = pkgconfig.cflags(lib).split() [sagelib9.2.beta0] File "/Users/buildbotsage/slave/sage_git/build/local/lib/python3.7/sitepackages/pkgconfig/pkgconfig.py", line 144, in cflags [sagelib9.2.beta0] _raise_if_not_exists(package) [sagelib9.2.beta0] File "/Users/buildbotsage/slave/sage_git/build/local/lib/python3.7/sitepackages/pkgconfig/pkgconfig.py", line 103, in _raise_if_not_exists [sagelib9.2.beta0] raise PackageNotFoundError(package) [sagelib9.2.beta0] pkgconfig.pkgconfig.PackageNotFoundError: zlib not found [sagelib9.2.beta0] ************************************************************************ [sagelib9.2.beta0] Error building the Sage library [sagelib9.2.beta0] ************************************************************************ [sagelib9.2.beta0] Please email sagedevel (http://groups.google.com/group/sagedevel) [sagelib9.2.beta0] explaining the problem and including the relevant part of the log file [sagelib9.2.beta0] /Users/buildbotsage/slave/sage_git/build/logs/pkgs/sagelib9.2.beta0.log [sagelib9.2.beta0] Describe your computer, operating system, etc. [sagelib9.2.beta0] ************************************************************************
comment:41 Changed 2 years ago by
 Branch changed from c536daa32dcc69adbc7b37d9a170e22fd733bca8 to u/mkoeppe/c536daa32dcc69adbc7b37d9a170e22fd733bca8
comment:42 Changed 2 years ago by
 Commit set to b1b378712d10cb15555889e8235d089f3cf0e391
That fix looks plausible to me.
Last 10 new commits:
17ca1d7  src/sage/env.py (cython_aliases): Add aliases for libraries use in module_list.py

346bd88  src/sage/modules: Move Extension options from src/module_list.py to distutils directives

70901ea  src/sage/env.py (cython_aliases): Update doctest

eed920e  src/sage/tests: Move Extension options from src/module_list.py to distutils directives

0d25d1d  src/sage/structure: Move Extension options from src/module_list.py to distutils directives

808f46a  src/sage/stats: Move Extension options from src/module_list.py to distutils directives

10f7542  src/sage/schemes: Move Extension options from src/module_list.py to distutils directives

5867c05  src/sage/modular/pollack_stevens/dist.pyx: Add missing distutils directives

c536daa  Remove selflisting in distutils sources directive

b1b3787  sage.env.cython_aliases: Fix for systems without zlib pc

comment:43 Changed 2 years ago by
 Status changed from new to needs_review
comment:44 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:45 Changed 2 years ago by
Thanks!
comment:46 Changed 2 years ago by
 Status changed from positive_review to needs_work
Still getting on OSX:
[sagelib9.2.beta1] ************************************************************************ [sagelib9.2.beta1] Traceback (most recent call last): [sagelib9.2.beta1] File "setup.py", line 72, in <module> [sagelib9.2.beta1] from module_list import ext_modules, library_order [sagelib9.2.beta1] File "/Users/buildbotsage/slave/sage_git/build/src/module_list.py", line 80, in <module> [sagelib9.2.beta1] aliases = cython_aliases() [sagelib9.2.beta1] File "/Users/buildbotsage/slave/sage_git/build/src/sage/env.py", line 409, in cython_aliases [sagelib9.2.beta1] aliases[var + "LIBEXTRA"] = list(filter(lambda s: not s.startswith(('l','L')), pkgconfig.libs(lib).split())) [sagelib9.2.beta1] File "/Users/buildbotsage/slave/sage_git/build/local/lib/python3.7/sitepackages/pkgconfig/pkgconfig.py", line 165, in libs [sagelib9.2.beta1] _raise_if_not_exists(package) [sagelib9.2.beta1] File "/Users/buildbotsage/slave/sage_git/build/local/lib/python3.7/sitepackages/pkgconfig/pkgconfig.py", line 103, in _raise_if_not_exists [sagelib9.2.beta1] raise PackageNotFoundError(package) [sagelib9.2.beta1] pkgconfig.pkgconfig.PackageNotFoundError: zlib not found [sagelib9.2.beta1] ************************************************************************ [sagelib9.2.beta1] Error building the Sage library
comment:47 Changed 2 years ago by
 Commit changed from b1b378712d10cb15555889e8235d089f3cf0e391 to 1bfe30dffe9128f58c9110c2ebb594e52a7c40f5
Branch pushed to git repo; I updated commit sha1. New commits:
1bfe30d  sage.env.cython_aliases: Another fix for macOS without zlib pc

comment:48 Changed 2 years ago by
 Status changed from needs_work to needs_review
Sorry, here's another fix.
comment:49 Changed 2 years ago by
Any suggestions for how to reproduce the error? I've tried using the previous version of the branch, and either building with lots of system packages or with very few, but I haven't hit this build error.
comment:50 Changed 2 years ago by
Basically, one needs a macOS without homebrew (which installs .pc files for system libraries). Or do the following to simulate that:
(cd /usr/local/Homebrew/Library/Homebrew/os/mac/ && mv pkgconfig pkgconfigmoved)
comment:51 Changed 2 years ago by
I created #29929 (tox.ini: Add a macos environment without homebrew, conda
) so we can test such a barebones macOS environment on GH Actions
comment:52 followup: ↓ 53 Changed 2 years ago by
Have you tried the current branch with such a system? I am completely clueless, whether the last commit resolves the problem.
comment:53 in reply to: ↑ 52 Changed 2 years ago by
Replying to ghkliem:
Have you tried the current branch with such a system? I am completely clueless, whether the last commit resolves the problem.
Yes, a build from scratch with the manipulation mentioned in comment 50 just finished successfully on my machine.
comment:54 Changed 2 years ago by
I have not yet been able to reproduce the error, so I can't tell if the most recent change fixes anything. Renaming /usr/local/Homebrew/Library/Homebrew/os/mac/pkgconfig
didn't cause breakage, nor did renaming /usr/local/lib/pkgconfig
. I'm trying on a different machine by just removing homebrew altogether.
comment:55 Changed 2 years ago by
 Status changed from needs_review to positive_review
I can confirm on a machine without Homebrew, I hit the failure that Volker mentioned, and the most recent change fixes it. I'm going to switch to a positive review.
comment:56 Changed 2 years ago by
 Reviewers changed from Jonathan Kliem to Jonathan Kliem, John Palmieri
Thank you!
comment:57 Changed 23 months ago by
 Branch changed from u/mkoeppe/c536daa32dcc69adbc7b37d9a170e22fd733bca8 to 1bfe30dffe9128f58c9110c2ebb594e52a7c40f5
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
src/sage/geometry: Move Extension options from src/module_list.py to distutils directives