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: sage-8.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:

Status badges

Description (last modified by jdemeyer)

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 jdemeyer

Cc: embray jhpalmieri mkoeppe added
Description: modified (diff)
Status: newneeds_review

comment:2 Changed 5 years ago by jdemeyer

Branch: u/jdemeyer/ticket/23799

comment:3 Changed 5 years ago by git

Commit: 7a04c1cf037f6a1c398db2ba93b193f5c4313b33

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

7a04c1cImport pkgconfig only when needed

comment:4 Changed 5 years ago by git

Commit: 7a04c1cf037f6a1c398db2ba93b193f5c4313b33f06d3b365a6927e4fd0b4d0d8bf67299ee13e2d1

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

f06d3b3Merge tag '8.1.beta5' into t/23799/ticket/23799

comment:5 Changed 5 years ago by jdemeyer

Cc: vdelecroix added

comment:6 Changed 5 years ago by vdelecroix

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:7 Changed 5 years ago by jdemeyer

You mean messing with globals()? I don't like that.

comment:8 Changed 5 years ago by vdelecroix

Replying to jdemeyer:

You mean messing with globals()? I don't like that.

I meant

packages = ['fflas-ffpack', '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 git

Commit: f06d3b365a6927e4fd0b4d0d8bf67299ee13e2d107ff90860b6e318513a7fe21ac7d07cce30e0198

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

07ff908Generate aliases automatically

comment:10 Changed 5 years ago by jdemeyer

Good suggestion, done.

comment:11 Changed 5 years ago by 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, 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 in reply to:  11 Changed 5 years ago by jdemeyer

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, fflas-ffpack or givaro.

comment:13 Changed 5 years ago by fbissey

In sage-on-gentoo which is compiled with --as-needed I get

fbissey@moonloop ~ $ readelf -d /usr/lib64/python2.7/site-packages/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 jdemeyer

Sorry, I was wrong indeed. I missed the modn part, I thought that this was about floating-point matrices.

comment:15 in reply to:  11 Changed 5 years ago by jdemeyer

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, fflas-ffpack 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 Changed 5 years ago by fbissey

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 fflas-ffpack wasn't split yet from linbox.

comment:17 in reply to:  16 Changed 5 years ago by jdemeyer

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 Changed 5 years ago by 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.

comment:19 in reply to:  18 Changed 5 years ago by fbissey

Reviewers: François Bissey
Status: needs_reviewpositive_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 vbraun

Branch: u/jdemeyer/ticket/2379907ff90860b6e318513a7fe21ac7d07cce30e0198
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.