#29502 closed enhancement (fixed)
spkg-configure for suitesparse
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.2 |
Component: | build: configure | Keywords: | |
Cc: | mkoeppe, mjo, gh-thierry-FreeBSD, fbissey | Merged in: | |
Authors: | Dima Pasechnik | Reviewers: | Michael Orlitzky |
Report Upstream: | N/A | Work issues: | |
Branch: | f2cc5b4 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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.
Attachments (1)
Change History (60)
comment:1 Changed 2 years ago by
- Cc gh-thierry-FreeBSD added
comment:2 Changed 2 years ago by
- Cc fbissey added
- Description modified (diff)
Changed 2 years ago by
comment:3 Changed 2 years ago by
comment:4 Changed 2 years ago by
- Branch set to u/dimpase/packages/suitesparseconf
- Commit set to 10ffa1f72a788e05e4e826c18ac899ad678b13aa
please try this branch (not tested much, though)
New commits:
10ffa1f | spkg-configure for suitesparse (draft)
|
comment:5 Changed 2 years ago by
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.
comment:6 Changed 2 years ago by
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.
comment:7 Changed 2 years ago by
On Debian, the following hack makes things work:
-
build/pkgs/cvxopt/spkg-install.in
a b export CVXOPT_BLAS_LIB="$(pkg_libs blas)" 21 21 export CVXOPT_BLAS_LIB_DIR="$(pkg-config --variable=libdir blas)" 22 22 export CVXOPT_LAPACK_LIB="$(pkg_libs lapack)" 23 23 24 export CVXOPT_SUITESPARSE_LIB_DIR="${SAGE_LOCAL}"25 export CVXOPT_SUITESPARSE_INC_DIR="${SAGE_LOCAL}/include"24 # export CVXOPT_SUITESPARSE_LIB_DIR="${SAGE_LOCAL}" 25 # export CVXOPT_SUITESPARSE_INC_DIR="${SAGE_LOCAL}/include" 26 26 27 27 export CVXOPT_BUILD_GLPK=1 28 28 export CVXOPT_GLPK_LIB_DIR="${SAGE_LOCAL}"
I'll do a proper fix later, but this should get one
comment:8 follow-up: ↓ 9 Changed 2 years ago by
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?
comment:9 in reply to: ↑ 8 Changed 2 years ago by
Replying to gh-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).
comment:10 follow-up: ↓ 11 Changed 2 years ago by
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?
comment:11 in reply to: ↑ 10 Changed 2 years ago by
Replying to gh-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.
comment:12 Changed 2 years ago by
- Commit changed from 10ffa1f72a788e05e4e826c18ac899ad678b13aa to b7b85d7915c94159287213235400cd2be6956fad
Branch pushed to git repo; I updated commit sha1. New commits:
b7b85d7 | more distros info
|
comment:13 Changed 2 years ago by
- Description modified (diff)
comment:14 Changed 2 years ago by
So the FreeBSD question is whether adding -I /usr/local/include/
to CFLAGS
and comment:7 allow the thing to build.
comment:15 Changed 2 years ago by
- Commit changed from b7b85d7915c94159287213235400cd2be6956fad to 184b88ad58fd788f11f6637e613015cf90f25f87
comment:16 Changed 2 years ago by
- Status changed from new to needs_review
comment:17 Changed 2 years ago by
Great! The missing part was SAGE_SUITESPARSE_PREFIX.
comment:18 Changed 2 years ago by
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)
comment:19 Changed 2 years ago by
- Milestone changed from sage-9.2 to sage-9.1
perhaps this can make it into 9.1?
comment:20 Changed 2 years ago by
Needs rebasing on top of #29492 or something like that
comment:21 Changed 2 years ago by
I can't seem to get the branch of #29492 for some reason.
comment:22 Changed 2 years ago by
- Status changed from needs_review to needs_work
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.
comment:23 follow-up: ↓ 24 Changed 2 years ago by
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.
comment:24 in reply to: ↑ 23 Changed 2 years ago by
Replying to mjo:
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.
comment:25 Changed 2 years ago by
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.
comment:26 Changed 2 years ago by
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?
comment:27 Changed 2 years ago by
- Commit changed from 184b88ad58fd788f11f6637e613015cf90f25f87 to 18f3e9c1fc6a0175fb3cabd0eef546c3fcceafae
comment:28 Changed 2 years ago by
- Status changed from needs_work to needs_review
rebased over 9.1.rc1 - needs review
comment:29 Changed 2 years ago by
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
.
comment:30 follow-up: ↓ 31 Changed 2 years ago by
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?
comment:31 in reply to: ↑ 30 Changed 2 years ago by
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.
comment:32 Changed 2 years ago by
- Commit changed from 18f3e9c1fc6a0175fb3cabd0eef546c3fcceafae to 7bc622bf554be5f6f0f754677ce59f186762f4da
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
|
comment:33 Changed 2 years ago by
- Commit changed from 7bc622bf554be5f6f0f754677ce59f186762f4da to b8c83088cff21aec5c26866435f18bf10d1300f4
Branch pushed to git repo; I updated commit sha1. New commits:
b8c8308 | check for amd.h too
|
comment:34 Changed 2 years ago by
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
|
comment:35 Changed 2 years ago by
- Description modified (diff)
comment:36 Changed 2 years ago by
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.
comment:37 Changed 2 years ago by
- Commit changed from b8c83088cff21aec5c26866435f18bf10d1300f4 to 45d6d0760d841c2b79794b696eb91f3eb46d3887
Branch pushed to git repo; I updated commit sha1. New commits:
45d6d07 | adjust gentoo libs list
|
comment:38 Changed 2 years ago by
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?
comment:39 follow-up: ↓ 40 Changed 2 years ago by
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.
comment:40 in reply to: ↑ 39 ; follow-up: ↓ 42 Changed 2 years ago by
- Status changed from needs_review to needs_work
Replying to fbissey:
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."
comment:41 Changed 2 years ago by
Works for me on OS X, for what it's worth.
comment:42 in reply to: ↑ 40 Changed 2 years ago by
Replying to mjo:
Replying to fbissey:
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 forAC_CHECK_HEADERS
say "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
.
comment:43 follow-up: ↓ 44 Changed 2 years ago by
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.
comment:44 in reply to: ↑ 43 Changed 2 years ago by
Replying to mjo:
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.
Sounds valid.
comment:45 Changed 2 years ago by
- Commit changed from 45d6d0760d841c2b79794b696eb91f3eb46d3887 to e0d812f6786d1846e40257bdc61059514a2c7d1f
Branch pushed to git repo; I updated commit sha1. New commits:
e0d812f | correct the use of AC_CHECK_HEADERS
|
comment:46 Changed 2 years ago by
- Status changed from needs_work to needs_review
this appears to fix the use of AC_CHECK_HEADERS - thanks for catching this.
comment:47 Changed 2 years ago by
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?
comment:48 follow-up: ↓ 50 Changed 2 years ago by
I am not trying to compute and pass the prefix of the installation of suitesparse. So it is probably a misnaming.
comment:49 Changed 2 years ago by
- Commit changed from e0d812f6786d1846e40257bdc61059514a2c7d1f to f2cc5b457f4945d15970bf823f5046d62564522a
Branch pushed to git repo; I updated commit sha1. New commits:
f2cc5b4 | added quoting, renamed SAGE_SUITESPARSE_PREFIX to <>_LOCALINSTALL
|
comment:50 in reply to: ↑ 48 Changed 2 years ago by
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?
comment:51 follow-up: ↓ 52 Changed 2 years ago by
yes, it is a boolean
comment:52 in reply to: ↑ 51 ; follow-up: ↓ 54 Changed 2 years ago by
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?
comment:53 Changed 2 years ago by
I would suggest to defer this to 9.2
comment:54 in reply to: ↑ 52 ; follow-up: ↓ 55 Changed 2 years ago by
Replying to mjo:
run it though github first to make sure all of the other distros have usable versions of suitesparse?
Yes please
comment:55 in reply to: ↑ 54 Changed 2 years ago by
Replying to mkoeppe:
Replying to mjo:
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)
comment:56 Changed 2 years ago by
- Milestone changed from sage-9.1 to sage-9.2
comment:57 Changed 2 years ago by
- Reviewers set to Michael Orlitzky
- Status changed from needs_review to positive_review
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.
comment:58 Changed 2 years ago by
- Branch changed from u/dimpase/packages/suitesparseconf to f2cc5b457f4945d15970bf823f5046d62564522a
- Resolution set to fixed
- Status changed from positive_review to closed
comment:59 Changed 2 years ago by
- Commit f2cc5b457f4945d15970bf823f5046d62564522a deleted
Follow-up: #30052 ubuntu-eoan-i386: cvxopt build fails
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.