Opened 3 years ago
Closed 3 years ago
#28832 closed defect (fixed)
suitesparse spkg does not properly use a DESTDIR installation
Reported by:  Erik Bray  Owned by:  

Priority:  critical  Milestone:  sage9.0 
Component:  packages: standard  Keywords:  suitesparse cvxopt 
Cc:  Dima Pasechnik  Merged in:  
Authors:  Erik Bray, François Bissey  Reviewers:  François Bissey, Erik Bray 
Report Upstream:  N/A  Work issues:  
Branch:  efeab2f (Commits, GitHub, GitLab)  Commit:  efeab2f799c7d4d6b2d89e110b9ddaffc77b2712 
Dependencies:  Stopgaps: 
Description
This SPKG was introduced in #22380 as a standard package.
However, it does not properly use the the facilities for DESTDIR installation, and thus a file manifest is not produced for it:
$ cat local/var/lib/sage/installed/suitesparse5.6.0 { "package_name": "suitesparse", "package_version": "5.6.0", "install_date": "Mon Dec 2 11:44:16 UTC 2019", "system_uname": "Linux sagedocker 3.13.0170generic #220Ubuntu SMP Thu May 9 12:40:49 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux", "sage_version": "SageMath version 9.0.beta8, Release Date: 20191201", "test_result": "", "files": [ ] }
suitesparse apparently has a custom build system (parts of it contain sublibraries built with CMake (GraphBLAS, Mongoose) and hence should support DESTDIR
but according to the SPKG.txt
we don't install those components anyways.
We should have better documentation, for adding new SPKGs, about ensuring that DESTDIR install works. I opened #28831 for that.
Change History (24)
comment:1 Changed 3 years ago by
Authors:  → Erik Bray 

Branch:  → u/embray/ticket28832 
Cc:  Dima Pasechnik added 
Commit:  → 19212a8db7f8deec0a864145b802b3aa85099d0c 
Status:  new → needs_review 
comment:2 followup: 3 Changed 3 years ago by
Reviewers:  → François Bissey 

Status:  needs_review → positive_review 
Fair enough, I didn't know it was this important. Otherwise, I would have made the effort. Looks good to me.
comment:3 Changed 3 years ago by
Replying to fbissey:
Fair enough, I didn't know it was this important. Otherwise, I would have made the effort. Looks good to me.
Yeah don't worry, not your fault. I need to do more work to make this clearer. It's not even currently that important for "standard" packages, but is still useful for proper uninstallation/cleanup when updating package versions.
comment:4 Changed 3 years ago by
Status:  positive_review → needs_work 

I'm getting
ranlib libamd.a gcc L/home/release/Sage/local/lib Wl,rpath,/home/release/Sage/local/lib shared Wl,soname Wl,libamd.so.2 Wl,noundefined amd_i_aat.o amd_i_1.o amd_i_2.o amd_i_dump.o amd_i_postorder.o amd_i_defaults.o amd_i_post_tree.o amd_i_order.o amd_i_control.o amd_i_info.o amd_i_valid.o amd_i_preprocess.o amd_l_aat.o amd_l_1.o amd_l_2.o amd_l_dump.o amd_l_postorder.o amd_l_defaults.o amd_l_post_tree.o amd_l_order.o amd_l_control.o amd_l_info.o amd_l_valid.o amd_l_preprocess.o o /home/release/Sage/local/var/tmp/sage/build/suitesparse5.6.0/inst/home/release/Sage/local/lib/libamd.so.2.4.6 lm lrt lsuitesparseconfig /usr/bin/ld: cannot find lsuitesparseconfig collect2: error: ld returned 1 exit status make[6]: *** [Makefile:90: /home/release/Sage/local/var/tmp/sage/build/suitesparse5.6.0/inst/home/release/Sage/local/lib/libamd.so.2.4.6] Error 1 make[6]: Leaving directory '/home/release/Sage/local/var/tmp/sage/build/suitesparse5.6.0/src/AMD/Lib'
on my Ryzen 3800X. Note:
[release@zen Sage]$ find local  grep suitesparseconfig local/var/tmp/sage/build/suitesparse5.6.0/src/SuiteSparse_config/libsuitesparseconfig.a local/var/tmp/sage/build/suitesparse5.6.0/inst/home/release/Sage/local/lib/libsuitesparseconfig.so.5.5.0 local/var/tmp/sage/build/suitesparse5.6.0/inst/home/release/Sage/local/lib/libsuitesparseconfig.so local/var/tmp/sage/build/suitesparse5.6.0/inst/home/release/Sage/local/lib/libsuitesparseconfig.so.5
comment:5 Changed 3 years ago by
Hum, it should have worked. There must be something no behaving in AMD's makefile. Investigating.
comment:6 followup: 20 Changed 3 years ago by
I think I know what's happening. The new patch removes
+ LDFLAGS += L$(INSTALL_LIB)
And later we have
SO_OPTS = $(LDFLAGS)
So we only get sage's LDFLAGS
which doesn't include the path to the temporary installation. I think that part of the patch should be removed.
comment:7 Changed 3 years ago by
Branch:  u/embray/ticket28832 → u/fbissey/ticket28832 

Commit:  19212a8db7f8deec0a864145b802b3aa85099d0c → 5b54ee73c5fbd24e0b4bd19abd610bba202eba85 
Status:  needs_work → needs_review 
New commits:
5b54ee7  Readd some LDFLAGS that are needed in suitesparse

comment:8 Changed 3 years ago by
I'm getting doctesting errors on OS X 10.14.6, for example:
sage t long src/sage/numerical/optimize.py ********************************************************************** File "src/sage/numerical/optimize.py", line 597, in sage.numerical.optimize.linear_program Failed example: sol=linear_program(c,G,h) Exception raised: Traceback (most recent call last): File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 681, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 1123, in compile_and_execute exec(compiled, globs) File "<doctest sage.numerical.optimize.linear_program[3]>", line 1, in <module> sol=linear_program(c,G,h) File "sage/misc/lazy_import.pyx", line 353, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3686) return self.get_object()(*args, **kwds) File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/sage/numerical/optimize.py", line 639, in linear_program sol=solvers.lp(c_,G_,h_,solver=solver) File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/cvxopt/coneprog.py", line 2783, in lp from cvxopt import base, blas, misc File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/cvxopt/misc.py", line 21, in <module> from cvxopt import base, blas, lapack, cholmod, misc_solvers ImportError: dlopen(/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/cvxopt/cholmod.cpython37mdarwin.so, 2): Library not loaded: /Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/libcholmod.3.0.13.dylib Referenced from: /Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage9.0.beta8/local/lib/python3.7/sitepackages/cvxopt/cholmod.cpython37mdarwin.so Reason: image not found
comment:9 Changed 3 years ago by
Is it just with this ticket, or since we introduced suitesparse? Also does cvxopt work and pass its testsuite on the same machine?
comment:10 Changed 3 years ago by
Lack of install_name
on OS X could be an issue but should also be present without this ticket.
comment:11 Changed 3 years ago by
Actually, now I am thinking that install_name
could be automatically set to the location provided by the o
argument in linking. Which, with this ticket will be prefixed by DESTDIR
and that would be an issue. And now that I tried to ask you some output of otool L
I realise that the prefix is present in what the loader asks. So this is the problem. I will fix it ASAP.
comment:12 Changed 3 years ago by
Commit:  5b54ee73c5fbd24e0b4bd19abd610bba202eba85 → 8d795908b343506192a5f9cdcaab76a2a7624180 

