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: | sage-9.2 |
Component: | refactoring | Keywords: | sd109 |
Cc: | fbissey, dimpase, jhpalmieri, tscrim, gh-kliem, 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.
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 ... (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 follow-up: ↓ 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 ; follow-up: ↓ 25 Changed 2 years ago by
Replying to gh-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.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 follow-up: ↓ 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/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?
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/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.
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 self-listing in distutils sources directive
|
comment:25 in reply to: ↑ 16 Changed 2 years ago by
Replying to mkoeppe:
Replying to gh-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.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 gh-kliem:
How come
numerical
,rings
etc. weren't touched?
The present ticket only touches top-level 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/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.
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:
[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] ************************************************************************
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 self-listing 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:
[sagelib-9.2.beta1] ************************************************************************ [sagelib-9.2.beta1] Traceback (most recent call last): [sagelib-9.2.beta1] File "setup.py", line 72, in <module> [sagelib-9.2.beta1] from module_list import ext_modules, library_order [sagelib-9.2.beta1] File "/Users/buildbot-sage/slave/sage_git/build/src/module_list.py", line 80, in <module> [sagelib-9.2.beta1] aliases = cython_aliases() [sagelib-9.2.beta1] File "/Users/buildbot-sage/slave/sage_git/build/src/sage/env.py", line 409, in cython_aliases [sagelib-9.2.beta1] aliases[var + "LIBEXTRA"] = list(filter(lambda s: not s.startswith(('-l','-L')), pkgconfig.libs(lib).split())) [sagelib-9.2.beta1] File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py", line 165, in libs [sagelib-9.2.beta1] _raise_if_not_exists(package) [sagelib-9.2.beta1] 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.beta1] raise PackageNotFoundError(package) [sagelib-9.2.beta1] pkgconfig.pkgconfig.PackageNotFoundError: zlib not found [sagelib-9.2.beta1] ************************************************************************ [sagelib-9.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 pkgconfig-moved)
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 follow-up: ↓ 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 gh-kliem:
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