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

Priority:  critical  Milestone:  sage9.0 
Component:  packages: standard  Keywords:  suitesparse cvxopt 
Cc:  dimpase  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 2 years ago by
 Branch set to u/embray/ticket28832
 Cc dimpase added
 Commit set to 19212a8db7f8deec0a864145b802b3aa85099d0c
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 2 years ago by
 Reviewers set to François Bissey
 Status changed from needs_review to 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 in reply to: ↑ 2 Changed 2 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 2 years ago by
 Status changed from positive_review to 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 2 years ago by
Hum, it should have worked. There must be something no behaving in AMD's makefile. Investigating.
comment:6 followup: ↓ 20 Changed 2 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 2 years ago by
 Branch changed from u/embray/ticket28832 to u/fbissey/ticket28832
 Commit changed from 19212a8db7f8deec0a864145b802b3aa85099d0c to 5b54ee73c5fbd24e0b4bd19abd610bba202eba85
 Status changed from needs_work to needs_review
New commits:
5b54ee7  Readd some LDFLAGS that are needed in suitesparse

comment:8 Changed 2 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 2 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 2 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 2 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 2 years ago by
 Commit changed from 5b54ee73c5fbd24e0b4bd19abd610bba202eba85 to 8d795908b343506192a5f9cdcaab76a2a7624180
Branch pushed to git repo; I updated commit sha1. New commits:
8d79590  fix install_name on OS X

comment:13 Changed 2 years ago by
Try it now.
comment:14 Changed 2 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 2 years ago by
Even with my updated branch? What does otool L local/lib/libcholmod.3.0.13.dylib
report?
comment:16 Changed 2 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:17 Changed 2 years ago by
I am officially stupid. Correcting...
comment:18 Changed 2 years ago by
 Commit changed from 8d795908b343506192a5f9cdcaab76a2a7624180 to efeab2f799c7d4d6b2d89e110b9ddaffc77b2712
Branch pushed to git repo; I updated commit sha1. New commits:
efeab2f  correct the final path in install_name

comment:19 Changed 2 years ago by
Should be better now.
comment:20 in reply to: ↑ 6 Changed 2 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 2 years ago by
Thank you for adding the OSX stuffI should've caught that. Positive review from me pending review from John.
comment:22 Changed 2 years ago by
 Status changed from needs_review to positive_review
This version works for me.
comment:23 Changed 2 years ago by
 Reviewers changed from François Bissey to François Bissey, Erik Bray
comment:24 Changed 2 years ago by
 Branch changed from u/fbissey/ticket28832 to efeab2f799c7d4d6b2d89e110b9ddaffc77b2712
 Resolution set to fixed
 Status changed from positive_review to 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.