Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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:

Status badges

Description (last modified by dimpase)

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)

umfpack.m4 (4.8 KB) - added by gh-thierry-FreeBSD 2 years ago.

Download all attachments as: .zip

Change History (60)

comment:1 Changed 2 years ago by dimpase

  • Cc gh-thierry-FreeBSD added

comment:2 Changed 2 years ago by dimpase

  • Cc fbissey added
  • Description modified (diff)

Changed 2 years ago by gh-thierry-FreeBSD

comment:3 Changed 2 years ago by gh-thierry-FreeBSD

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.

comment:4 Changed 2 years ago by dimpase

  • Branch set to u/dimpase/packages/suitesparseconf
  • Commit set to 10ffa1f72a788e05e4e826c18ac899ad678b13aa

please try this branch (not tested much, though)


New commits:

10ffa1fspkg-configure for suitesparse (draft)

comment:5 Changed 2 years ago by gh-thierry-FreeBSD

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 dimpase

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 dimpase

On Debian, the following hack makes things work:

  • build/pkgs/cvxopt/spkg-install.in

    a b export CVXOPT_BLAS_LIB="$(pkg_libs blas)" 
    2121export CVXOPT_BLAS_LIB_DIR="$(pkg-config --variable=libdir blas)"
    2222export CVXOPT_LAPACK_LIB="$(pkg_libs lapack)"
    2323
    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"
    2626
    2727export CVXOPT_BUILD_GLPK=1
    2828export CVXOPT_GLPK_LIB_DIR="${SAGE_LOCAL}"

I'll do a proper fix later, but this should get one

comment:8 follow-up: Changed 2 years ago by gh-thierry-FreeBSD

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 dimpase

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: Changed 2 years ago by 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?

comment:11 in reply to: ↑ 10 Changed 2 years ago by dimpase

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 git

  • Commit changed from 10ffa1f72a788e05e4e826c18ac899ad678b13aa to b7b85d7915c94159287213235400cd2be6956fad

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

b7b85d7more distros info

comment:13 Changed 2 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Description modified (diff)

comment:14 Changed 2 years ago by dimpase

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 git

  • Commit changed from b7b85d7915c94159287213235400cd2be6956fad to 184b88ad58fd788f11f6637e613015cf90f25f87

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

0f5619cset up SAGE_SUITESPARSE_PREFIX
9b6b1a8remove an old FreeBSD leftover
184b88atests for other libs in sparsesuite

comment:16 Changed 2 years ago by dimpase

  • Status changed from new to needs_review

comment:17 Changed 2 years ago by gh-thierry-FreeBSD

Great! The missing part was SAGE_SUITESPARSE_PREFIX.

comment:18 Changed 2 years ago by dimpase

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 dimpase

  • 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 mkoeppe

Needs rebasing on top of #29492 or something like that

comment:21 Changed 2 years ago by dimpase

I can't seem to get the branch of #29492 for some reason.

comment:22 Changed 2 years ago by mjo

  • 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: Changed 2 years ago by 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.

comment:24 in reply to: ↑ 23 Changed 2 years ago by fbissey

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 fbissey

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 dimpase

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 git

  • Commit changed from 184b88ad58fd788f11f6637e613015cf90f25f87 to 18f3e9c1fc6a0175fb3cabd0eef546c3fcceafae

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f091320spkg-configure for suitesparse (draft)
84d124bmore distros info
e6d8c59set up SAGE_SUITESPARSE_PREFIX
8f135bfremove an old FreeBSD leftover
18f3e9ctests for other libs in sparsesuite

comment:28 Changed 2 years ago by dimpase

  • Status changed from needs_work to needs_review

rebased over 9.1.rc1 - needs review

comment:29 Changed 2 years ago by mjo

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: Changed 2 years ago by 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?

comment:31 in reply to: ↑ 30 Changed 2 years ago by mjo

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 git

  • Commit changed from 18f3e9c1fc6a0175fb3cabd0eef546c3fcceafae to 7bc622bf554be5f6f0f754677ce59f186762f4da

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

01d9afbspkg-configure for suitesparse (draft)
8aec534more distros info
e6b207bset up SAGE_SUITESPARSE_PREFIX
17a95e6remove an old FreeBSD leftover
3d9d0c1tests for other libs in sparsesuite
7bc622bcheck for unprefixed header, too

comment:33 Changed 2 years ago by git

  • Commit changed from 7bc622bf554be5f6f0f754677ce59f186762f4da to b8c83088cff21aec5c26866435f18bf10d1300f4

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

b8c8308check for amd.h too

comment:34 Changed 2 years ago by dimpase

test run here: https://github.com/dimpase/sage/actions/runs/88161668


New commits:

b8c8308check for amd.h too

New commits:

b8c8308check for amd.h too

comment:35 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:36 Changed 2 years ago by mjo

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 git

  • Commit changed from b8c83088cff21aec5c26866435f18bf10d1300f4 to 45d6d0760d841c2b79794b696eb91f3eb46d3887

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

45d6d07adjust gentoo libs list

comment:38 Changed 2 years ago by mjo

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

comment:40 in reply to: ↑ 39 ; follow-up: Changed 2 years ago by mjo

  • 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 jhpalmieri

Works for me on OS X, for what it's worth.

comment:42 in reply to: ↑ 40 Changed 2 years ago by fbissey

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 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."

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

comment:44 in reply to: ↑ 43 Changed 2 years ago by fbissey

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 git

  • Commit changed from 45d6d0760d841c2b79794b696eb91f3eb46d3887 to e0d812f6786d1846e40257bdc61059514a2c7d1f

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

e0d812fcorrect the use of AC_CHECK_HEADERS

comment:46 Changed 2 years ago by dimpase

  • 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 mjo

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: Changed 2 years ago by dimpase

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 git

  • Commit changed from e0d812f6786d1846e40257bdc61059514a2c7d1f to f2cc5b457f4945d15970bf823f5046d62564522a

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

f2cc5b4added quoting, renamed SAGE_SUITESPARSE_PREFIX to <>_LOCALINSTALL

comment:50 in reply to: ↑ 48 Changed 2 years ago by mjo

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: Changed 2 years ago by dimpase

yes, it is a boolean

comment:52 in reply to: ↑ 51 ; follow-up: Changed 2 years ago by mjo

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 mkoeppe

I would suggest to defer this to 9.2

comment:54 in reply to: ↑ 52 ; follow-up: Changed 2 years ago by mkoeppe

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 dimpase

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 mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:57 Changed 2 years ago by mjo

  • 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 vbraun

  • 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 mkoeppe

  • Commit f2cc5b457f4945d15970bf823f5046d62564522a deleted

Follow-up: #30052 ubuntu-eoan-i386: cvxopt build fails

Note: See TracTickets for help on using tickets.