Sage: Ticket #10286: sage-native-execute does not unset path etc.
https://trac.sagemath.org/ticket/10286
<p>
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.
</p>
<p>
3d plots on the Sage command line call "sage-native-execute jmol",
which is why 3d plotting in Fedora is broken since forever, see <a class="closed ticket" href="https://trac.sagemath.org/ticket/9232" title="#9232: defect: jmol on commandline broken (closed: fixed)">#9232</a>.
</p>
<p>
The goal of this ticket is to
</p>
<ol><li>fix <code>sage-native-execute</code> to revert more of the pre-Sage environment, in particular the PATH.
</li><li>fix the sage library to not call <code>sage-native-execute</code> for Sage components like jmol.
</li></ol><p>
Related tickets:
</p>
<ul><li><a class="needs_work ticket" href="https://trac.sagemath.org/ticket/9386" title="#9386: defect: sage-native-execute leaves traces of sage (needs_work)">#9386</a>: sage-native-execute leaves traces of sage
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10286
Trac 1.2Volker BraunWed, 17 Nov 2010 22:10:53 GMTattachment set
https://trac.sagemath.org/ticket/10286
https://trac.sagemath.org/ticket/10286
<ul>
<li><strong>attachment</strong>
set to <em>trac_10286_fix_sage-native-execute.patch</em>
</li>
</ul>
<p>
apply to SAGE_LOCAL/bin
</p>
TicketVolker BraunWed, 17 Nov 2010 22:15:11 GMTattachment set
https://trac.sagemath.org/ticket/10286
https://trac.sagemath.org/ticket/10286
<ul>
<li><strong>attachment</strong>
set to <em>trac_10286_call_jmol_correctly.patch</em>
</li>
</ul>
<p>
Apply to sage library
</p>
TicketVolker BraunWed, 17 Nov 2010 22:19:09 GMTdescription changed; cc set
https://trac.sagemath.org/ticket/10286#comment:1
https://trac.sagemath.org/ticket/10286#comment:1
<ul>
<li><strong>cc</strong>
<em>Mike Hansen</em> <em>Nils Bruin</em> <em>tbd</em> added
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10286?action=diff&version=1">diff</a>)
</li>
</ul>
<p>
The two patches fix <a class="closed ticket" href="https://trac.sagemath.org/ticket/9232" title="#9232: defect: jmol on commandline broken (closed: fixed)">#9232</a> for me.
</p>
TicketLeif LeonhardyMon, 13 Dec 2010 11:41:36 GMTstatus changed
https://trac.sagemath.org/ticket/10286#comment:2
https://trac.sagemath.org/ticket/10286#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketLeif LeonhardyMon, 13 Dec 2010 14:18:07 GMTkeywords changed; reviewer set
https://trac.sagemath.org/ticket/10286#comment:3
https://trac.sagemath.org/ticket/10286#comment:3
<ul>
<li><strong>keywords</strong>
<em>LD_LIBRARY_PATH</em> <em>original</em> <em>save</em> <em>restore</em> <em>sage-env</em> added
</li>
<li><strong>reviewer</strong>
set to <em>Leif Leonhardy</em>
</li>
</ul>
<p>
The patch to the Sage library looks ok, the patches to the scripts less optimal.
</p>
<p>
Besides that I get eye cancer from <code>[ "x$VAR" = "xVALUE" ]</code>, a few comments:
</p>
<p>
The usage of curly braces around environment variables is inconsistent; I would simply drop them here.
</p>
<p>
If we really use additional variables just to record if others were already set (which is mostly superfluous), I would either use <code>true</code> or, analogously to others, <code>"yes"</code>.
</p>
<p>
I would also move the definition of <code>UNAME</code> up in <code>sage-env</code>, such that we call <code>uname</code> only once (i.e., use <code>"$UNAME"</code> in the tests).
</p>
<p>
There are lots of useless <code>export</code> statements:
</p>
<ul><li>It's weird to export variables that have been <code>unset</code>.
</li><li>We don't have to re-export <code>PATH</code> etc. since they never get unset.
</li><li>Setting variables to other, probably empty or undefined variables (like e.g. <code>SAGE_ORIG_LD_LIBRARY_PATH</code>) is pretty ok, so the tests in <code>sage-native-execute</code> are all superfluous (see above).
</li></ul><p>
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 <code>sage-native-execute</code>. ;-)
</p>
<p>
<code>sage-native-execute</code> should do <code>exec "$@"</code>.
</p>
<p>
s/can not/cannot/.
</p>
<p>
I don't know where <code>sage-native-execute</code> 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 <code>execve()</code> to do such.)
</p>
<p>
With the above changes, <code>sage-native-execute</code> reduces to:
</p>
<div class="wiki-code"><div class="code"><pre><span class="ch">#!/bin/sh
</span>
<span class="nb">unset</span> PYTHONHOME PYTHONPATH
<span class="nv">PATH</span><span class="o">=</span><span class="nv">$SAGE_ORIG_PATH</span>
<span class="nv">LD_LIBRARY_PATH</span><span class="o">=</span><span class="nv">$SAGE_ORIG_LD_LIBRARY_PATH</span>
<span class="nv">DYLD_LIBRARY_PATH</span><span class="o">=</span><span class="nv">$SAGE_ORIG_DYLD_LIBRARY_PATH</span> <span class="c1"># doesn't pollute environment on non-Darwin systems
</span>
<span class="nb">exec</span> <span class="s2">"</span><span class="nv">$@</span><span class="s2">"</span>
</pre></div></div><p>
or maybe (if we don't want to rely on <code>sage-env</code> having been sourced)
</p>
<div class="wiki-code"><div class="code"><pre>...
<span class="nb">test</span> -n <span class="s2">"</span><span class="nv">$SAGE_ORIG_PATH</span><span class="s2">"</span> <span class="o">&&</span> <span class="nv">PATH</span><span class="o">=</span><span class="nv">$SAGE_ORIG_PATH</span>
<span class="c1"># etc., which is equivalent to
</span><span class="nv">PATH</span><span class="o">=</span><span class="si">${</span><span class="nv">SAGE_ORIG_PATH</span><span class="k">:-</span><span class="nv">$PATH</span><span class="si">}</span>
...
</pre></div></div><p>
(Or, if desired, we could instead just add the usual "sanity check" <code>if [ -z "$SAGE_LOCAL" ]; then ... exit 1; fi</code>, which also ensures that the original paths had been saved.)
</p>
<p>
In <code>sage-env</code>, we could also use
</p>
<div class="wiki-code"><div class="code"><pre>: <span class="si">${</span><span class="nv">SAGE_ORIG_PATH</span><span class="p">:=</span><span class="nv">$PATH</span><span class="si">}</span> <span class="c1"># assign if not already set
# etc., and drop the *_SET variables
</span></pre></div></div><p>
(which may set <code>SAGE_ORIG_LD_LIBRARY_PATH</code> to Sage's modified, not the original one in case it was empty and <code>sage-env</code> got sourced more than once. But we should IMHO prevent the latter at the top of <code>sage-env</code> by simply returning zero if e.g. <code>SAGE_ENV_SOURCED</code> is non-empty, otherwise defining it to e.g. <code>"yes"</code>. This prevents other odd behavior as well.)
</p>
<p>
I wonder if we shouldn't also save <code>PYTHONPATH</code> and <code>PYTHONHOME</code> in <code>sage-env</code> (and restore them in <code>sage-native-execute</code> as well).
</p>
<p>
The lines of the commit messages shouldn't exceed 80 columns.
</p>
<p>
Just my 2 cents.
</p>
TicketLeif LeonhardyMon, 13 Dec 2010 18:40:41 GMT
https://trac.sagemath.org/ticket/10286#comment:4
https://trac.sagemath.org/ticket/10286#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10286#comment:3" title="Comment 3">leif</a>:
</p>
<blockquote class="citation">
<p>
[...]
If we really use additional variables just to record if others were already set (which is mostly superfluous), I would either use <code>true</code> or, analogously to others, <code>"yes"</code>.
[...] <br />
In <code>sage-env</code>, we could also use
</p>
</blockquote>
<div class="wiki-code"><div class="code"><pre>: <span class="si">${</span><span class="nv">SAGE_ORIG_PATH</span><span class="p">:=</span><span class="nv">$PATH</span><span class="si">}</span> <span class="c1"># assign if not already set
# etc., and drop the *_SET variables
</span></pre></div></div><blockquote class="citation">
<p>
(which may set <code>SAGE_ORIG_LD_LIBRARY_PATH</code> to Sage's modified, not the original one in case it was empty and <code>sage-env</code> got sourced more than once. But we should IMHO prevent the latter at the top of <code>sage-env</code> by simply returning zero if e.g. <code>SAGE_ENV_SOURCED</code> is non-empty, otherwise defining it to e.g. <code>"yes"</code>. This prevents other odd behavior as well.)
</p>
</blockquote>
<p>
I've opened <a class="closed ticket" href="https://trac.sagemath.org/ticket/10469" title="#10469: defect: Don't (effectively) source sage-env more than once (closed: fixed)">#10469</a> to address that (sourcing <code>sage-env</code> / saving the original paths only once).
</p>
TicketKarl-Dieter CrismanFri, 25 Mar 2011 18:46:40 GMT
https://trac.sagemath.org/ticket/10286#comment:5
https://trac.sagemath.org/ticket/10286#comment:5
<p>
The patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10286/trac_10286_call_jmol_correctly.patch" title="Attachment 'trac_10286_call_jmol_correctly.patch' in Ticket #10286">trac_10286_call_jmol_correctly.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10286/trac_10286_call_jmol_correctly.patch" title="Download"></a> will be used on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9232" title="#9232: defect: jmol on commandline broken (closed: fixed)">#9232</a> instead.
</p>
TicketJeroen DemeyerWed, 29 Aug 2012 18:22:01 GMT
https://trac.sagemath.org/ticket/10286#comment:6
https://trac.sagemath.org/ticket/10286#comment:6
<p>
Is this still relevant? Why is <code>sage-native-execute</code> needed anyway?
</p>
TicketNils BruinWed, 29 Aug 2012 18:34:10 GMT
https://trac.sagemath.org/ticket/10286#comment:7
https://trac.sagemath.org/ticket/10286#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10286#comment:6" title="Comment 6">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Is this still relevant? Why is <code>sage-native-execute</code> needed anyway?
</p>
</blockquote>
<p>
I think it still is. For instance if you start up magma (in the notebook for instance), it's run via <code>sage-native-execute</code>. 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 <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/9386" title="#9386: defect: sage-native-execute leaves traces of sage (needs_work)">#9386</a>). I worked around it by restoring the environment in the magma startup script itself.
</p>
TicketKarl-Dieter CrismanWed, 29 Aug 2012 18:35:15 GMT
https://trac.sagemath.org/ticket/10286#comment:8
https://trac.sagemath.org/ticket/10286#comment:8
<p>
See for instance <a class="closed ticket" href="https://trac.sagemath.org/ticket/8560" title="#8560: defect: magma interface should be changed to use sage-native-execute (closed: fixed)">#8560</a>, before which Nils' comment wasn't true, I think.
</p>
TicketJeroen DemeyerWed, 29 Aug 2012 18:42:20 GMT
https://trac.sagemath.org/ticket/10286#comment:9
https://trac.sagemath.org/ticket/10286#comment:9
<p>
Fine, so it seems that <code>sage-native-execute</code> does serve a purpose, good to know.
</p>
<p>
On my system, I just replaced <code>sage-native-execute</code> with a no-op and all doctests passed.
</p>
TicketJeroen DemeyerFri, 21 Sep 2012 14:48:12 GMTstatus changed
https://trac.sagemath.org/ticket/10286#comment:10
https://trac.sagemath.org/ticket/10286#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
In any case, this needs to be rebased.
</p>
TicketJeroen DemeyerTue, 13 Aug 2013 15:35:53 GMTmilestone changed
https://trac.sagemath.org/ticket/10286#comment:11
https://trac.sagemath.org/ticket/10286#comment:11
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
TicketFor batch modificationsThu, 30 Jan 2014 21:20:52 GMTmilestone changed
https://trac.sagemath.org/ticket/10286#comment:12
https://trac.sagemath.org/ticket/10286#comment:12
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.1</em> to <em>sage-6.2</em>
</li>
</ul>
TicketFor batch modificationsTue, 06 May 2014 15:20:58 GMTmilestone changed
https://trac.sagemath.org/ticket/10286#comment:13
https://trac.sagemath.org/ticket/10286#comment:13
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.2</em> to <em>sage-6.3</em>
</li>
</ul>
TicketFor batch modificationsSun, 10 Aug 2014 16:51:03 GMTmilestone changed
https://trac.sagemath.org/ticket/10286#comment:14
https://trac.sagemath.org/ticket/10286#comment:14
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.3</em> to <em>sage-6.4</em>
</li>
</ul>
TicketThierry MonteilSun, 08 Feb 2015 17:44:09 GMT
https://trac.sagemath.org/ticket/10286#comment:15
https://trac.sagemath.org/ticket/10286#comment:15
<p>
Since the jmol issue was solved elsewhere, isn't this ticket a duplicate of <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/9386" title="#9386: defect: sage-native-execute leaves traces of sage (needs_work)">#9386</a> ?
</p>
TicketNils BruinThu, 07 May 2015 17:06:46 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/10286#comment:16
https://trac.sagemath.org/ticket/10286#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-6.4</em> to <em>sage-duplicate/invalid/wontfix</em>
</li>
</ul>
TicketVolker BraunTue, 19 May 2015 06:43:26 GMTstatus changed; resolution set
https://trac.sagemath.org/ticket/10286#comment:17
https://trac.sagemath.org/ticket/10286#comment:17
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>duplicate</em>
</li>
</ul>
Ticket