Further clean up of ATLAS (or equivalent BLAS) linking

[jpflori]

Different pkgs find ATLAS (or equivalent libs e.g. on OS X and Cygwin) libs in different ways. In particular the situation is broken on Cygwin.

The one we tweak ourselves are:

• ATLAS when we pass SAGE_ATLAS_LIB to decide what to copy/symlink
• fflas-ffpack
• iml
• R
• the Sage library through SAGE_BLAS and module_list.py and its BLAS/BLAS2 madness

The ones we hack ourselves:

• cvxopt (we patch and hardcode in setup.py).

Other pkgs depend on a BLAS implem but are autonomous(?):

• gsl,
• scipy (envvars are unset...),
• numpy (same same as scipy).

My proposal before a pkgconfig solution gets available at #17075 is that when ATLAS spkg-install script is run it decides what should be used once and for all and other packages use this information (like used to be done a little bit when the linbox spkg-install script wrote echo $LINBOX_BLAS > $SAGE_LOCAL/share/cblas_config)

[jpflori]

Info for scipy/numpy: ​http://www.scipy.org/scipylib/building/linux.html

[jpflori]

Fot cvxopt: ​http://cvxopt.org/install/

[fbissey]

Yes definitely of interest. I think cvxopt 1.1.7 is better at handling things with environment variables so we shouldn't patch anymore, I should update the cvxopt ebuild for gentoo. From cvxopt setup.py

BLAS_NOUNDERSCORES = int(os.environ.get("CVXOPT_BLAS_NOUNDERSCORES",BLAS_NOUNDERSCORES)) == True
BLAS_LIB = os.environ.get("CVXOPT_BLAS_LIB",BLAS_LIB)
LAPACK_LIB = os.environ.get("CVXOPT_LAPACK_LIB",LAPACK_LIB)
BLAS_LIB_DIR = os.environ.get("CVXOPT_BLAS_LIB_DIR",BLAS_LIB_DIR)
BLAS_EXTRA_LINK_ARGS = os.environ.get("CVXOPT_BLAS_EXTRA_LINK_ARGS",BLAS_EXTRA_LINK_ARGS)
if type(BLAS_LIB) is str: BLAS_LIB = BLAS_LIB.strip().split(',')
if type(LAPACK_LIB) is str: LAPACK_LIB = LAPACK_LIB.strip().split(',')
if type(BLAS_EXTRA_LINK_ARGS) is str: BLAS_EXTRA_LINK_ARGS = BLAS_EXTRA_LINK_ARGS.strip().split(',')
BUILD_GSL = int(os.environ.get("CVXOPT_BUILD_GSL",BUILD_GSL))
GSL_LIB_DIR = os.environ.get("CVXOPT_GSL_LIB_DIR",GSL_LIB_DIR)
GSL_INC_DIR = os.environ.get("CVXOPT_GSL_INC_DIR",GSL_INC_DIR)
BUILD_FFTW = int(os.environ.get("CVXOPT_BUILD_FFTW",BUILD_FFTW))
FFTW_LIB_DIR = os.environ.get("CVXOPT_FFTW_LIB_DIR",FFTW_LIB_DIR)
FFTW_INC_DIR = os.environ.get("CVXOPT_FFTW_INC_DIR",FFTW_INC_DIR)
BUILD_GLPK = int(os.environ.get("CVXOPT_BUILD_GLPK",BUILD_GLPK))
GLPK_LIB_DIR = os.environ.get("CVXOPT_GLPK_LIB_DIR",GLPK_LIB_DIR)
GLPK_INC_DIR = os.environ.get("CVXOPT_GLPK_INC_DIR",GLPK_INC_DIR)
BUILD_DSDP = int(os.environ.get("CVXOPT_BUILD_DSDP",BUILD_DSDP))
DSDP_LIB_DIR = os.environ.get("CVXOPT_DSDP_LIB_DIR",DSDP_LIB_DIR)
DSDP_INC_DIR = os.environ.get("CVXOPT_DSDP_INC_DIR",DSDP_INC_DIR)

gsl makes its own cblas (but no fortran blas) and by default libgsl is not linked with any cblas, you are supposed to link with a cblas when you use the library. Setting blas/lapack in numpy/scipy makes me crazy half the time. Environment variable or setup.cfg can be used to change the default (currently looking for MKL, ATLAS, netlib and netlib source in that order if I am not mistaken). Making a setup.cfg from pkg-config is hard

pc_incdir() {
	$(tc-getPKG_CONFIG) --cflags-only-I $@ | \
		sed -e 's/^-I//' -e 's/[ ]*-I/:/g' -e 's/[ ]*$//' -e 's|^:||'
}

pc_libdir() {
	$(tc-getPKG_CONFIG) --libs-only-L $@ | \
		sed -e 's/^-L//' -e 's/[ ]*-L/:/g' -e 's/[ ]*$//' -e 's|^:||'
}