Branch pushed to git repo; I updated commit sha1. New commits:
8d79590  fix install_name on OS X

comment:14 Changed 3 years ago by
I'm using a different machine (OS X 10.15.1) but same result. cvxopt
fails its test suite, too: same problem. With plain Sage 9.0.beta8, all tests pass and cvxopt
passes its test suite.
comment:15 Changed 3 years ago by
Even with my updated branch? What does otool L local/lib/libcholmod.3.0.13.dylib
report?
comment:16 Changed 3 years ago by
Yes, this with the updated branch; the latest entry in git log
is
commit 8d795908b343506192a5f9cdcaab76a2a7624180 (HEAD > t/28832/ticket28832, trac/u/fbissey/ticket28832) Author: François Bissey <frp.bissey@gmail.com> Date: Fri Dec 6 14:10:15 2019 +1300 fix install_name on OS X
I unpacked a tar file, ran git trac config
, git trac checkout 28832
, and then make ptestlong
.
The paths are broken — they point to the temporary build directory:
% otool L local/lib/libcholmod.3.0.13.dylib local/lib/libcholmod.3.0.13.dylib: /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libcholmod.3.dylib (compatibility version 3.0.0, current version 3.0.13) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0) /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libamd.2.dylib (compatibility version 2.0.0, current version 2.4.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libcolamd.2.dylib (compatibility version 2.0.0, current version 2.9.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libsuitesparseconfig.5.dylib (compatibility version 5.0.0, current version 5.5.0) /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libccolamd.2.dylib (compatibility version 2.0.0, current version 2.9.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/var/tmp/sage/build/suitesparse5.6.0/inst/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libcamd.2.dylib (compatibility version 2.0.0, current version 2.4.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.0.beta8/local/lib/libopenblas_haswellpr0.3.6.dylib (compatibility version 0.0.0, current version 0.0.0)
In comparison, with plain 9.0.beta8:
% otool L local/lib/libcholmod.3.0.13.dylib local/lib/libcholmod.3.0.13.dylib: /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libcholmod.3.0.13.dylib (compatibility version 3.0.0, current version 3.0.13) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0) /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libamd.2.4.6.dylib (compatibility version 2.0.0, current version 2.4.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libcolamd.2.9.6.dylib (compatibility version 2.0.0, current version 2.9.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libsuitesparseconfig.5.5.0.dylib (compatibility version 5.0.0, current version 5.5.0) /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libccolamd.2.9.6.dylib (compatibility version 2.0.0, current version 2.9.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libcamd.2.4.6.dylib (compatibility version 2.0.0, current version 2.4.6) /Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage9.0.beta8/local/lib/libopenblas_haswellpr0.3.6.dylib (compatibility version 0.0.0, current version 0.0.0)
comment:18 Changed 3 years ago by
Commit:  8d795908b343506192a5f9cdcaab76a2a7624180 → efeab2f799c7d4d6b2d89e110b9ddaffc77b2712 

Branch pushed to git repo; I updated commit sha1. New commits:
efeab2f  correct the final path in install_name

comment:20 Changed 3 years ago by
Replying to fbissey:
I think I know what's happening. The new patch removes
+ LDFLAGS += L$(INSTALL_LIB)And later we have
SO_OPTS = $(LDFLAGS)So we only get sage's
LDFLAGS
which doesn't include the path to the temporary installation. I think that part of the patch should be removed.
You're right, of course. I made the same update to the patch in #28829, but I really should have fixed the version here. I'll have to rebase #28829 on this one if you think it's ready.
comment:21 Changed 3 years ago by
Thank you for adding the OSX stuffI should've caught that. Positive review from me pending review from John.
comment:23 Changed 3 years ago by
Authors:  Erik Bray → Erik Bray, François Bissey 

Reviewers:  François Bissey → François Bissey, Erik Bray 
comment:24 Changed 3 years ago by
Branch:  u/fbissey/ticket28832 → efeab2f799c7d4d6b2d89e110b9ddaffc77b2712 

Resolution:  → fixed 
Status:  positive_review → closed 
This fixes it:
I also confirmed that none of the installed files contain the DESTDIR temporary path, which is generally required for a proper staged install.
New commits:
Trac #28832: Support staged installation of suitesparse using DESTDIR.