search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.31k stars 451 forks source link

update SuiteSparse to v.7.6, allow its headers to be in suitesparse/ subdir #37192

Closed dimpase closed 1 week ago

dimpase commented 7 months ago

Problem Description

SuiteSparse is now on GitHub: https://github.com/DrTimothyAldenDavis/SuiteSparse and has a new release, 7.6.0. We are still on major version 5. v.7.6.0 is already provided by Homebrew, see #37149 for the related issue.

The SuiteSparse headers have been moved to suitesparse/* - which of course makes perfect sense, but needs to support older versions adds a complication (not unknown to us though)

SuiteSparse now builds with cmake, which is nice.

Proposed Solution

We should upgrade, and allow headers to be in the right place.

Cf as well #37161 and

Is there an existing issue for this?

dimpase commented 7 months ago

@kiwifb @orlitzky @jhpalmieri

kiwifb commented 7 months ago

Yes, I do have ebuild for these in sage-on-gentoo that I will push to Gentoo proper soon. There is a work around for the header, leaving them in the subfolder breaks all the downstreams badly at the moment. Adding -DSUITESPARSE_INCLUDEDIR_POSTFIX="" to the cmake invocation will remove that prefix.

mkoeppe commented 7 months ago

Previous attempt to upgrade: #34206

kiwifb commented 7 months ago

Forgot all about that one. Hopefully, we will not be stopped by broken toolchains this time around.

dimpase commented 7 months ago

Yes, I do have ebuild for these in sage-on-gentoo that I will push to Gentoo proper soon. There is a work around for the header, leaving them in the subfolder breaks all the downstreams badly at the moment. Adding -DSUITESPARSE_INCLUDEDIR_POSTFIX="" to the cmake invocation will remove that prefix.

isn't this "badly" merely the matter of adding -I$prefix/include/suitesparse to CFLAGS ?

Somehow Homebrew was brave enough to keep the prefix.

kiwifb commented 7 months ago

Mostly, but you have to touch all the packages that depend on it. And it turns out that is a surprisingly large number. You have also .pc files that are now included which could be leveraged, but ideally upstream in all those dependencies.

In an ideal coordinated world, all the dependencies should already have adopted a mechanism .pc or .cmake files to get suitesparse configuration (and default to check with default setting when unavailable) and then suitesparse could do the change without impacting anyone.

dimpase commented 7 months ago

Mostly, but you have to touch all the packages that depend on it. And it turns out that is a surprisingly large number. You have also .pc files that are now included which could be leveraged, but ideally upstream in all those dependencies.

In an ideal coordinated world, all the dependencies should already have adopted a mechanism .pc or .cmake files to get suitesparse configuration (and default to check with default setting when unavailable) and then suitesparse could do the change without impacting anyone.

I guess that's the price suitesparse has to pay for building being a mess for far too long.

kiwifb commented 7 months ago

I am going to guess that default folder has been inserted on the request from debian maintainers who have been shipping that way for a while. So, they already have their ducks in a row. My first draft will be without it, but we should test with it as well since it seems spkg-check.m4 explicitly looks for headers the "debian" way.

kiwifb commented 7 months ago

Ok, putting headers in the suitesparse subfolder will break cvxopt build right now. The build system properly work with debian's header because CFLAGS are adjusted for it at configuration time by spkg-check.m4. There is currently one test failure in the CI and it is not suitesparse related.