pc_libs() {
	$(tc-getPKG_CONFIG) --libs-only-l $@ | \
		sed -e 's/[ ]-l*\(pthread\|m\)\([ ]\|$\)//g' \
		-e 's/^-l//' -e 's/[ ]*-l/,/g' -e 's/[ ]*$//' \
		| tr ',' '\n' | sort -u | tr '\n' ',' | sed -e 's|,$||'
}

		local libdir="${EPREFIX}"/usr/$(get_libdir) # local tree
		# make sure _dotblas.so gets built
		sed -i -e '/NO_ATLAS_INFO/,+1d' numpy/core/setup.py || die
		cat >> site.cfg <<-EOF
			[blas]
			include_dirs = $(pc_incdir cblas)
			library_dirs = $(pc_libdir cblas blas):${libdir}
			blas_libs = $(pc_libs cblas blas)
			[lapack]
			library_dirs = $(pc_libdir lapack):${libdir}
			lapack_libs = $(pc_libs lapack)
		EOF

but with the python binding it should be easier.

[jpflori]

This is definitely not really needs_reviex, in particular, OS X surely needs further tweaking, not sure that passing -lblas or lcblas would always work, or if one sometines needs to pass the -framework stuff... I've basically tested it on Linux and Cygwin and it looks ok.

Anyway, I won't be able to do anything for at leasr one month, so here is the current state of affair. Feel free to finalize and merge it!

---

Last 10 new commits:

​79c712c	Simplify BLAS/LAPACK config for IML.
​030b90b	Simplify BLAS/LAPACK config for numpy.
​2a14692	Simplify BLAS/LAPACK config for Sage library.
​355fff8	Let ATLAS install script write cblas_config.
​14675a5	Remove SAGE_CBLAS documentation.
​cf79fc3	First try to simplify BLAS/LAPACK configuration for cvxopt.
​534ec94	Fix cblas_config mechanism.
​09e8128	Further modifications to numpy/scipy build systems.
​37abe37	Slight cleanup of cvxopt directory.
​b02e159	Fixes for cleaner BLAS config.

[jpflori]

Also note that part of the changes here have been shamelessly stolen from sage-on-gentoo patches posted on #17075.

[fbissey]

Replying to jpflori:

Also note that part of the changes here have been shamelessly stolen from sage-on-gentoo patches posted on #17075.

You didn't go all the way with libdir, but you certainly stole the spirit of it. The libdir part is important if we use an external blas that is not in a standard directory. Hum.... may be I should add a runpath for that case, at the very least LD_LIBRARY_PATH or its equivalent needs to be set.

[jpflori]

Please note that the idea here is just to clean up how we use our default (and unique) BLAS. We should not try to do everything at once!

[fbissey]

Well, I'd still like to include libdir as forward thinking - that and it makes my life easier sage-on-gentoo side - of course lots of things land in sage that make my life ugly but I'd like to minimize it if I can.

[jpflori]

Any chance to get this merged and go on working in a follow up ticket?

[fbissey]

Missed that. Probably to late for 6.6 but we should be able to rebase for early inclusion in 6.7.

[git]

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

​091e941

Merge remote-tracking branch 'trac/develop' into ticket/17630

[fbissey]

Here is what is happening when you try to use Accelerate in R on OS X

gcc -std=gnu99 -dynamiclib -Wl,-headerpad_max_install_names  -undefined dynamic_lookup -single_module -multiply_defined suppress -L../../../lib -L/Users/fbissey/build/sage-6.5.beta1/local/lib/  -o internet.so Rhttpd.o Rsock.o internet.o nanoftp.o nanohttp.o sock.o sockconn.o -lR   -Wl,-framework -Wl,CoreFoundation
mkdir /Users/fbissey/build/sage-6.5.beta1/local/var/tmp/sage/build/r-3.1.2.p0/src/modules
making Lapack.d from Lapack.c
making vecLibg95c.d from vecLibg95c.c
<built-in>: warning: subframework include vecLib/vecLib.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vecLibTypes.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vBasicOps.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vBigNum.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vectorOps.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vfp.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vDSP.h conflicts with framework include
<built-in>: warning: subframework include vecLib/cblas.h conflicts with framework include
<built-in>: warning: subframework include vecLib/clapack.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/LinearAlgebra.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/base.h conflicts with framework include
In file included from /usr/include/os/object.h:27:0,
                 from /System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/LinearAlgebra/base.h:6,
                 from /System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/LinearAlgebra/LinearAlgebra.h:10,
                 from /System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/vecLib.h:65,
                 from /System/Library/Frameworks/Accelerate.framework/Headers/Accelerate.h:20,
                 from vecLibg95c.c:8:
/usr/include/os/base.h:113:20: error: missing binary operator before token "("
 #if __has_extension(attribute_overloadable)
                    ^
/usr/include/os/base.h:119:54: error: missing binary operator before token "("
 #if __has_feature(objc_fixed_enum) || __has_extension(cxx_strong_enums)
                                                      ^
