#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:

Status badges

Description (last modified by Matthias Köppe)

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 Matthias Köppe

Branch: u/mkoeppe/fix_suitesparse_cvxopt__usr_local_leakage

comment:2 Changed 19 months ago by git

Commit: 52537d120927ed57ce9659683fed15c405b1d1c7

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

52537d1build/pkgs/cvxopt/spkg-install.in: Actually use the value of SAGE_SUITESPARSE_PREFIX

comment:3 Changed 19 months ago by Matthias Köppe

Authors: Matthias Koeppe
Cc: Michael Orlitzky added
Status: newneeds_review

comment:4 Changed 19 months ago by Matthias Köppe

Tested in #31567

comment:5 Changed 19 months ago by Michael Orlitzky

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 Changed 19 months ago by Michael Orlitzky

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 Matthias Köppe

Description: modified (diff)

I have expanded the ticket description, please take a look

comment:8 in reply to:  5 Changed 19 months ago by Matthias Köppe

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 in reply to:  5 Changed 19 months ago by Matthias Köppe

Replying to mjo:

why aren't we passing the prefix unconditionally?

Because normal compile/link tests do not determine the install prefix.

comment:10 in reply to:  6 ; Changed 19 months ago by Matthias Köppe

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 in reply to:  10 ; Changed 18 months ago by Michael Orlitzky

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 Matthias Köppe

Sorry, I mistyped: I meant to type "when we use suitesparse from SPKG".

comment:13 in reply to:  11 Changed 18 months ago by Matthias Köppe

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:14 Changed 18 months ago by Matthias Köppe

The bug is fixed by the first commit (7ff26dfad).

comment:15 Changed 18 months ago by Michael Orlitzky

Status: needs_reviewpositive_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:16 Changed 18 months ago by Matthias Köppe

Thanks for reviewing!

comment:17 Changed 18 months ago by Volker Braun

Branch: u/mkoeppe/fix_suitesparse_cvxopt__usr_local_leakage52537d120927ed57ce9659683fed15c405b1d1c7
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.