spkg-configure for suitesparse
dimpase opened this issue · 70 comments
Different distos use different subsets of SuiteSparse,
but this is needed to be done. Apparently all we need is CHOLMOD (with its dependencies),UMFRACK and suitesparseconfig.
CC: @mkoeppe @orlitzky @thierry-FreeBSD @kiwifb
Component: build: configure
Author: Dima Pasechnik
Branch: f2cc5b4
Reviewer: Michael Orlitzky
Issue created by migration from https://trac.sagemath.org/ticket/29502
Description changed:
---
+++
@@ -1,2 +1,2 @@
I guess that different distos use different subsets of SuiteSparse,
-but this is needed to be done.
+but this is needed to be done. Apparently all we need is CHOLMOD (with its dependencies).Attachment: umfpack.m4.gz
Thanks! I do not know anything about macro m4 and autotools, but I already tried to work on this. For that, I borrowed a file umfpack.m4 from the Dune project, before they switched to cmake. It is released under a GPL license. Please see the attachment if it could help.
please try this branch (not tested much, though)
New commits:
10ffa1f | spkg-configure for suitesparse (draft) |
Configure is OK for iml and suitesparse:
Checking whether SageMath should install SPKG iml...
checking whether any of gmp mpir openblas is installed or will be installed as SPKG... no
checking for iml.h... yes
checking for library containing nonsingSolvLlhsMM... -liml
configure: will use system package and not install SPKG iml
...
Checking whether SageMath should install SPKG suitesparse...
checking whether any of openblas is installed or will be installed as SPKG... no
checking suitesparse/SuiteSparse_config.h usability... yes
checking suitesparse/SuiteSparse_config.h presence... yes
checking for suitesparse/SuiteSparse_config.h... yes
checking for library containing cholmod_speye... -lcholmod
configure: will use system package and not install SPKG suitesparse
...
iml-1.0.4p1.p2: using system package; SPKG will not be installed
...
suitesparse-5.6.0: using system package; SPKG will not be installed
But now cvxopt fails:
[cvxopt-1.2.3] src/C/umfpack.c:23:10: fatal error: 'umfpack.h' file not found
[cvxopt-1.2.3] #include "umfpack.h"
Note: on FreeBSD umfpack.h is located under /usr/local/include/suitesparse/umfpack.h. Just let me know if I fix it for FreeBSD or if the same failure could occur on other systems.
On Debian the location of the header is also prefixed.
Probably this is a cvxopt bug.
One might try to find out how Debian works around this.
On Debian, the following hack makes things work:
--- a/build/pkgs/cvxopt/spkg-install.in
+++ b/build/pkgs/cvxopt/spkg-install.in
@@ -21,8 +21,8 @@ export CVXOPT_BLAS_LIB="$(pkg_libs blas)"
export CVXOPT_BLAS_LIB_DIR="$(pkg-config --variable=libdir blas)"
export CVXOPT_LAPACK_LIB="$(pkg_libs lapack)"
-export CVXOPT_SUITESPARSE_LIB_DIR="${SAGE_LOCAL}"
-export CVXOPT_SUITESPARSE_INC_DIR="${SAGE_LOCAL}/include"
+# export CVXOPT_SUITESPARSE_LIB_DIR="${SAGE_LOCAL}"
+# export CVXOPT_SUITESPARSE_INC_DIR="${SAGE_LOCAL}/include"
export CVXOPT_BUILD_GLPK=1
export CVXOPT_GLPK_LIB_DIR="${SAGE_LOCAL}"I'll do a proper fix later, but this should get one
A naive work-around (just adding -I /usr/local/include/suitesparse) was sufficient to build everything.
I'm going to test your patch to check if it works for me.
Note about u/dimpase/packages/suitesparseconf: actually cvxopt produces 3 python libraries, site-packages/cvxopt/amd.so, site-packages/cvxopt/cholmod.so and site-packages/cvxopt/umfpack.so, and they are linked to libamd.so.2, libsuitesparseconfig.so.5 and libumfpack.so.5 from suitesparse; maybe you would check if they are available in its spkg-configure.m4?
Replying to @thierry-FreeBSD:
Note about u/dimpase/packages/suitesparseconf: actually cvxopt produces 3 python libraries, site-packages/cvxopt/amd.so, site-packages/cvxopt/cholmod.so and site-packages/cvxopt/umfpack.so, and they are linked to libamd.so.2, libsuitesparseconfig.so.5 and libumfpack.so.5 from suitesparse; maybe you would check if they are available in its spkg-configure.m4?
AMD, COLAMD, CCOLAMD are dependencies of CHOLMOD in SuiteSparse, so it apparently suffices to add a check for UMFPACK (and suitesparseconfig - which presumably is always there, but OK).
Same error on FreeBSD with the Debian's patch of build/pkgs/cvxopt/spkg-install.in : umfpack.h is not found.
I would set something like:
export CVXOPT_SUITESPARSE_INC_DIR="${LOCALBASE}/suitesparse/include"
but $LOCALBASE is not set!
Would it be possible to modify build/pkgs/suitesparse/spkg-configure.m4 so that it sets a variable of the include path where SuiteSparse_config.h has been found?
Replying to @thierry-FreeBSD:
Same error on FreeBSD with the Debian's patch of build/pkgs/cvxopt/spkg-install.in : umfpack.h is not found.
I would set something like:
export CVXOPT_SUITESPARSE_INC_DIR="${LOCALBASE}/suitesparse/include"
but $LOCALBASE is not set!
Would it be possible to modify build/pkgs/suitesparse/spkg-configure.m4 so that it sets a variable of the include path where SuiteSparse_config.h has been found?
getting a full include path is always a pain, and should not be needed as long as it is in the default compiler path for headers. Doesn't FreeBSD's clang look in /usr/local/include/ by default (which is always the case on every Linux I know, and on MacOS) ? Apparently it does not (which is insane, IMHO) - otherwise I don't understand this error on FreeBSD.
Branch pushed to git repo; I updated commit sha1. New commits:
b7b85d7 | more distros info |
Author: Dima Pasechnik
Description changed:
---
+++
@@ -1,2 +1,4 @@
I guess that different distos use different subsets of SuiteSparse,
-but this is needed to be done. Apparently all we need is CHOLMOD (with its dependencies).
+but this is needed to be done. Apparently all we need is CHOLMOD (with its dependencies),UMFRACK and suitesparseconfig.
+
+We assume that the headers are prefixed with `suitesparse/`So the FreeBSD question is whether adding -I /usr/local/include/ to CFLAGS and comment:7 allow the thing to build.
Great! The missing part was SAGE_SUITESPARSE_PREFIX.
tests being run here: https://github.com/dimpase/sage/actions?query=workflow%3A%22Run+SAGE_ROOT%2Ftox.ini%22
looks good (modulo unrelated things, e.g. problems with brial on Fedora, etc)
perhaps this can make it into 9.1?
On Gentoo, I think users only need to install
- sci-libs/cholmod
- sci-libs/suitesparseconfig
- sci-libs/umfpack
which is a bit less than the whole suitesparse. But currently it doesn't work anyway because the check is looking for the header in the wrong place. Unless they moved it in v5.6.0, upstream SuiteSparse_config installs the header without the suitesparse/ prefix.
CVXOPT however checks both /usr/include and /usr/include/suitesparse, and nobody else looks for that header, so I think it would be fine to just add the un-prefixed path as an option to that header check.
Looking at CVXOPT's setup.py,
https://github.com/cvxopt/cvxopt/blob/master/setup.py
I also see "amd" (sci-libs/amd) and "colamd" (sci-libs/colamd) make an appearance. I don't pretend to understand what's going on in there, but it's worth a look to see if we need to check for those libraries too.
Replying to @orlitzky:
Looking at CVXOPT's setup.py,
https://github.com/cvxopt/cvxopt/blob/master/setup.py
I also see "amd" (sci-libs/amd) and "colamd" (sci-libs/colamd) make an appearance. I don't pretend to understand what's going on in there, but it's worth a look to see if we need to check for those libraries too.
Those two and even suitesparseconfig should be pulled in as dependency of cholmod. However, if we need dev packages for those as well then we need to check for them.
Yup, I can at least see amd.h in cvxopt sources. So we need to check dev packages for those. Haven't seen colamd, but that cannot hurt.
amd is a dependency of cholmod.
as to the headers location, unprefixed 3-letter filenames for headers as upstream does are silly. Is Gentoo the only distro that does not prefix it?
rebased over 9.1.rc1 - needs review
Sorry, I've been missing trac notifications again.
If amd.h is used by cvxopt, we should check for it, because users will probably need to install a -dev package on the binary distros to get it. If we don't check here, the ./configure script might think everything's OK with only e.g. amd installed and not amd-dev until the cvxopt build system crashes much later on.
I agree that not prefixing the headers upstream shows a lack of foresight, but renaming other people's headers as a workaround is a dick move. Gentoo can't prefix them for the same reason no one else should prefix them: other software needs to know where to find them. If we move the headers, every consumer of the library fails to build. Renaming the headers completely changes a package's public API. It's on the same level as changing all of a library's function names to swahili to avoid symbol collisions and then packaging it up with the same name. It can work on a binary distro, but only if you're OK with supporting only pre-built binary software forever. When every binary distro does it, you wind up with n*m different cases in the n packages looking for the m places that someone decided to put them.
Slackware, nix, conda, and homebrew are also likely to have them unprefixed for that reason. Not to mention people who just go the website, download the tarball, and install to /usr/local.
Am I correst that cvxopt is able to work with suitesparse headers anywhere, so it's just a matter of checking for headers at another location too, right?
Replying to @dimpase:
Am I correst that cvxopt is able to work with suitesparse headers anywhere, so it's just a matter of checking for headers at another location too, right?
Right. As n=1 here, you can see the 1*m cases attempted in the setup.py I linked earlier.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
01d9afb | spkg-configure for suitesparse (draft) |
8aec534 | more distros info |
e6b207b | set up SAGE_SUITESPARSE_PREFIX |
17a95e6 | remove an old FreeBSD leftover |
3d9d0c1 | tests for other libs in sparsesuite |
7bc622b | check for unprefixed header, too |
Branch pushed to git repo; I updated commit sha1. New commits:
b8c8308 | check for amd.h too |
test run here: https://github.com/dimpase/sage/actions/runs/88161668
New commits:
b8c8308 | check for amd.h too |
New commits:
b8c8308 | check for amd.h too |
Description changed:
---
+++
@@ -1,4 +1,4 @@
-I guess that different distos use different subsets of SuiteSparse,
+Different distos use different subsets of SuiteSparse,
but this is needed to be done. Apparently all we need is CHOLMOD (with its dependencies),UMFRACK and suitesparseconfig.
-We assume that the headers are prefixed with `suitesparse/`
+My system copy is detected now, thanks. In gentoo.txt, I think we should have only
sci-libs/amd sci-libs/cholmod sci-libs/suitesparseconfig sci-libs/umfpack
The sci-libs/suitesparse in there right now is a metapackage that pulls in everything. I'm still not sure about "colamd", but we're safe in any case so long as it remains a dependency of cholmod.
Branch pushed to git repo; I updated commit sha1. New commits:
45d6d07 | adjust gentoo libs list |
I was going to test this with the suitesparse-5.4.0 in gentoo (to see if we need version bounds), but I can't figure out which part of sage (if any) actually uses suitesparse. It's listed as a standard package, so it should be used somewhere. Does anyone have any ideas?
It is exclusively a dependency of cvxopt. Nothing else uses it - although we could unbundle some bit of glpk to use it. That's about it.
Replying to @kiwifb:
It is exclusively a dependency of cvxopt. Nothing else uses it - although we could unbundle some bit of glpk to use it. That's about it.
Oh, I could have sworn that cvxopt was an optional package. That makes sense, then.
This test,
AC_CHECK_HEADERS([SuiteSparse_config.h amd.h],
[suispa_header_found=yes])
is intended to set suispa_header_found to "yes" if both headers are found, but the docs for AC_CHECK_HEADERS say "If action-if-found is given, it is additional shell code to execute when one of the header files is found."
Works for me on OS X, for what it's worth.
Replying to @orlitzky:
Replying to @kiwifb:
It is exclusively a dependency of cvxopt. Nothing else uses it - although we could unbundle some bit of glpk to use it. That's about it.
Oh, I could have sworn that cvxopt was an optional package. That makes sense, then.
This test,
AC_CHECK_HEADERS([SuiteSparse_config.h amd.h], [suispa_header_found=yes])is intended to set
suispa_header_foundto "yes" if both headers are found, but the docs forAC_CHECK_HEADERSsay "If action-if-found is given, it is additional shell code to execute when one of the header files is found."
Yes, you don't want to do that. That code would be executed if either of the headers were to be found, so the variable would be defined even if one is missing. You would be better to check for ac_cv_header_suitesparse_config_h and ac_cv_header_amd_h (are those name correct?) in a shell test after the call to AC_CHECK_HEADERS.
Another option is to set the "headers found" variable to "yes" by default, and then use the action-if-not-found branch of AC_CHECK_HEADERS to set it to "no" if some header is not found. You'd have to reset the variable between the prefixed and unprefixed checks, but it looks like it would work anyway.
Replying to @orlitzky:
Another option is to set the "headers found" variable to "yes" by default, and then use the action-if-not-found branch of
AC_CHECK_HEADERSto set it to "no" if some header is not found. You'd have to reset the variable between the prefixed and unprefixed checks, but it looks like it would work anyway.
Sounds valid.
Branch pushed to git repo; I updated commit sha1. New commits:
e0d812f | correct the use of AC_CHECK_HEADERS |
this appears to fix the use of AC_CHECK_HEADERS - thanks for catching this.
Looks good now. One last nitpick; you should add quotes here for the people who have spaces in their sage path:
if test x$SAGE_SUITESPARSE_PREFIX != x; then
export CVXOPT_SUITESPARSE_LIB_DIR="${SAGE_LOCAL}"
export CVXOPT_SUITESPARSE_INC_DIR="${SAGE_LOCAL}/include"
fi
And while the two are equivalent, I think using SAGE_SUITESPARSE_PREFIX instead of SAGE_LOCAL...
if test "x$SAGE_SUITESPARSE_PREFIX" != "x"; then
export CVXOPT_SUITESPARSE_LIB_DIR="${SAGE_SUITESPARSE_PREFIX}"
export CVXOPT_SUITESPARSE_INC_DIR="${SAGE_SUITESPARSE_PREFIX}/include"
fi
avoids some duplication and uses the SAGE_SUITESPARSE_PREFIX variable the way you intended it?
I am not trying to compute and pass the prefix of the installation of suitesparse.
So it is probably a misnaming.
Branch pushed to git repo; I updated commit sha1. New commits:
f2cc5b4 | added quoting, renamed SAGE_SUITESPARSE_PREFIX to <>_LOCALINSTALL |
Replying to @dimpase:
I am not trying to compute and pass the prefix of the installation of suitesparse.
So it is probably a misnaming.
Is it just supposed to be a boolean? I guess what I'm asking is, why define SAGE_SUITESPARSE_LOCALINSTALL to either $SAGE_LOCAL or the empty string if it's only used as a flag?
yes, it is a boolean
Replying to @dimpase:
yes, it is a boolean
It think it would make more sense to set it to "yes" rather than "$SAGE_LOCAL" in that case?
Either way, it works fine on Gentoo with suitesparse-5.4.0. Should we merge it and cross our fingers, or run it though github first to make sure all of the other distros have usable versions of suitesparse?
I would suggest to defer this to 9.2
Replying to @orlitzky:
run it though github first to make sure all of the other distros have usable versions of suitesparse?
Yes please
Replying to @mkoeppe:
Replying to @orlitzky:
run it though github first to make sure all of the other distros have usable versions of suitesparse?
Yes please
I did this https://github.com/dimpase/sage/actions/runs/88161666
before commits starting at comment:45 and have not found any problems.
(apart from the trivial absense of useful system openblas implying no suitesparse,
either)
Reviewer: Michael Orlitzky
The spkg-install for cvxopt is doing some weird stuff, but I can't find any immediate problems (and don't think we need to fix them here, in any case). For example,
export CVXOPT_GLPK_LIB_DIR="${SAGE_LOCAL}"
export CVXOPT_GLPK_INC_DIR="${SAGE_LOCAL}/include"
is unconditionally adding those two library/include paths to the command line as -L and -I flags, even when we're using system packages. This doesn't cause an immediate problem because --with-system-foo=yes and --with-system-foo=force get ignored if the foo SPKG is installed, and that's the only way the extra dirs on the command-line could cause a problem. So for example if I install the suitesparse SPKG manually and attempt to use the system suitesparse, there's no risk of the SPKG being accidentally linked into cvxopt via the lines above, because there's no way to avoid using the SPKG in favor of the system copy in the first place.
These calls to pkg-config share the same risk,
export CVXOPT_GSL_LIB_DIR="$(pkg-config --variable=libdir gsl)"
export CVXOPT_GSL_INC_DIR="$(pkg-config --variable=includedir gsl)"
but so long as any SPKG that is installed and provides a *.pc file must be used (ignoring --with-system flags), they won't hurt.
Changed branch from u/dimpase/packages/suitesparseconf to f2cc5b4