Opened 9 years ago

Closed 5 years ago

#10286 closed defect (duplicate)

sage-native-execute does not unset path etc.

Reported by: vbraun Owned by: jason
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: misc Keywords: sage-native-execute jmol LD_LIBRARY_PATH original save restore sage-env
Cc: mhansen, nbruin, tbd Merged in:
Authors: Volker Braun Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

The script unsets the LD_LIBRARY_PATH but not the PATH, and then executes the argument. So it essentially breaks execution of all programs that are shipped with sage since they can't find their libraries any more, unless you are lucky and the system libraries have the same version.

3d plots on the Sage command line call "sage-native-execute jmol", which is why 3d plotting in Fedora is broken since forever, see #9232.

The goal of this ticket is to

  1. fix sage-native-execute to revert more of the pre-Sage environment, in particular the PATH.
  2. fix the sage library to not call sage-native-execute for Sage components like jmol.

Related tickets:

  • #9386: sage-native-execute leaves traces of sage

Attachments (2)

trac_10286_fix_sage-native-execute.patch (3.7 KB) - added by vbraun 9 years ago.
apply to SAGE_LOCAL/bin
trac_10286_call_jmol_correctly.patch (929 bytes) - added by vbraun 9 years ago.
Apply to sage library

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by vbraun

apply to SAGE_LOCAL/bin

Changed 9 years ago by vbraun

Apply to sage library

comment:1 Changed 9 years ago by vbraun

  • Cc mhansen nbruin tbd added
  • Description modified (diff)

The two patches fix #9232 for me.

comment:2 Changed 9 years ago by leif

  • Status changed from new to needs_review

comment:3 follow-up: Changed 9 years ago by leif

  • Keywords LD_LIBRARY_PATH original save restore sage-env added
  • Reviewers set to Leif Leonhardy

The patch to the Sage library looks ok, the patches to the scripts less optimal.

Besides that I get eye cancer from [ "x$VAR" = "xVALUE" ], a few comments:

The usage of curly braces around environment variables is inconsistent; I would simply drop them here.

If we really use additional variables just to record if others were already set (which is mostly superfluous), I would either use true or, analogously to others, "yes".

I would also move the definition of UNAME up in sage-env, such that we call uname only once (i.e., use "$UNAME" in the tests).

There are lots of useless export statements:

  • It's weird to export variables that have been unset.
  • We don't have to re-export PATH etc. since they never get unset.
  • Setting variables to other, probably empty or undefined variables (like e.g. SAGE_ORIG_LD_LIBRARY_PATH) is pretty ok, so the tests in sage-native-execute are all superfluous (see above).

Perhaps we should also "save" Sage's modified paths to be able to revert the settings in scripts / programs called with the original settings, e.g. by sage-native-execute. ;-)

sage-native-execute should do exec "$@".

s/can not/cannot/.

I don't know where sage-native-execute gets used, but from Python etc. I would use a Python function that sets up the desired environment for the command to be executed, not call yet another script which in turn runs the program we want to execute. (The C library e.g. contains a family of POSIX functions that wrap the system call execve() to do such.)

With the above changes, sage-native-execute reduces to:

#!/bin/sh

unset PYTHONHOME PYTHONPATH
PATH=$SAGE_ORIG_PATH
LD_LIBRARY_PATH=$SAGE_ORIG_LD_LIBRARY_PATH
DYLD_LIBRARY_PATH=$SAGE_ORIG_DYLD_LIBRARY_PATH # doesn't pollute environment on non-Darwin systems

exec "$@"

or maybe (if we don't want to rely on sage-env having been sourced)

...
test -n "$SAGE_ORIG_PATH" && PATH=$SAGE_ORIG_PATH
# etc., which is equivalent to
PATH=${SAGE_ORIG_PATH:-$PATH}
...

(Or, if desired, we could instead just add the usual "sanity check" if [ -z "$SAGE_LOCAL" ]; then ... exit 1; fi, which also ensures that the original paths had been saved.)

In sage-env, we could also use

: ${SAGE_ORIG_PATH:=$PATH}  # assign if not already set
# etc., and drop the *_SET variables

(which may set SAGE_ORIG_LD_LIBRARY_PATH to Sage's modified, not the original one in case it was empty and sage-env got sourced more than once. But we should IMHO prevent the latter at the top of sage-env by simply returning zero if e.g. SAGE_ENV_SOURCED is non-empty, otherwise defining it to e.g. "yes". This prevents other odd behavior as well.)

I wonder if we shouldn't also save PYTHONPATH and PYTHONHOME in sage-env (and restore them in sage-native-execute as well).

The lines of the commit messages shouldn't exceed 80 columns.

Just my 2 cents.

comment:4 in reply to: ↑ 3 Changed 9 years ago by leif

Replying to leif:

[...] If we really use additional variables just to record if others were already set (which is mostly superfluous), I would either use true or, analogously to others, "yes". [...]
In sage-env, we could also use

: ${SAGE_ORIG_PATH:=$PATH}  # assign if not already set
# etc., and drop the *_SET variables

(which may set SAGE_ORIG_LD_LIBRARY_PATH to Sage's modified, not the original one in case it was empty and sage-env got sourced more than once. But we should IMHO prevent the latter at the top of sage-env by simply returning zero if e.g. SAGE_ENV_SOURCED is non-empty, otherwise defining it to e.g. "yes". This prevents other odd behavior as well.)

I've opened #10469 to address that (sourcing sage-env / saving the original paths only once).

comment:5 Changed 9 years ago by kcrisman

The patch trac_10286_call_jmol_correctly.patch will be used on #9232 instead.

comment:6 follow-up: Changed 7 years ago by jdemeyer

Is this still relevant? Why is sage-native-execute needed anyway?

comment:7 in reply to: ↑ 6 Changed 7 years ago by nbruin

Replying to jdemeyer:

Is this still relevant? Why is sage-native-execute needed anyway?

I think it still is. For instance if you start up magma (in the notebook for instance), it's run via sage-native-execute. If you do anything in the magma startup script that depends on having paths/environment properly set, it won't work. For a while, it meant I couldn't use magma through the notebook (see #9386). I worked around it by restoring the environment in the magma startup script itself.

comment:8 Changed 7 years ago by kcrisman

See for instance #8560, before which Nils' comment wasn't true, I think.

comment:9 Changed 7 years ago by jdemeyer

Fine, so it seems that sage-native-execute does serve a purpose, good to know.

On my system, I just replaced sage-native-execute with a no-op and all doctests passed.

comment:10 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

In any case, this needs to be rebased.

comment:11 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:13 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:14 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:15 Changed 5 years ago by tmonteil

Since the jmol issue was solved elsewhere, isn't this ticket a duplicate of #9386 ?

comment:16 Changed 5 years ago by nbruin

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to positive_review

comment:17 Changed 5 years ago by vbraun

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.