<built-in>: warning: subframework include vecLib/LinearAlgebra/object.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/matrix.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/vector.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/splat.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/arithmetic.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/linear_systems.h conflicts with framework include
<built-in>: warning: subframework include vecLib/LinearAlgebra/norms.h conflicts with framework include
<built-in>: warning: subframework include vecLib/vForce.h conflicts with framework include
make[3]: *** [vecLibg95c.d] Error 1
make[2]: *** [make.lapack] Error 2
make[1]: *** [R] Error 1
make: *** [R] Error 1
Error building R.

BOOOM!

[jpflori]

Ok, so let's not use it.

[jpflori]

If you want to add the "libdir" stuff, I'll happily review it.

[fbissey]

Other than that little details:

• I would prefer cblas (C) and blas (f77) to be stored separately.
• Do you really want to enable lapack in fflas-ffpack/linbox? Is there a real point to it?

I would like to add the libdir stuff that would make my life easier thank you.

The point about accelerate on OS X is there was a question in the spkg-install so I set out to find the answer.

[jpflori]

Replying to fbissey:

Other than that little details:

• I would prefer cblas (C) and blas (f77) to be stored separately.

Sure, that makes sense.

• Do you really want to enable lapack in fflas-ffpack/linbox? Is there a real point to it?

In linbox itself it is not used. I don't remember about the fflas-ffpack part...

I would like to add the libdir stuff that would make my life easier thank you.

The point about accelerate on OS X is there was a question in the spkg-install so I set out to find the answer.

Ah, thanks!

[fbissey]

Well, you added lapack to fflas-ffpack and added lapack for linbox in module_list.py. If that doesn't enable any extra functionality or anything we should do away with it.

[jpflori]

It might enhance performance, I did not check. I only know for sure that fflas-ffpack can be configure to (potentially) use LAPACK. If you don't feel safe with this, we can postpone such an inclusion and deal with it in #14390.

[fbissey]

I would postpone the fflas-ffpack stuff, don't you have a series of ticket for givaro/fflas-ffpack/linbox update? We could also do that when we get around it.

[jpflori]

Replying to fbissey:

I would postpone the fflas-ffpack stuff, don't you have a series of ticket for givaro/fflas-ffpack/linbox update? We could also do that when we get around it.

Yes: #17635 and friends. Note we'll have to wait for a fflas-ffpack 2.1 release and linbox x.y release as the current versions of the three libs are not compatible. They should have come out in march, but didn't and Clément did not answer my latest mail.

[git]

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

​882d720

Wrong conflict resolution for numpy.

[git]

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

​73a21a9

Second try for numpy fixing.

[git]

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

​a9d2dcf

And final fixes.

[fbissey]

Do you have further things to commit or should I start putting my own comments into action?

[git]

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

​045266e

Final fix for a correct conflict resolution.

[jpflori]

I think I've sanitized the situation after my miserable failure to correctly resolve the conflict for numpy spkg-install script between this ticket and #18142.

Please go ahead.

[jdemeyer]

Is this ticket really needs_review or are further changes planned (in the latter case, set the status to needs_work please).

[fbissey]

There are a couple more changes we are/ I am planning. I should have put it back to "needs_work" earlier. I should be able to further work on this ticket tomorrow.

[jdemeyer]

When you think you're done with this ticket, you should really ask Volker to test it on the buildbots.

[fbissey]

Oh, yes indeed.

[jdemeyer]

FYI: this will conflict with #18450.

[fbissey]

I think it will be better to rebase on top of #18450 I will have to rebase the corresponding sage-on-gentoo patch anyway and it will be less patching in the end.

[jpflori]

François, any progress on that front?

[fbissey]

Sorry but no, I have got side-tracked by the upgrade of matplotlib ticket #17618 and I was thinking bumping numpy to 1.9.2 at #17642 would be trivial but it isn't and I am kind of stuck on it. Working on this ticket could actually be useful distraction but I am busy right now trying to get something scheduling properly on an AIX cluster.

[jdemeyer]

Whenever you work on this ticket, please do it in the spirit of #18450: it is better to define libraries in the .pxd files of the libraries if possible.

[fbissey]

Replying to jdemeyer:

Whenever you work on this ticket, please do it in the spirit of #18450: it is better to define libraries in the .pxd files of the libraries if possible.

Just so I am sure I understand correctly:

# distutils: libraries = gmp pari

for example will add "-lgmp -lpari" to the linking line (possibly not in that order since it could break)? Is it possible to read the correct library or import from another file? I am asking because our forward thinking is to move the definition of the blas/lapack stuff to .pc files and be able to choose between several implementations that won't all have the same name.

[jdemeyer]

Replying to fbissey:

Just so I am sure I understand correctly:

# distutils: libraries = gmp pari

for example will add "-lgmp -lpari" to the linking line

Yes indeed.

possibly not in that order since it could break

There is a global order library_order_list defined in src/module_list.py. Libraries are sorted according to that order, so the order given in the # distutils declaration is not relevant (unless the library is not listed in library_order_list).

Is it possible to read the correct library or import from another file?

Yes, see the aliases variable in src/module_list.py

Just define whatever you need in that variable and then use the alias name in the # distutils declaration.

[jdemeyer]

