#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:

Status badges

Description (last modified by mkoeppe)

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 - OptionalExtensions)
  • #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

Change History (57)

comment:1 Changed 18 months ago by mkoeppe

  • Cc roed vdelecroix vbraun added

comment:2 Changed 18 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 18 months ago by mkoeppe

  • Branch set to u/mkoeppe/move_extension_options_from_src_module_list_py_to__distutils___directives_in_the_individual_files

comment:4 Changed 18 months ago by git

  • Commit set to f31b6a09b38cd9b6e3b9070ab5e20cb605866812

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

comment:5 Changed 18 months ago by git

  • Commit changed from f31b6a09b38cd9b6e3b9070ab5e20cb605866812 to 346bd8887cc45d189a17174107291c72e8b9ad6a

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

comment:6 Changed 18 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • 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 18 months ago by mkoeppe

  • Description modified (diff)

comment:8 Changed 18 months ago by git

  • Commit changed from 346bd8887cc45d189a17174107291c72e8b9ad6a to 70901ea558546af579693dc17775774922ec7e93

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

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

comment:9 Changed 18 months ago by mkoeppe

  • Description modified (diff)

comment:10 Changed 18 months ago by dimpase

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 18 months ago by mkoeppe

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

comment:12 Changed 17 months ago by git

  • Commit changed from 70901ea558546af579693dc17775774922ec7e93 to 10f754224ab62457353dc4385468ef47af413db5

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

comment:13 Changed 17 months ago by mkoeppe

  • 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: Changed 17 months ago by 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.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.

Last edited 17 months ago by gh-kliem (previous) (diff)

comment:15 Changed 17 months ago by gh-kliem

  • Keywords sd109 added

comment:16 in reply to: ↑ 14 ; follow-up: Changed 17 months ago by 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.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.

comment:17 Changed 17 months ago by gh-kliem

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

comment:18 follow-up: Changed 17 months ago by gh-kliem

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 17 months ago by dimpase

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

comment:20 Changed 17 months ago by gh-kliem

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 17 months ago by gh-kliem

In comparison to almost everything succeeding at 9.1 release.

comment:22 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:23 Changed 17 months ago by git

  • Commit changed from 10f754224ab62457353dc4385468ef47af413db5 to 5867c054f9605ecb740120ef9e22d5e3c9b6a5e2

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

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

comment:24 Changed 17 months ago by git

  • Commit changed from 5867c054f9605ecb740120ef9e22d5e3c9b6a5e2 to c536daa32dcc69adbc7b37d9a170e22fd733bca8

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

c536daaRemove self-listing in distutils sources directive

comment:25 in reply to: ↑ 16 Changed 17 months ago by mkoeppe

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.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.

comment:26 in reply to: ↑ 18 Changed 17 months ago by mkoeppe

Replying to gh-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.

comment:27 Changed 17 months ago by gh-kliem

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 17 months ago by mkoeppe

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

comment:29 Changed 17 months ago by mkoeppe

  • 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 17 months ago by gh-kliem

  • 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 17 months ago by mkoeppe

Thanks!

comment:32 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:33 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:34 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:35 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:36 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:37 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:38 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:39 Changed 17 months ago by vbraun

  • 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 17 months ago by vbraun

  • 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 17 months ago by mkoeppe

  • Branch changed from c536daa32dcc69adbc7b37d9a170e22fd733bca8 to u/mkoeppe/c536daa32dcc69adbc7b37d9a170e22fd733bca8

comment:42 Changed 17 months ago by gh-kliem

  • Commit set to b1b378712d10cb15555889e8235d089f3cf0e391

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

comment:43 Changed 17 months ago by mkoeppe

  • Status changed from new to needs_review

comment:44 Changed 17 months ago by gh-kliem

  • Status changed from needs_review to positive_review

comment:45 Changed 17 months ago by mkoeppe

Thanks!

comment:46 Changed 16 months ago by vbraun

  • 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 16 months ago by git

  • Commit changed from b1b378712d10cb15555889e8235d089f3cf0e391 to 1bfe30dffe9128f58c9110c2ebb594e52a7c40f5

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

1bfe30dsage.env.cython_aliases: Another fix for macOS without zlib pc

comment:48 Changed 16 months ago by mkoeppe

  • Status changed from needs_work to needs_review

Sorry, here's another fix.

comment:49 Changed 16 months ago by jhpalmieri

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 16 months ago by mkoeppe

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 16 months ago by mkoeppe

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: Changed 16 months ago by gh-kliem

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 16 months ago by mkoeppe

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 16 months ago by jhpalmieri

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 16 months ago by jhpalmieri

  • 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 16 months ago by mkoeppe

  • Reviewers changed from Jonathan Kliem to Jonathan Kliem, John Palmieri

Thank you!

comment:57 Changed 16 months ago by vbraun

  • Branch changed from u/mkoeppe/c536daa32dcc69adbc7b37d9a170e22fd733bca8 to 1bfe30dffe9128f58c9110c2ebb594e52a7c40f5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.