sage-native-execute does not unset path etc.

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

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

CC: @mwhansen @nbruin tbd

Component: misc

Keywords: sage-native-execute jmol LD_LIBRARY_PATH original save restore sage-env

Author: Volker Braun

Reviewer: Leif Leonhardy

Issue created by migration from https://trac.sagemath.org/ticket/10286

[vbraun]

apply to SAGE_LOCAL/bin

[vbraun]

Attachment: trac_10286_fix_sage-native-execute.patch.gz

Attachment: trac_10286_call_jmol_correctly.patch.gz

Apply to sage library

[vbraun]

comment:1

The two patches fix #9232 for me.

[nexttime]

comment:3

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.

[nexttime]

Changed keywords from sage-native-execute, jmol to sage-native-execute jmol LD_LIBRARY_PATH original save restore sage-env

[nexttime]

Reviewer: Leif Leonhardy

[nexttime]

comment:4

Replying to @nexttime:

[...]
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).

[kcrisman]

comment:5

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

[jdemeyer]

comment:6

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

[nbruin]

comment:7

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.

[kcrisman]

comment:8

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

[jdemeyer]

comment:9

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.

[jdemeyer]

comment:10

In any case, this needs to be rebased.

[sagetrac-tmonteil]

comment:15

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