#10286: sage-native-execute does not unset path etc.
------------------------------+---------------------------------------------
Reporter: vbraun | Owner: jason
Type: defect | Status: needs_review
Priority: major | Milestone: sage-4.6.1
Component: misc | Keywords: sage-native-execute jmol
LD_LIBRARY_PATH original save restore sage-env
Author: Volker Braun | Upstream: N/A
Reviewer: Leif Leonhardy | Merged:
Work_issues: |
------------------------------+---------------------------------------------
Changes (by leif):
* keywords: sage-native-execute, jmol => sage-native-execute jmol
LD_LIBRARY_PATH original save restore sage-
env
* reviewer: => Leif Leonhardy
Comment:
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:
{{{
#!sh
#!/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)
{{{
#!sh
...
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
{{{
#!sh
: ${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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/10286#comment:3>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
