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 )
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
- fix
sage-native-execute
to revert more of the pre-Sage environment, in particular the PATH. - 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)
Change History (19)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Cc mhansen nbruin tbd added
- Description modified (diff)
The two patches fix #9232 for me.
comment:2 Changed 9 years ago by
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 9 years ago by
- 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 insage-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
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"
. [...]
Insage-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 andsage-env
got sourced more than once. But we should IMHO prevent the latter at the top ofsage-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
The patch trac_10286_call_jmol_correctly.patch will be used on #9232 instead.
comment:6 follow-up: ↓ 7 Changed 7 years ago by
Is this still relevant? Why is sage-native-execute
needed anyway?
comment:7 in reply to: ↑ 6 Changed 7 years ago by
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
See for instance #8560, before which Nils' comment wasn't true, I think.
comment:9 Changed 7 years ago by
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
- Status changed from needs_review to needs_work
In any case, this needs to be rebased.
comment:11 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:12 Changed 6 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:13 Changed 6 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:14 Changed 5 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:15 Changed 5 years ago by
Since the jmol issue was solved elsewhere, isn't this ticket a duplicate of #9386 ?
comment:16 Changed 5 years ago by
- 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
- Resolution set to duplicate
- Status changed from positive_review to closed
apply to SAGE_LOCAL/bin