Opened 12 years ago

Closed 7 years ago

#10286 closed defect (duplicate)

sage-native-execute does not unset path etc.

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

Status badges

Description (last modified by Volker Braun)

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 Volker Braun 12 years ago.
apply to SAGE_LOCAL/bin
trac_10286_call_jmol_correctly.patch (929 bytes) - added by Volker Braun 12 years ago.
Apply to sage library

Download all attachments as: .zip

Change History (19)

Changed 12 years ago by Volker Braun

apply to SAGE_LOCAL/bin

Changed 12 years ago by Volker Braun

Apply to sage library

comment:1 Changed 12 years ago by Volker Braun

Cc: Mike Hansen Nils Bruin tbd added
Description: modified (diff)

The two patches fix #9232 for me.

comment:2 Changed 12 years ago by Leif Leonhardy

Status: newneeds_review

comment:3 Changed 12 years ago by Leif Leonhardy

Keywords: LD_LIBRARY_PATH original save restore sage-env added
Reviewers: 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 12 years ago by Leif Leonhardy

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 12 years ago by Karl-Dieter Crisman

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

comment:6 Changed 10 years ago by Jeroen Demeyer

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

comment:7 in reply to:  6 Changed 10 years ago by Nils Bruin

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 10 years ago by Karl-Dieter Crisman

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

comment:9 Changed 10 years ago by Jeroen Demeyer

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 10 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

In any case, this needs to be rebased.

comment:11 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:12 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:13 Changed 8 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:14 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:15 Changed 8 years ago by Thierry Monteil

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

comment:16 Changed 7 years ago by Nils Bruin

Milestone: sage-6.4sage-duplicate/invalid/wontfix
Status: needs_workpositive_review

comment:17 Changed 7 years ago by Volker Braun

Resolution: duplicate
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.