sagemath / sage

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

suitesparse: Update to 7.0.1, fix DESTDIR bug #34206

Open mkoeppe opened 2 years ago

mkoeppe commented 2 years ago

(from #34203 comment:34)

[suitesparse-5.10.1] gcc -std=c99 -L/Users/mkoeppe/s/sage/sage-rebasing/local/lib -Wl,-rpath,/Users/mkoeppe/s/sage/sage-rebasing/local/lib  -L/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/inst/Users/mkoeppe/s/sage/sage-rebasing/local/lib -L/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/inst/Users/mkoeppe/s/sage/sage-rebasing/local/lib -L/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/inst/Users/mkoeppe/s/sage/sage-rebasing/local/lib -L/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/inst/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/src/lib -dynamiclib -compatibility_version 1 -current_version 1.0.2 -Wl,-install_name -Wl,/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/src/lib/libsliplu.1.dylib -shared -undefined dynamic_lookup slip_matrix_div.o slip_create_mpq_array.o SLIP_free.o SLIP_LU_factorize.o SLIP_realloc.o slip_matrix_mul.o slip_get_largest_pivot.o slip_ref_triangular_solve.o SLIP_backslash.o slip_create_mpz_array.o slip_get_nonzero_pivot.o SLIP_LU_solve.o slip_back_sub.o slip_cumsum.o slip_get_pivot.o SLIP_malloc.o slip_sparse_collapse.o SLIP_calloc.o slip_dfs.o slip_get_smallest_pivot.o SLIP_matrix_allocate.o slip_sparse_realloc.o slip_cast_array.o slip_expand_double_array.o SLIP_gmp.o SLIP_matrix_copy.o SLIP_matrix_check.o slip_cast_matrix.o slip_expand_mpfr_array.o SLIP_initialize.o SLIP_matrix_free.o slip_check_solution.o slip_expand_mpq_array.o SLIP_initialize_expert.o SLIP_matrix_nnz.o SLIP_create_default_options.o SLIP_finalize.o SLIP_LU_analysis_free.o slip_permute_x.o slip_permute_b.o slip_create_mpfr_array.o slip_forward_sub.o SLIP_LU_analyze.o slip_reach.o -o /Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/inst/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/src/lib/libsliplu.1.0.2.dylib -lm -rpath /Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/inst/Users/mkoeppe/s/sage/sage-rebasing/local/var/tmp/sage/build/suitesparse-5.10.1/src/lib -lsuitesparseconfig -lamd -lcolamd -lm -lgmp -lmpfr

DESTDIR is used twice in the -o.

DESTDIR handling is coming from build/pkgs/suitesparse/patches/03-buildflags.patch

We upgrade to 5.12.0 and pick up a fresh set of patches from Debian or another distro

Depends on #34746 Depends on #34835

CC: @kiwifb

Component: packages: standard

Author: François Bissey

Branch/Commit: u/mkoeppe/suitesparse6.0.1 @ 424cb49

Issue created by migration from https://trac.sagemath.org/ticket/34206

mkoeppe commented 2 years ago

Branch: u/mkoeppe/suitesparse__fix_destdir_bug

mkoeppe commented 2 years ago

New commits:

bad180abuild/pkgs/suitesparse: Update to 5.12.0
abb0174build/pkgs/suitesparse: Add cmake dependency, activate components that need cmake
mkoeppe commented 2 years ago

Commit: abb0174

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -5,3 +5,8 @@

DESTDIR is used twice in the -o. + +DESTDIR handling is coming from build/pkgs/suitesparse/patches/03-buildflags.patch + +We upgrade to 5.12.0 and pick up a fresh set of patches from Debian or another distro +

mkoeppe commented 2 years ago
comment:3

https://github.com/DrTimothyAldenDavis/SuiteSparse/issues/86 - "build system: Support staged installs via DESTDIR"

kiwifb commented 2 years ago
comment:4

My original spkg was always a quick and dirty job. It shouldn't be a surprise that some bugs appear.

kiwifb commented 1 year ago
comment:7

I guess I should have a stab at this.

kiwifb commented 1 year ago
comment:8

Way too many things are built by default. We can split the package or at least not building the big stuff that we do not need like graphblas and mongoose which are very specialised stuff.

mkoeppe commented 1 year ago
comment:9

I think our optional igraph package uses graphblas

kiwifb commented 1 year ago
comment:10

That's new and graphblas wasn't built before because I had removed all the subpackages using cmake at the time the initial suitesparse was included in sage. But if it can be used, why not.

In other news, DESTDIR works properly in 6.0.1 after some basic testing.

kiwifb commented 1 year ago
comment:11

Cannot see a dependency on GraphBLAS in igraph but if you can point to it, I'll certainly include it.

kiwifb commented 1 year ago
comment:12

I am testing a first draft locally. The changes are big enough that none of the current patches actually apply or are meaningful. I will need your input for any tweaks you require for BLAS/LAPACK on OS X. It should be easy enough to figure out if something is needed.

mkoeppe commented 1 year ago
comment:13

Replying to François Bissey:

Cannot see a dependency on GraphBLAS in igraph but if you can point to it, I'll certainly include it.

Sorry for the noise - looks like I misremembered

kiwifb commented 1 year ago
comment:14

Do we want to build and install static libraries?

mkoeppe commented 1 year ago
comment:15

No, only shared

kiwifb commented 1 year ago

Changed branch from u/mkoeppe/suitesparse__fix_destdir_bug to u/fbissey/suitsparse6.0.1

kiwifb commented 1 year ago

Changed commit from abb0174 to none

kiwifb commented 1 year ago

Author: François Bissey

kiwifb commented 1 year ago
comment:16

Ready for inspection.

kiwifb commented 1 year ago

Changed branch from u/fbissey/suitsparse6.0.1 to trac/u/fbissey/suitsparse6.0.1

kiwifb commented 1 year ago

Changed branch from trac/u/fbissey/suitsparse6.0.1 to u/fbissey/suitesparse6.0.1

kiwifb commented 1 year ago

New commits:

57fc607First draft of upgrade to suitesparse 6.0.1
kiwifb commented 1 year ago

Commit: 57fc607

kiwifb commented 1 year ago
comment:19

OK got the right branch name this time.

mkoeppe commented 1 year ago
comment:20

Builds OK on macOS

mkoeppe commented 1 year ago
comment:21

However it builds against the Accelerate framework, not our openblas:

 otool -L local/lib/libumfpack.dylib                                                                                                         git:t/34206/suitesparse6.0.1
local/lib/libumfpack.dylib:
        @rpath/libumfpack.6.dylib (compatibility version 6.0.0, current version 6.0.1)
        @rpath/libsuitesparseconfig.6.dylib (compatibility version 6.0.0, current version 6.0.1)
        /usr/local/opt/libomp/lib/libomp.dylib (compatibility version 5.0.0, current version 5.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)
        @rpath/libamd.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate (compatibility version 1.0.0, current version 4.0.0)
        @rpath/libcholmod.4.dylib (compatibility version 4.0.0, current version 4.0.1)
kiwifb commented 1 year ago
comment:22

That's what I thought could happen. I think, I can make it go for openblas easily, I found the doc for it, the other day.

This is also a relatively stripped down build like boost_cropped. I probably should add a few more sub-packages if we want to really dignify it with the full "suitesparse" name. And I also should write a few comments.

kiwifb commented 1 year ago
comment:23

And upstream is preparing 6.0.2. But the bump would be trivial on this branch.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from 57fc607 to 8b32e70

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

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

8b32e70Enforce the use of openblas. Add comments.
mkoeppe commented 1 year ago
comment:25

This gets openblas but not the right one on my macOS system.

[suitesparse-6.0.1] -- BLAS libraries:      /opt/local/lib/libopenblas.dylib 
[suitesparse-6.0.1] -- BLAS linker flags:    
[suitesparse-6.0.1] -- LAPACK libraries:    /opt/local/lib/libopenblas.dylib;/opt/local/lib/libopenblas.dylib 
[suitesparse-6.0.1] -- LAPACK linker flags:  
mkoeppe commented 1 year ago
comment:26

The following change fixes it for me:

diff --git a/build/pkgs/suitesparse/spkg-build.in b/build/pkgs/suitesparse/spkg-build.in
index b137a5ecb9..822519b078 100644
--- a/build/pkgs/suitesparse/spkg-build.in
+++ b/build/pkgs/suitesparse/spkg-build.in
@@ -11,7 +11,10 @@ for pkg in ${pkg_list}; do
    # Enforce usage of OpenBlas
    # No static libraries
    # Verbose builds
-   sdh_cmake -DBLA_VENDOR=OpenBLAS -DNSTATIC=ON -DCMAKE_VERBOSE_MAKEFILE=1 ..
+   sdh_cmake -DBLA_VENDOR=OpenBLAS \
+                  -DBLAS_LIBRARIES="$(pkg-config --libs blas)" \
+                  -DLAPACK_LIBRARIES="$(pkg-config --libs lapack)" \
+                  -DNSTATIC=ON -DCMAKE_VERBOSE_MAKEFILE=1 ..
    # Build
    sdh_make
    popd
kiwifb commented 1 year ago
comment:27

Yes, system wide would be picked up first without a hint. Pushing shortly.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

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

b80a672Add hint to locate openblas used by sage
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from 8b32e70 to b80a672

mkoeppe commented 1 year ago
comment:29

Failing on centos-stream-9-python3.9-standard (https://github.com/mkoeppe/sage/actions/runs/3662737597/jobs/6192051075)

CMake Error at CMakeLists.txt:14 (cmake_minimum_required):
  CMake 3.22 or higher is required.  You are running version 3.20.2

This will need an increased lower bound for system cmake - increased from #34746, which set 3.11)

mkoeppe commented 1 year ago

Dependencies: #34746

mkoeppe commented 1 year ago
comment:30

On debian-sid-minimal (https://github.com/mkoeppe/sage/actions/runs/3662737597/jobs/6200588473):

[suitesparse-6.0.1]   /sage/local/var/tmp/sage/build/suitesparse-6.0.1/src/SuiteSparse_config/SuiteSparse_config.h:643:65: error: expected ')' before ',' token
  [suitesparse-6.0.1]     643 |     #define SUITESPARSE_BLAS(name,NAME) SUITESPARSE_FORTRAN(name,NAME)
  [suitesparse-6.0.1]         |                                                                 ^
  [suitesparse-6.0.1]   /sage/local/var/tmp/sage/build/suitesparse-6.0.1/src/SuiteSparse_config/SuiteSparse_config.h:676:37: note: in expansion of macro 'SUITESPARSE_BLAS'
  [suitesparse-6.0.1]     676 | #define SUITESPARSE_LAPACK_ZLARF    SUITESPARSE_BLAS ( zlarf  , ZLAR   )
  [suitesparse-6.0.1]         |                                     ^~~~~~~~~~~~~~~~
  [suitesparse-6.0.1]   /sage/local/var/tmp/sage/build/suitesparse-6.0.1/src/SuiteSparse_config/SuiteSparse_config.h:1434:6: note: in expansion of macro 'SUITESPARSE_LAPACK_ZLARF'
  [suitesparse-6.0.1]    1434 | void SUITESPARSE_LAPACK_ZLARF       // apply Householder reflector
  [suitesparse-6.0.1]         |      ^~~~~~~~~~~~~~~~~~~~~~~~
  [suitesparse-6.0.1]   make[5]: *** [CMakeFiles/cholmod.dir/build.make:1465: CMakeFiles/cholmod.dir/Supernodal/cholmod_super_solve.c.o] Error 1
  [suitesparse-6.0.1]   make[5]: *** [CMakeFiles/cholmod.dir/build.make:1479: CMakeFiles/cholmod.dir/Supernodal/cholmod_super_symbolic.c.o] Error 1
  [suitesparse-6.0.1]   make[5]: Target 'CMakeFiles/cholmod.dir/build' not remade because of errors.
kiwifb commented 1 year ago
comment:31

Can't seem to see the log from that run. What compiler is sid currently using?

mkoeppe commented 1 year ago
comment:32

That's gcc 12.2.0-9.1

mkoeppe commented 1 year ago
comment:33

Same on debian-bookworm-minimal (https://github.com/mkoeppe/sage/actions/runs/3662737597/jobs/6200588437, docker run -it ghcr.io/mkoeppe/sage/sage-docker-debian-bookworm-minimal-with-targets-pre:9.5-107209-g31c5448dc1-failed bash) and on ubuntu-kinetic-minimal and fedora-36-minimal

kiwifb commented 1 year ago
comment:34

I wonder if it has to do with

-- Detecting Fortran/C Interface
Failed to compile

When cmake is run to configure cholmod. It looks like we are trying to mix C and Fortran and it is failing.

kiwifb commented 1 year ago
comment:35

While it is annoying, we could turn off the bits needing blas/lapack which seem to be the one involved here. But really I suspect there is something fishy in the compiler.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

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

7a3f584Merge branch 'develop' into suitesparse6.0.1
552fd83Turn off suprenodal for cholmod to see if c/fortran mixing is causing failures on some debian instances.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from b80a672 to 552fd83

mkoeppe commented 1 year ago
comment:37

tox -e docker-debian-bookworm-minimal-incremental -- suitesparse gives the same errors

kiwifb commented 1 year ago
comment:38

I am running out of ideas on what can cause this. Some compiler setting is the most obvious culprit, but what would it be. Can you get access to the docker instance to fish stuff out of it?

kiwifb commented 1 year ago
comment:39

6.0.2 is just out, so I will bump that and we will see if it helps.

mkoeppe commented 1 year ago
comment:40

I've set up a separate GH Actions test at https://github.com/mkoeppe/SuiteSparse/actions/runs/3672968344

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

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

2e7881dmove suitesparse to 6.0.2