Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mkoeppe opened this issue May 18, 2020 · 64 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented May 18, 2020

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 mkoeppe added this to the sage-9.2 milestone May 18, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented May 18, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2020

Commit: f31b6a0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2020

Changed commit from f31b6a0 to 346bd88

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2020

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
Copy link
Member Author

mkoeppe commented May 18, 2020

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Move Extension options from src/module_list.py to "distutils:" directives in the individual files Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1) May 18, 2020
@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2020

Changed commit from 346bd88 to 70901ea

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2020

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

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

@mkoeppe

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented May 18, 2020

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
Copy link
Member Author

mkoeppe commented May 18, 2020

comment:11

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2020

Changed commit from 70901ea to 10f7542

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2020

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

@mkoeppe mkoeppe changed the title Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1) Move Extension options from src/module_list.py to "distutils:" directives in the individual files (part 1 - packages without OptionalExtensions) May 21, 2020
@kliem
Copy link
Contributor

kliem commented May 27, 2020

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

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

@kliem
Copy link
Contributor

kliem commented May 27, 2020

Changed keywords from none to sd109

@mkoeppe
Copy link
Member Author

mkoeppe commented May 29, 2020

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
Copy link
Contributor

kliem commented May 30, 2020

comment:17

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

@kliem
Copy link
Contributor

kliem commented May 30, 2020

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
Copy link
Member

dimpase commented May 30, 2020

comment:19

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

@kliem
Copy link
Contributor

kliem commented May 31, 2020

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2020

Changed commit from b1b3787 to 1bfe30d

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 21, 2020

comment:48

Sorry, here's another fix.

@jhpalmieri
Copy link
Member

comment:49

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 22, 2020

comment:50

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)

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 22, 2020

comment:51

I created #29929 (tox.ini: Add a macos environment without homebrew, conda) so we can test such a barebones macOS environment on GH Actions

@kliem
Copy link
Contributor

kliem commented Jun 22, 2020

comment:52

Have you tried the current branch with such a system? I am completely clueless, whether the last commit resolves the problem.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 22, 2020

comment:53

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

@jhpalmieri
Copy link
Member

comment:54

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.

@jhpalmieri
Copy link
Member

comment:55

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 22, 2020

Changed reviewer from Jonathan Kliem to Jonathan Kliem, John Palmieri

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 22, 2020

comment:56

Thank you!

@vbraun
Copy link
Member

vbraun commented Jun 24, 2020

Changed branch from u/mkoeppe/c536daa32dcc69adbc7b37d9a170e22fd733bca8 to 1bfe30d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants