Opened 5 years ago
Closed 5 years ago
#23799 closed enhancement (fixed)
Move module_list aliases to env.py
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  cython  Keywords:  
Cc:  embray, jhpalmieri, mkoeppe, vdelecroix  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  François Bissey 
Report Upstream:  N/A  Work issues:  
Branch:  07ff908 (Commits, GitHub, GitLab)  Commit:  07ff90860b6e318513a7fe21ac7d07cce30e0198 
Dependencies:  Stopgaps: 
Description (last modified by )
Move the aliases from src/module_list.py
to src/sage/env.py
such that they can be used at runtime by cython()
too. This is needed for #22461.
Change History (20)
comment:1 Changed 5 years ago by
Cc:  embray jhpalmieri mkoeppe added 

Description:  modified (diff) 
Status:  new → needs_review 
comment:2 Changed 5 years ago by
Branch:  → u/jdemeyer/ticket/23799 

comment:3 Changed 5 years ago by
Commit:  → 7a04c1cf037f6a1c398db2ba93b193f5c4313b33 

comment:4 Changed 5 years ago by
Commit:  7a04c1cf037f6a1c398db2ba93b193f5c4313b33 → f06d3b365a6927e4fd0b4d0d8bf67299ee13e2d1 

Branch pushed to git repo; I updated commit sha1. New commits:
f06d3b3  Merge tag '8.1.beta5' into t/23799/ticket/23799

comment:5 Changed 5 years ago by
Cc:  vdelecroix added 

comment:6 Changed 5 years ago by
Given that the procedure is the same each time
X_pc = pkgconfig.parse('X') X_libs = X_pc['libraries'] X_library_dirs = X_pc['library_dirs'] X_cflags = pkgconfig.cflags('X').split()
could that be factored? (slight lie: no cflags involved for gsl, no include dirs for linbox, ... why?)
comment:8 Changed 5 years ago by
Replying to jdemeyer:
You mean messing with
globals()
? I don't like that.
I meant
packages = ['fflasffpack', 'givaro', 'linbox', 'gsl', 'singular'] ans = {} for pkg in packages: pc = pkgconfig.parse(pkg) name = pkg.replace('', '').upper() ans['%s_LIBRARIES' %name] = pc['libraries'] ans['%s_LIBDIR' %name] = pc['library_dirs'] ans['%s_INCDIR' %name] = pc['include_dirs'] ans['%s_CFLAGS' %name] = pkgconfig.cflags(pkg).split() return ans
comment:9 Changed 5 years ago by
Commit:  f06d3b365a6927e4fd0b4d0d8bf67299ee13e2d1 → 07ff90860b6e318513a7fe21ac7d07cce30e0198 

Branch pushed to git repo; I updated commit sha1. New commits:
07ff908  Generate aliases automatically

comment:11 followups: 12 15 Changed 5 years ago by
I like the ticket but I have one more question before setting it to positive review
@@ 881,14 +829,14 @@ ext_modules = [ Extension('sage.matrix.matrix_modn_dense_float', sources = ['sage/matrix/matrix_modn_dense_float.pyx'], language="c++",  libraries = linbox_libs + cblas_libs, + libraries = cblas_libs, library_dirs = cblas_library_dirs, include_dirs = cblas_include_dirs), Extension('sage.matrix.matrix_modn_dense_double', sources = ['sage/matrix/matrix_modn_dense_double.pyx'], language="c++",  libraries = linbox_libs + cblas_libs, + libraries = cblas_libs, library_dirs = cblas_library_dirs, include_dirs = cblas_include_dirs, extra_compile_args = ["D_XPG6"]),
It is correct that those don't link to linbox, but the linbox flags provide linking flags to fflas, ffpack and givaro that are needed. Are those brought by a dependency that can escape a cursory look? I could not find any distutils instructions in those particular files to account for the dependencies on fflas, ffpack and givaro.
comment:12 Changed 5 years ago by
Replying to fbissey:
I like the ticket but I have one more question before setting it to positive review
@@ 881,14 +829,14 @@ ext_modules = [ Extension('sage.matrix.matrix_modn_dense_float', sources = ['sage/matrix/matrix_modn_dense_float.pyx'], language="c++",  libraries = linbox_libs + cblas_libs, + libraries = cblas_libs, library_dirs = cblas_library_dirs, include_dirs = cblas_include_dirs), Extension('sage.matrix.matrix_modn_dense_double', sources = ['sage/matrix/matrix_modn_dense_double.pyx'], language="c++",  libraries = linbox_libs + cblas_libs, + libraries = cblas_libs, library_dirs = cblas_library_dirs, include_dirs = cblas_include_dirs, extra_compile_args = ["D_XPG6"]),
As far as I know, those files do not actually depend on linbox, fflasffpack or givaro.
comment:13 Changed 5 years ago by
In sageongentoo which is compiled with asneeded
I get
fbissey@moonloop ~ $ readelf d /usr/lib64/python2.7/sitepackages/sage/matrix/matrix_modn_dense_float.so Dynamic section at offset 0x5ea98 contains 32 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libfflas.so.1] 0x0000000000000001 (NEEDED) Shared library: [libffpack.so.1] 0x0000000000000001 (NEEDED) Shared library: [libgivaro.so.9] 0x0000000000000001 (NEEDED) Shared library: [libgmp.so.10] 0x0000000000000001 (NEEDED) Shared library: [libstdc++.so.6] 0x0000000000000001 (NEEDED) Shared library: [libpython2.7.so.1.0] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libgcc_s.so.1] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
I wouldn't expect those to be present unless some symbols are directly needed. That warrant a closer look.
comment:14 Changed 5 years ago by
Sorry, I was wrong indeed. I missed the modn
part, I thought that this was about floatingpoint matrices.
comment:15 Changed 5 years ago by
Replying to fbissey:
I like the ticket but I have one more question before setting it to positive review
@@ 881,14 +829,14 @@ ext_modules = [ Extension('sage.matrix.matrix_modn_dense_float', sources = ['sage/matrix/matrix_modn_dense_float.pyx'], language="c++",  libraries = linbox_libs + cblas_libs, + libraries = cblas_libs, library_dirs = cblas_library_dirs, include_dirs = cblas_include_dirs), Extension('sage.matrix.matrix_modn_dense_double', sources = ['sage/matrix/matrix_modn_dense_double.pyx'], language="c++",  libraries = linbox_libs + cblas_libs, + libraries = cblas_libs, library_dirs = cblas_library_dirs, include_dirs = cblas_include_dirs, extra_compile_args = ["D_XPG6"]),It is correct that those don't link to linbox
They do actually link against linbox, but the linbox, fflasffpack and givaro flags are brought in by cimports of sage.libs.linbox....
at the beginning of src/sage/matrix/matrix_modn_dense_float.pyx
and src/sage/matrix/matrix_modn_dense_double.pyx
comment:16 followup: 17 Changed 5 years ago by
Yet the final product doesn't have any symbols from linbox, otherwise it would be in the list of libraries. I am guessing the content of sage.libs.linbox
still reflects the time when fflasffpack
wasn't split yet from linbox.
comment:17 Changed 5 years ago by
Replying to fbissey:
Yet the final product doesn't have any symbols from linbox, otherwise it would be in the list of libraries.
Which list of libraries do you mean?
At least when compiling those extensions, llinbox llinboxsage
is specified. This is true before and after this ticket.
comment:18 followup: 19 Changed 5 years ago by
If your comment is that llinbox
is specified while it is not needed: that is outside the scope of this ticket. There are probably many such cases.
comment:19 Changed 5 years ago by
Reviewers:  → François Bissey 

Status:  needs_review → positive_review 
Replying to jdemeyer:
If your comment is that
llinbox
is specified while it is not needed: that is outside the scope of this ticket. There are probably many such cases.
That is the case. But now I am more worried about the lack of lfflas lffpack lgivaro
but given the bot is green, I am guessing it gets in the same way as llinbox
. Let's move things along.
comment:20 Changed 5 years ago by
Branch:  u/jdemeyer/ticket/23799 → 07ff90860b6e318513a7fe21ac7d07cce30e0198 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Import pkgconfig only when needed