As far as I know, nothing in Sage directly uses BLAS, it is only used through external libraries (linbox, numpy, ...)

So it's a mystery why some extensions in src/module_list.py list the BLAS libraries without any other library. And indeed, on my Linux box, Sage still works after the following change:

src/module_list.py

@@ -1024 +1024 @@
               sources = ['sage/matrix/echelon_matrix.pyx']),
 
     Extension('sage.matrix.change_ring',
-              sources = ['sage/matrix/change_ring.pyx'],
-              libraries=[BLAS, BLAS2]),
+              sources = ['sage/matrix/change_ring.pyx']),
 
     Extension('sage.matrix.matrix',
               sources = ['sage/matrix/matrix.pyx']),
@@ -1040 +1039 @@
               sources = ['sage/matrix/matrix2.pyx']),
 
     Extension('sage.matrix.matrix_complex_double_dense',
-              sources = ['sage/matrix/matrix_complex_double_dense.pyx'],
-              libraries=[BLAS, BLAS2]),
+              sources = ['sage/matrix/matrix_complex_double_dense.pyx']),
 
     Extension('sage.matrix.matrix_cyclo_dense',
               sources = ['sage/matrix/matrix_cyclo_dense.pyx'],
@@ -1052 +1050 @@
               sources = ['sage/matrix/matrix_dense.pyx']),
 
     Extension('sage.matrix.matrix_double_dense',
-              sources = ['sage/matrix/matrix_double_dense.pyx'],
-              libraries=[BLAS, BLAS2]),
+              sources = ['sage/matrix/matrix_double_dense.pyx']),
 
     Extension('sage.matrix.matrix_generic_dense',
               sources = ['sage/matrix/matrix_generic_dense.pyx']),
@@ -1116 +1113 @@
               sources = ['sage/matrix/matrix_rational_sparse.pyx']),
 
     Extension('sage.matrix.matrix_real_double_dense',
-              sources = ['sage/matrix/matrix_real_double_dense.pyx'],
-              libraries=[BLAS, BLAS2]),
+              sources = ['sage/matrix/matrix_real_double_dense.pyx']),
 
     Extension('sage.matrix.matrix_sparse',
               sources = ['sage/matrix/matrix_sparse.pyx']),
@@ -1254 +1250 @@
               sources = ['sage/modules/module.pyx']),
 
     Extension('sage.modules.vector_complex_double_dense',
-              ['sage/modules/vector_complex_double_dense.pyx'],
-              libraries = [BLAS, BLAS2]),
+              ['sage/modules/vector_complex_double_dense.pyx']),
 
     Extension('sage.modules.vector_double_dense',
-              ['sage/modules/vector_double_dense.pyx'],
-              libraries = [BLAS, BLAS2]),
+              ['sage/modules/vector_double_dense.pyx']),
 
     Extension('sage.modules.vector_integer_dense',
               sources = ['sage/modules/vector_integer_dense.pyx']),
@@ -1278 +1272 @@
               sources = ['sage/modules/vector_rational_dense.pyx']),
 
     Extension('sage.modules.vector_real_double_dense',
-              ['sage/modules/vector_real_double_dense.pyx'],
-              libraries = [BLAS, BLAS2]),
+              ['sage/modules/vector_real_double_dense.pyx']),
 
     ################################
     ##

Perhaps those libraries are needed for some reason (I know Linux in particular is very flexible with defining libraries) but it's something to be investigated...

[jdemeyer]

See #18777.

[jdemeyer]

I did a serious clean-up of the Cython interface to GSL, see #18778.

[fbissey]

This is just merging the previous work of Jean-Pierre into 6.8.beta5 and solving the conflict. There are a couple more things I want to do. Then I should explore transferring libraries to .pxd files.

---

New commits:

​217c051

Merge branch 'develop' into blas

[git]

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

​d448831

Forgot a very important "+" in module_list.py

[jdemeyer]

I would move cblas_libs() to sage/env.py, analogous to sage_include_directories().

And this

f = open(os.path.join(SAGE_LOCAL, 'share/cblas_config'), 'r')
libs = f.readline().split()
f.close()
return libs

can be written as

with open(os.path.join(SAGE_SHARE, 'cblas_config')) as f:
    return f.readline().split()

[fbissey]

That's good suggestions. I will do that once I finished my current round of nitpicking.

[jdemeyer]

One more nitpicking: replace

cat x | sed ...

by

sed ... x

[jdemeyer]

I would also like to know the justification for this in sage-env:

-if [ "$LDFLAGS" = "" ]; then
- LDFLAGS="" && export LDFLAGS
+if [ -z "$LDFLAGS" ]; then
+ unset LDFLAGS
 fi

[fbissey]

Replying to jdemeyer:

I would also like to know the justification for this in sage-env:

-if [ "$LDFLAGS" = "" ]; then
- LDFLAGS="" && export LDFLAGS
+if [ -z "$LDFLAGS" ]; then
+ unset LDFLAGS
 fi

That must have come from one of Jean-Pierre's commit. Difference is very subtle and I am not sure it matters but I am wondering about the whole point of the test.

