Opened 19 months ago
Closed 18 months ago
#31584 closed defect (fixed)
Fix suitesparse/cvxopt /usr/local leakage
Reported by: | Matthias Köppe | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-9.3 |
Component: | packages: standard | Keywords: | |
Cc: | John Palmieri, Dima Pasechnik, Michael Orlitzky | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | Michael Orlitzky |
Report Upstream: | N/A | Work issues: | |
Branch: | 52537d1 (Commits, GitHub, GitLab) | Commit: | 52537d120927ed57ce9659683fed15c405b1d1c7 |
Dependencies: | Stopgaps: |
Description (last modified by )
We fix a simple bug in our build system:
The value of SAGE_SUITESPARSE_LOCALINSTALL
is set in build/pkgs/suitesparse/spkg-configure.m4
but is not actually passed on to build/pkgs/cvxopt/spkg-install.in
. A different variable, SAGE_SUITESPARSE_PREFIX
, is passed by build/bin/sage-build-env-config.in
from the configure stage to the build stage, but it is never set or used.
$ git grep SAGE_SUITESPARSE build/bin/sage-build-env-config.in:export SAGE_SUITESPARSE_PREFIX="@SAGE_SUITESPARSE_PREFIX@" build/pkgs/cvxopt/spkg-install.in:if test "x$SAGE_SUITESPARSE_LOCALINSTALL" != "x"; then build/pkgs/suitesparse/spkg-configure.m4: AC_SUBST(SAGE_SUITESPARSE_LOCALINSTALL, ['$SAGE_LOCAL']) build/pkgs/suitesparse/spkg-configure.m4: AC_SUBST(SAGE_SUITESPARSE_LOCALINSTALL, [''])
As a result, suitesparse
from /usr/local
can leak in, as observed in https://trac.sagemath.org/ticket/31567?replyto=25#comment:25
Clearly these two variables were intended to be the same variable. We fix this. This unified variable, SAGE_SUITESPARSE_PREFIX
, is analogous to other variables SAGE_..._PREFIX
.
Change History (17)
comment:1 Changed 19 months ago by
Branch: | → u/mkoeppe/fix_suitesparse_cvxopt__usr_local_leakage |
---|
comment:2 Changed 19 months ago by
Commit: | → 52537d120927ed57ce9659683fed15c405b1d1c7 |
---|
comment:3 Changed 19 months ago by
Authors: | → Matthias Koeppe |
---|---|
Cc: | Michael Orlitzky added |
Status: | new → needs_review |
comment:5 follow-ups: 8 9 Changed 19 months ago by
Reviewers: | → Michael Orlitzky |
---|
Why the variable name change? SAGE_SUITESPARSE_PREFIX
doesn't contain the prefix when suitesparse from the system is being used so this looks wrong (why aren't we passing the prefix unconditionally?) even though it's doing the right thing. I think that's why dima changed the name in the first place in f2cc5b45.
comment:6 follow-up: 10 Changed 19 months ago by
On second thought, the only possible values of SAGE_SUITESPARSE_PREFIX
are $SAGE_LOCAL
and the empty string, so I'm not sure how this fixes the problem.
comment:7 Changed 19 months ago by
Description: | modified (diff) |
---|
I have expanded the ticket description, please take a look
comment:8 Changed 19 months ago by
Replying to mjo:
SAGE_SUITESPARSE_PREFIX
doesn't contain the prefix when suitesparse from the system is being used
That's right, but this matches the semantics of the other SAGE_..._PREFIX
variables; see for example build/pkgs/mpir/spkg-configure.m4
for SAGE_GMP_PREFIX
.
comment:9 Changed 19 months ago by
Replying to mjo:
why aren't we passing the prefix unconditionally?
Because normal compile/link tests do not determine the install prefix.
comment:10 follow-up: 11 Changed 19 months ago by
Replying to mjo:
the only possible values of
SAGE_SUITESPARSE_PREFIX
are$SAGE_LOCAL
and the empty string, so I'm not sure how this fixes the problem.
From build/pkgs/cvxopt/spkg-install.in
:
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
Effectively we only set these variables for cvxopt when we use suitesparse from spkg-configure. Setting these variables disables cvxopt's use of /usr/local
- https://github.com/cvxopt/cvxopt/blob/master/setup.py#L55
We do not need to do this when we use system suitesparse, as we do not want to claim authority that our configure script's suitesparse-finding technique is better than the one in cvxopt's install script.
comment:11 follow-up: 13 Changed 18 months ago by
Replying to mkoeppe:
Effectively we only set these variables for cvxopt when we use suitesparse from spkg-configure. Setting these variables disables cvxopt's use of
/usr/local
- https://github.com/cvxopt/cvxopt/blob/master/setup.py#L55
Isn't it the other way around? From build/pkgs/suitesparse/spkg-configure.m4
,
AS_IF([test x$sage_spkg_install_suitesparse = xyes], [ AC_SUBST(SAGE_SUITESPARSE_PREFIX, ['$SAGE_LOCAL']) ], [ AC_SUBST(SAGE_SUITESPARSE_PREFIX, ['']) ])
As a result, I think this block only gets executed when $SAGE_SUITESPARSE_PREFIX = $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
which is why I don't think that replacing $SAGE_LOCAL
with $SAGE_SUITESPARSE_PREFIX
has changed anything there.
comment:12 Changed 18 months ago by
Sorry, I mistyped: I meant to type "when we use suitesparse from SPKG".
comment:13 Changed 18 months ago by
Replying to mjo:
I don't think that replacing
$SAGE_LOCAL
with$SAGE_SUITESPARSE_PREFIX
has changed anything there.
That's right, this part of the change makes no difference.
comment:15 Changed 18 months ago by
Status: | needs_review → positive_review |
---|
Oh, I see it now. The variable SAGE_SUITESPARSE_PREFIX
appears in build/bin/sage-build-env-config.in
(before the fix), but SAGE_SUITESPARSE_LOCALINSTALL
does not.
comment:17 Changed 18 months ago by
Branch: | u/mkoeppe/fix_suitesparse_cvxopt__usr_local_leakage → 52537d120927ed57ce9659683fed15c405b1d1c7 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch pushed to git repo; I updated commit sha1. New commits:
build/pkgs/cvxopt/spkg-install.in: Actually use the value of SAGE_SUITESPARSE_PREFIX