[jdemeyer]

Instead of

IML_BLAS=--with-cblas="$cblas_libs"

./configure "$IML_BLAS"

with awkward quoting, why not simply

./configure --with-cblas="$cblas_libs"

(same for FFLAS_FFPACK_BLAS)

[jdemeyer]

Also please use $IML_CONFIGURE instead of "$IML_CONFIGURE" (such variables are usually not quoted).

[jdemeyer]

I would also like to know the justification for the removal of

build/pkgs/numpy/patches/cygwin-lapack_lite-setup.py.diff

[jpflori]

Replying to jdemeyer:

I would also like to know the justification for this in sage-env:

-if [ "$LDFLAGS" = "" ]; then
- LDFLAGS="" && export LDFLAGS
+if [ -z "$LDFLAGS" ]; then
+ unset LDFLAGS
 fi

Because this does not do the same thing :). IIRC the first option exports LDFLAGS = "" when LDFLAGS is not set. Or something in this spirit. This can cause trouble later on (and I thing I was hit by that).

Whatsoever IIRC once again, the "new" treatment for LDFLAGS is similar to the one for CFLAGS and CPPFLAGS.

[jpflori]

Replying to jdemeyer:

Instead of

IML_BLAS=--with-cblas="$cblas_libs"

./configure "$IML_BLAS"

with awkward quoting, why not simply

./configure --with-cblas="$cblas_libs"

(same for FFLAS_FFPACK_BLAS)

I don't really remember, but both might work as well. IIRC FFLAS-FFPACK configure scripts is very sensitive to quoting and spaces and it was a nightmare to make it accept multiple libraries to be linked separated by spaces.

[jdemeyer]

Same for the removal of

build/pkgs/cvxopt/patches/setup.py.patch

It's not at all clear whether these changes are related to BLAS setup. Perhaps all non-BLAS-related changes should be factored out to a different ticket, that would make reviewing easier.

[jdemeyer]

Replying to jpflori:

Because this does not do the same thing :).

That's exactly why such changes shouldn't be smuggled inside a ticket dealing with BLAS.

[jpflori]

Replying to jdemeyer:

Same for the removal of

build/pkgs/cvxopt/patches/setup.py.patch

It's not at all clear whether these changes are related to BLAS setup.

This patch is a dirty hack to configure cvxopt to use BLAS related stuff.

Perhaps all non-BLAS-related changes should be factored out to a different ticket, that would make reviewing easier.

Sure that would be the best way to go.

[jpflori]

Replying to jdemeyer:

I would also like to know the justification for the removal of

build/pkgs/numpy/patches/cygwin-lapack_lite-setup.py.diff

I just remember this patch looked fishy and useless and I had no problem not applying it.

[jdemeyer]

Replying to jpflori:

Replying to jdemeyer:

Same for the removal of

build/pkgs/cvxopt/patches/setup.py.patch

It's not at all clear whether these changes are related to BLAS setup.

This patch is a dirty hack to configure cvxopt to use BLAS related stuff.

There is also GSL-related stuff in that patch.

[fbissey]

I guess we can split some of the cvxopt stuff to make things clearer. Prior to 1.1.7 it was difficult to set any library properly without patching. setup.py was requiring manual edit before installation unless you setup was closely matching the author. 1.1.7 introduced some mechanism to pass library settings from the environment and we just decided to do that at the same time. But a separate ticket is no problem.

[jdemeyer]

Now that you explain the cvxopt thing, it's clearer.

[git]

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

​5c9e7a5

Proper insertion in library_order

[fbissey]

I'll do some more work over the week end, the last commit is just so that sage with that branch is in a building state.

[jdemeyer]

For this to work properly, you will need to increase the patchlevel of the ATLAS package, forcing it to be rebuilt. For the other packages, I don't think it's needed if they depend on ATLAS.

[fbissey]

Replying to jdemeyer:

Now that you explain the cvxopt thing, it's clearer.

Do you still want a separate ticket?

[fbissey]

Replying to jpflori:

Replying to jdemeyer:

Instead of

IML_BLAS=--with-cblas="$cblas_libs"

./configure "$IML_BLAS"

with awkward quoting, why not simply

./configure --with-cblas="$cblas_libs"

(same for FFLAS_FFPACK_BLAS)

I don't really remember, but both might work as well. IIRC FFLAS-FFPACK configure scripts is very sensitive to quoting and spaces and it was a nightmare to make it accept multiple libraries to be linked separated by spaces.

In both case the variable is only defined on cygwin/darwin and we rely on autodetection in other cases. I guess we could throw the autodetection throw the window and put the stuff in every case. In fact if we want to be able to use something else than ATLAS in the future it is prudent to do so. So I will throw the particular cases out. If there is a strong case to still keep something for cygwin I am all ears.

[git]

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

​cc5fefc	Introduce a finer grain of configuration between blas/cblas/lapack. In the future we want to move to .pc file and the configuration will have the same granularity.
​57cbf6f	Clean up spkg-install of all packages affected by the changes in the ATLAS spkg. The new model in anticipation of future changes is to not rely on autodetection of
​eda2eb6	Centralize the reading of SAGE_SHARE/*_config files in sage/env.py where it can be accessed by setup.py/module_list.py and sage/misc/cython.py.

[fbissey]

I'll happily field comments on the new changes but I will otherwise start moving libraries to the .pxd files. After Jeroen clean up of the other tickets that's a total of 5 files (plus one optional file) so not a big job but I'll probably end with some redundancies on my first go (i.e. stuff that's not needed thanks to other declarations in .pxd).

[jdemeyer]

Replying to fbissey:

I'll happily field comments on the new changes but I will otherwise start moving libraries to the .pxd files.

I'd do that in a new ticket. There is already a lot of stuff on this ticket and I wouldn't add anything more.

I might not have much time these days to look at this ticket, so you can just go ahead. I am still very suspicious about the change to LDFLAGS in sage-env: it's a seemingly innocent change but with so much potential to break stuff. For handling CFLAGS, LDFLAGS,... in Sage we have always treated "unset" and "set to empty" equivalently. So such a change shouldn't be made lightly.

Two other small things I noticed now: there is some strange indentation in atlas/spkg-install, be sure to use mutiples of 4 spaces everywhere. And of course get_libs_config() needs a doctest.

[fbissey]

Replying to jdemeyer:

Replying to fbissey:

I'll happily field comments on the new changes but I will otherwise start moving libraries to the .pxd files.

I'd do that in a new ticket. There is already a lot of stuff on this ticket and I wouldn't add anything more.

That's a fair comment we touched things all over the place and some more will need to be "aligned" eventually.

I might not have much time these days to look at this ticket, so you can just go ahead. I am still very suspicious about the change to LDFLAGS in sage-env: it's a seemingly innocent change but with so much potential to break stuff. For handling CFLAGS, LDFLAGS,... in Sage we have always treated "unset" and "set to empty" equivalently. So such a change shouldn't be made lightly.

I am a bit concerned that there is even a need for these tests and setting/unsetting. It just feel wrong so I am probably not the best person to pass a judgement on that particular change, my own reaction is to make the whole thing disappear.

Two other small things I noticed now: there is some strange indentation in atlas/spkg-install, be sure to use mutiples of 4 spaces everywhere. And of course get_libs_config() needs a doctest.

I will check the indent ASAP and I was afraid you would ask for that doctest, but I know what to do.

@Jean-Pierre, any comments on the last few commits? As can be surmised by comments we'll eventually touch again some packages using blas/cblas/lapack to enforce settings in the future. But as Jeroen says there is enough here already.

[jpflori]

Replying to fbissey:

Replying to jdemeyer:

Replying to fbissey:

I'll happily field comments on the new changes but I will otherwise start moving libraries to the .pxd files.

I'd do that in a new ticket. There is already a lot of stuff on this ticket and I wouldn't add anything more.

That's a fair comment we touched things all over the place and some more will need to be "aligned" eventually.

I might not have much time these days to look at this ticket, so you can just go ahead. I am still very suspicious about the change to LDFLAGS in sage-env: it's a seemingly innocent change but with so much potential to break stuff. For handling CFLAGS, LDFLAGS,... in Sage we have always treated "unset" and "set to empty" equivalently. So such a change shouldn't be made lightly.

I am a bit concerned that there is even a need for these tests and setting/unsetting. It just feel wrong so I am probably not the best person to pass a judgement on that particular change, my own reaction is to make the whole thing disappear.

My only point is that now LDFLAGS just follows what CFLAGS does and gave me a better behavior. So I don't agree that we've always treated LDFLAGS and CFLAGS (and CXXFLAGS) in the same way when they are unset and empty.

The problem with the current behavior of LDFLAGS, is that if LDFLAGS is not set, then whe get LDFLAGS="". And some packages won't touch LDFLAGS if LFDLAGS="". If you feel worried about this, then just remove this change.

For the current behavior of CFLAGS, what you get is that if CFLAGS is empty, then it's unset, that might not be the best behavior as well...

Two other small things I noticed now: there is some strange indentation in atlas/spkg-install, be sure to use mutiples of 4 spaces everywhere. And of course get_libs_config() needs a doctest.

I will check the indent ASAP and I was afraid you would ask for that doctest, but I know what to do.

@Jean-Pierre, any comments on the last few commits? As can be surmised by comments we'll eventually touch again some packages using blas/cblas/lapack to enforce settings in the future. But as Jeroen says there is enough here already.

Yeah I generally agree with going step by step even if the current solution is not perfect.

I also spotted a few indentation issues in atlas/spkg-install and src/sage/env.py

[git]

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

​83fbfcf

Fix indent in ATLAS spkg-install, add a doctest for get_libs_config.

[fbissey]

Replying to jpflori:

I also spotted a few indentation issues in atlas/spkg-install and src/sage/env.py

Found some stuff in spkg-install but cannot see anything in env.py, if there is something you'll have to provide a line number. I am putting this for review now.

[jdemeyer]

49 hasn't been addressed.

[fbissey]

Replying to jdemeyer:

49 hasn't been addressed.

While I find the argument that LDFLAGS should be treated just the same as {C,CXX}FLAGS persuasive, I'll surrender to the fact that it belongs to another ticket that I'll happily review positively. Waiting for my OS X build to finish before committing that change.

[jpflori]

Yeah, just postpone it to another ticket (but also make sure that our treatment of LDFLAGS does not break CVXOPT or other package linking to BLAS which need to be fed stuff through LDFLAGS).

[jdemeyer]

Replying to fbissey:

While I find the argument that LDFLAGS should be treated just the same as {C,CXX}FLAGS persuasive

The treating of {C,CXX}FLAGS changed recently in #16149 and I don't understand why that change was needed.

Personally, I would prefer to not treat empty and non-empty FOOFLAGS fundamentally different (i.e. I would revert #16149 and leave LDFLAGS as they are).

[git]

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

​2e78027

typo in atlas spkg-install, manifest itself on OS X

[jpflori]

Replying to jdemeyer:

Replying to fbissey:

While I find the argument that LDFLAGS should be treated just the same as {C,CXX}FLAGS persuasive

The treating of {C,CXX}FLAGS changed recently in #16149 and I don't understand why that change was needed.

Personally, I would prefer to not treat empty and non-empty FOOFLAGS fundamentally different (i.e. I would revert #16149 and leave LDFLAGS as they are).

Having *FLAGS empty or unset are two different things. Maybe it should be the same, but some libraries deal with it differently. E.g. when it's unset it means do whatever you want with *FLAGS and if it is empty, it means leave it empty. Of course, this is not a universal truth and different libraries behaves differently. So as far as I'm concerned, I would just not touch *FLAGS at all.

[jdemeyer]

Replying to jpflori:

Having *FLAGS empty or unset are two different things.

I know and understand that, but my point is that it shouldn't be two different things.

[jdemeyer]

In any case, can we please in this ticket remove the changes to sage-env and discuss this somewhere else?

[fbissey]

Ticket causes problems on OS X, so not ready yet.

[fbissey]

The sed command is not doing its job

Mirage:sage-6.8.beta5 fbissey$ cat local/share/blas_config|sed 's/\([^ ]\+\)/-l\1/g'
blas

[jdemeyer]

I believe that \+ in sed regexps for "match 1 or more times" is not portable. But you can replace X\+ by XX* for any value of X.

[git]

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

​beeb264

Revert changes to sage-env

[git]

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

​b2495dd	Improve portability of sed script so that it works on OS X.
​764c3b5	Forgot to close the conditional when reversing LDFLAGS changes in sage-env

[fbissey]

Thanks for the portability tip Jeroen, that got me through the stuff using autotools. OS X stuff has been compromised in numpy and very likely scipy, working on that.

[jdemeyer]

I just got an idea: why aren't we using a pkgconfig file instead of local/share/blas_config? I seems like pkgconfig is exactly meant to solve the problems we're having on this ticket.

A typical file looks like

$ cat local/lib/pkgconfig/m4ri.pc 
SAGE_ROOT=/usr/local/src/sage-config
prefix=${SAGE_ROOT}/local
exec_prefix=${prefix}
libdir=${SAGE_ROOT}/local/lib
includedir=${prefix}/include

Name: M4RI
Description: Dense linear algebra over GF(2).
Version: 20140914
Libs: -L${libdir} -lm4ri -lm
Cflags: -I${includedir}/m4ri  -I${SAGE_ROOT}/local/include -g -fPIC -Wall -pedantic -O2  -mmmx -msse -msse2 -msse3 

I am sure we can make a blas.pc file using this format.

From the shell, you can do for example

(sage-sh) $ pkgconf --libs m4ri
-lm4ri -lm  

which would replace the ugly sed script.

There is also a Python interface:

sage: import pkgconfig
sage: pkgconfig.libs("m4ri")
u'-L/usr/local/src/sage-config/local/lib -lm4ri -lm'

[jpflori]

Using pkgconfig is actually the final destination. This ticket was just a cleanup ticket before #17075.

[jdemeyer]

What you're doing here on this ticket with $SAGE_SHARE/blas_config is very close to pkgconfig. So why not just use pkgconfig instead?

[jpflori]

I guess that at the time this ticket was opened, pkgconfig was not standard though I could not swear it.

[fbissey]

When I started talking about #17075 and associated work to be able to use any blas/lapack our goal was to use pkgconfig and https://github.com/matze/pkgconfig as a python interface to it. This is what sage-on-gentoo is currently using. The fact that the python interface was also standard escaped me.

We included pkgconfig in sage when we upgraded to matplotlib-1.3.x after I determined that it was the only to build it on OS X while reviewing that ticket. That would have been after #17075 but I am not sure if it was before this ticket.

For reference this is the current sage-on-gentoo patch for 6.8.beta6 ​https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-6.8-blas-r1.patch

The librarydir part is for dealing with BLAS/LAPACK installed in less standard location (/opt) where they may be out of the library search path when linking. It still relies on said path to be in ld.so.config or equivalent. Typical cases are intel MKL and AMD acml. The ability to compile against MKL is one of the key motivation here.

Since we have all the elements, going .pc all the way is fine. I'll rewrite the whole thing.

As mentioned before I spent some time working on OS X and that's also one of my motivation to rewrite some stuff. That's an interesting little story that touch sage-env (surprise)....

So Jean-Pierre put this in numpy and scipy

if [ -n "${LDFLAGS+set}" ]; then
    if [ "$UNAME" != "Darwin" ]; then
        export LDFLAGS="-bundle -undefined dynamic_lookup $LDFLAGS"
        export CPPFLAGS="-D__ACCELERATE__ $CPPFLAGS"
    else
        export LDFLAGS="-shared $LDFLAGS"
    fi
fi

Which definitely looked wrong on first glance but I trusted Jean-Pierre. Well it is wrong and compilation will fail on OS X because somehow you'll have -bundle and -shared in LDFLAGS at the same time. Unless you include Jean-Pierre changes to sage-env in which case numpy at least compile. I'll hazard that the change to sage-env enable compilation under linux as well somehow.

So there we are the change to sage-env was allowing wrong stuff in numpy and scipy (not on OS X) to work to some extent.

My local branch is in a bit of a mess and I think I pushed too much stuff on trac so I'll get a clean branch in.

[jpflori]

Replying to fbissey:

When I started talking about #17075 and associated work to be able to use any blas/lapack our goal was to use pkgconfig and https://github.com/matze/pkgconfig as a python interface to it. This is what sage-on-gentoo is currently using. The fact that the python interface was also standard escaped me.

We included pkgconfig in sage when we upgraded to matplotlib-1.3.x after I determined that it was the only to build it on OS X while reviewing that ticket. That would have been after #17075 but I am not sure if it was before this ticket.

For reference this is the current sage-on-gentoo patch for 6.8.beta6 ​https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-6.8-blas-r1.patch

The librarydir part is for dealing with BLAS/LAPACK installed in less standard location (/opt) where they may be out of the library search path when linking. It still relies on said path to be in ld.so.config or equivalent. Typical cases are intel MKL and AMD acml. The ability to compile against MKL is one of the key motivation here.

Since we have all the elements, going .pc all the way is fine. I'll rewrite the whole thing.

As mentioned before I spent some time working on OS X and that's also one of my motivation to rewrite some stuff. That's an interesting little story that touch sage-env (surprise)....

So Jean-Pierre put this in numpy and scipy

if [ -n "${LDFLAGS+set}" ]; then
    if [ "$UNAME" != "Darwin" ]; then
        export LDFLAGS="-bundle -undefined dynamic_lookup $LDFLAGS"
        export CPPFLAGS="-D__ACCELERATE__ $CPPFLAGS"
    else
        export LDFLAGS="-shared $LDFLAGS"
    fi
fi

Which definitely looked wrong on first glance but I trusted Jean-Pierre.

I wouldn't it definitely looks wrong on non-OS Xes :) Read the comment above this part of code. If LDFLAGS is set, you need to add -shared yourself. This is exactly what is done here. And yes you are right, this must be the reason for my changes to sage-env, as if LDFLAGS is set but empty, then shared won't be added and the compilation will fail. The easy solution is to unconditionnally add -shared to LDFLAGS on non-OS X.

As far as OS X is concerned, I guess I got this from "the web" as I don't have access to any OS X machine and cried for testing here before.

[jpflori]

In fact the OS X stuff comes from the previous scipy spkg-install.

If think that my idea to test whether LDFLAGS is set and only modify it if it is not (differently on OS X and non-OS Xes), is that if LDFLAGS is set then the user did it and knows what he did and will know how to fix it if compilation fails.

[jdemeyer]

Just a suggestion, feel free to ignore:

do we really need to change all packages using BLAS at the same time? In order to get something in Sage more quickly, could we for example just change ATLAS and the Sage library in this ticket and postpone the other packages to a new ticket?

[fbissey]

I approve the motion and that will enable me to move a bit faster and not to deal with the R bump. I would very much like to deal with #17642 next. That and making cython() use distutils as talked about in #18825

[jpflori]

If possible, let's modularize (though I won't personally have time to do anything serious before the 16th or 20th :( ). I think we could even make this ticket a task and:

• write pkgconfig files for ATLAS in #17075,
• clean up packages using BLAS in dedicated tickets.

Also note that the new LinBox?/FFLAS-FFPACK/Givaro compatible versions should be out very soon (only LinBox? 2.x is missing but there is currently a workshop taking place in Grenoble from what I understood).

[fbissey]

I have, I believe, finished the rewrite of ATLAS' spkg-install to create .pc files. I can push it first thing after breakfast to the right ticket.

[jdemeyer]

Is this ticket still relevant now that we have #17075?

[jpflori]

I think we can close this one.

[embray]

Closing tickets in the sage-duplicate/invalid/wontfix module with positive_review (i.e. someone has confirmed they should be closed).