Sage: Ticket #4029: sage-env kills the shell when called from "wrong" directory
https://trac.sagemath.org/ticket/4029
<p>
Sourcing sage-env from any directory other than SAGE_ROOT or SAGE_LOCAL/bin kills the shell. This is considerably disconcerting behaviour. An explanatory message would be nice (if not a proper fix).
</p>
<hr />
<p>
Once <a class="closed ticket" href="https://trac.sagemath.org/ticket/9960" title="defect: require SAGE_CHECK to be "yes" (closed: fixed)">#9960</a> is merged, this can be closed. Don't merge any of the patches here!
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/4029
Trac 1.1.6mabshoffWed, 17 Sep 2008 11:55:02 GMTmilestone set
https://trac.sagemath.org/ticket/4029#comment:1
https://trac.sagemath.org/ticket/4029#comment:1
<ul>
<li><strong>milestone</strong>
set to <em>sage-3.1.3</em>
</li>
</ul>
<p>
Oops, no milestone
</p>
TicketwasSat, 24 Jan 2009 12:46:35 GMTsummary changed
https://trac.sagemath.org/ticket/4029#comment:2
https://trac.sagemath.org/ticket/4029#comment:2
<ul>
<li><strong>summary</strong>
changed from <em>sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; needs rievew] sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
TicketwasSat, 24 Jan 2009 12:46:40 GMTmilestone changed
https://trac.sagemath.org/ticket/4029#comment:3
https://trac.sagemath.org/ticket/4029#comment:3
<ul>
<li><strong>milestone</strong>
changed from <em>sage-3.4.1</em> to <em>sage-3.3</em>
</li>
</ul>
TicketmabshoffSat, 24 Jan 2009 12:52:56 GMTsummary changed
https://trac.sagemath.org/ticket/4029#comment:4
https://trac.sagemath.org/ticket/4029#comment:4
<ul>
<li><strong>summary</strong>
changed from <em>[with patch; needs rievew] sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; positive review] sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
<p>
There is a typo in the commit message, but otherwise this is the correct fix. I initially also wanted to do the indenting, but this is a much cleaner and better solution.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketmabshoffSat, 24 Jan 2009 13:59:42 GMTsummary changed
https://trac.sagemath.org/ticket/4029#comment:5
https://trac.sagemath.org/ticket/4029#comment:5
<ul>
<li><strong>summary</strong>
changed from <em>[with patch; positive review] sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; positive review, needs rebase] sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
<p>
The patch does not apply to my merged tree. I believe <a class="closed ticket" href="https://trac.sagemath.org/ticket/22" title="enhancement: [with patch, positive review] add persistent SAGE64 building switch on ... (closed: fixed)">#22</a> collides with this, but it should be easy to rebase post 3.3.alpha2.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketmabshoffWed, 25 Mar 2009 07:18:59 GMTsummary changed
https://trac.sagemath.org/ticket/4029#comment:6
https://trac.sagemath.org/ticket/4029#comment:6
<ul>
<li><strong>summary</strong>
changed from <em>[with patch; positive review, needs rebase] sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; needs rebase] sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
<p>
The rebase needs to make sure that the changes from <a class="closed ticket" href="https://trac.sagemath.org/ticket/22" title="enhancement: [with patch, positive review] add persistent SAGE64 building switch on ... (closed: fixed)">#22</a> don't get lost :)
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketmhansenMon, 05 Oct 2009 07:18:06 GMTattachment set
https://trac.sagemath.org/ticket/4029
https://trac.sagemath.org/ticket/4029
<ul>
<li><strong>attachment</strong>
set to <em>trac_4029.patch</em>
</li>
</ul>
TicketmhansenMon, 05 Oct 2009 07:19:06 GMTsummary changed; author set
https://trac.sagemath.org/ticket/4029#comment:7
https://trac.sagemath.org/ticket/4029#comment:7
<ul>
<li><strong>summary</strong>
changed from <em>[with patch; needs rebase] sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; needs review] sage-env kills the shell when called from "wrong" directory</em>
</li>
<li><strong>author</strong>
set to <em>Mike Hansen</em>
</li>
</ul>
<p>
I've attached a new patch which implements a cleaner solution using "$(exit 1)". See <a class="ext-link" href="http://fvue.nl/wiki/Bash:_Return_or_exit%3F"><span class="icon"></span>http://fvue.nl/wiki/Bash:_Return_or_exit%3F</a>
</p>
TicketwasMon, 05 Oct 2009 13:22:32 GMTsummary changed
https://trac.sagemath.org/ticket/4029#comment:8
https://trac.sagemath.org/ticket/4029#comment:8
<ul>
<li><strong>summary</strong>
changed from <em>[with patch; needs review] sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; positive review] sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
<p>
merged in sage-4.1.2.rc1....
</p>
TicketwasMon, 05 Oct 2009 13:23:18 GMTmilestone changed
https://trac.sagemath.org/ticket/4029#comment:9
https://trac.sagemath.org/ticket/4029#comment:9
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.1.3</em> to <em>sage-4.1.2</em>
</li>
</ul>
TicketwasMon, 05 Oct 2009 13:31:45 GMTsummary changed
https://trac.sagemath.org/ticket/4029#comment:10
https://trac.sagemath.org/ticket/4029#comment:10
<ul>
<li><strong>summary</strong>
changed from <em>[with patch; positive review] sage-env kills the shell when called from "wrong" directory</em> to <em>[with patch; needs work] sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
<p>
Reopened because wjp pointed out a serious issue -- the rest of the script gets executed.
</p>
<p>
Some irc discussion about this:
</p>
<pre class="wiki">williamstein: Regarding sage-env, I redid a patch of yours at #4029.
[06:15am] williamstein:
Wow, what does "$(exit 1)" mean?
[06:15am] williamstein:
ah, you give a reference.
[06:16am] williamstein:
I still don't understand it.
[06:18am] williamstein:
Mike could you change the patch itself to include a comment about what $(exit 1) means? Or maybe just a link to that page in
[06:18am] williamstein:
a
[06:18am] williamstein:
comment?
[06:18am] williamstein:
Because googling for "$(exit 1)" would be tricky.
[06:18am] williamstein:
$(exit 1)
[06:18am] williamstein:
actually, it is easy to google for.
[06:18am] williamstein:
No, it isn't.
[06:20am] williamstein:
well if it works, it works, I guess.
[06:21am] mhansen:
Yeah, I don't quite understand it myself.
[06:22am] williamstein:
Well, it works perfecty.
[06:22am] williamstein:
so positive review.
[06:23am] williamstein:
And the current behavior in Sage (without the patch) is indeed "disconcerting".
[06:24am] mhansen:
I'm surprised that one page was the only page that I could find something about it.
[06:24am] williamstein:
Indeed.
[06:25am] williamstein:
It's hard to google though.
[06:27am] wjp:
isn't this trick only for passing a return status? I don't think it actually stops the script that's being sourced, right?
[06:28am] williamstein:
I don't know. But if you try it, for some reason it works.
[06:28am] wjp:
hm, strange. Did you also test the remainder of sage-env doesn't get executed?
[06:28am] williamstein:
wjp -- you seam like the type to pull open the bash source code, read it, and completely understand what $(exit 1) does :-)
[06:29am] wjp:
I thought $(...) was the same as `...`
[06:29am] wjp:
(yes, I am kind of that type :-) )
[06:29am] williamstein:
crap, in fact, the remainder does get executed.
[06:30am] williamstein:
hey mhansen -- what did my original patch do?
[06:30am] williamstein:
since I think you deleted it.
[06:30am] mhansen:
Ripped the bottom half of sage-env out and put it in a different file.
[06:30am] mhansen:
By bottom half, I meant the part below that line.
[06:31am] williamstein:
oh, now I remember doing that.
[06:31am] williamstein:
"When in doubt, refactor it out!"
</pre>
TicketiandrusWed, 29 Dec 2010 20:30:15 GMTattachment set
https://trac.sagemath.org/ticket/4029
https://trac.sagemath.org/ticket/4029
<ul>
<li><strong>attachment</strong>
set to <em>trac_4029.2.patch</em>
</li>
</ul>
TicketiandrusWed, 29 Dec 2010 20:31:59 GMTstatus changed; cc, upstream set
https://trac.sagemath.org/ticket/4029#comment:11
https://trac.sagemath.org/ticket/4029#comment:11
<ul>
<li><strong>cc</strong>
<em>iandrus</em> added
</li>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>upstream</strong>
set to <em>N/A</em>
</li>
</ul>
<p>
I used the very hacky (but effective)
</p>
<pre class="wiki">return 1 2>/dev/null || exit 1
</pre><p>
instead of <code>return</code> or <code>exit</code>.
</p>
TicketiandrusThu, 24 Mar 2011 00:08:44 GMT
https://trac.sagemath.org/ticket/4029#comment:12
https://trac.sagemath.org/ticket/4029#comment:12
<p>
Will be obsolete with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10469" title="defect: Don't (effectively) source sage-env more than once (closed: fixed)">#10469</a>.
</p>
TicketjhpalmieriThu, 24 Mar 2011 21:00:47 GMT
https://trac.sagemath.org/ticket/4029#comment:13
https://trac.sagemath.org/ticket/4029#comment:13
<p>
I don't think this will be obsolete with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10469" title="defect: Don't (effectively) source sage-env more than once (closed: fixed)">#10469</a>: after applying that patch, if I open up a new shell and source sage-env, the shell closes. Isn't the issue using "exit" instead of "return", since the script is being sourced? I'm not a shell script expert, but can we just replace "exit 1" with "return 1" everywhere? Failing that, can you explain your hack
</p>
<pre class="wiki">return 1 2>/dev/null || exit 1
</pre><p>
This seems to work for me.
</p>
TicketiandrusThu, 24 Mar 2011 23:31:55 GMTstatus changed
https://trac.sagemath.org/ticket/4029#comment:14
https://trac.sagemath.org/ticket/4029#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
You're right this won't be obsoleted by 10469, I was misremembering this ticket. However, I am a little confused about what is actually wanted--because of the <code>$(exit)</code> link I thought we wanted to be able to execute <code>sage-env</code>. I think the correct thing to do is change all of the exit's to return's since <code>sage-env</code> is meant to be sourced. This means that any script which sources sage-env will have to check return status and exit rather than relying on <code>sage-env</code> to exit for it.
</p>
<p>
I could only find <code>sage-sage</code>, <code>Makefile</code>, <code>sage-spkg</code>, and a <code>start-sage.sh</code> in the Mac app which source <code>sage-env</code> so we will have to change them to check the exit status, and <code>sage-sage</code> and <code>Makefile</code> already do. I talked with William and he said he doesn't know why it uses exit rather than having the calling script check the exit status.
</p>
<p>
If we want/need to keep the current behavior of exiting, we could use a hack like
</p>
<pre class="wiki"># Exit if not called interactively. For some reason scripts sourcing
# sage-env expect errors to exit them. However this is evil for
# interactive shells.
case $- in
*i*) # interactive shell should return
die=return;;
*) # non-interactive shell
die=exit;;
esac
# ...
# some error
$die 1
</pre><p>
to not exit the shell unless it's interactive. Unfortunately I'm not sure how portable it is--I checked bash and zsh.
</p>
<p>
---
</p>
<p>
What my previous hack does is try to return and if that fails (because it was executed instead of sourced) then it calls exit. If it's only ever sourced (per <a class="closed ticket" href="https://trac.sagemath.org/ticket/10469" title="defect: Don't (effectively) source sage-env more than once (closed: fixed)">#10469</a>) then it would be better to change them all to return, since that's what my hack would do.
</p>
TicketjhpalmieriThu, 24 Mar 2011 23:44:01 GMT
https://trac.sagemath.org/ticket/4029#comment:15
https://trac.sagemath.org/ticket/4029#comment:15
<p>
See <a class="closed ticket" href="https://trac.sagemath.org/ticket/9960" title="defect: require SAGE_CHECK to be "yes" (closed: fixed)">#9960</a>; that changes every "exit" to "return" in sage-env. Perhaps that one will obsolete this one. If you wanted to give it one more review, that would be great.
</p>
TicketiandrusFri, 25 Mar 2011 00:09:13 GMT
https://trac.sagemath.org/ticket/4029#comment:16
https://trac.sagemath.org/ticket/4029#comment:16
<p>
That would definitely obsolete this one, though we still need to change the <code>start-sage.sh</code> in the Mac app. I think that's fairly low priority though, and since I'm the one who maintains that, it should be easy.
</p>
TicketjhpalmieriFri, 25 Mar 2011 14:46:30 GMTstatus, description changed; reviewer set
https://trac.sagemath.org/ticket/4029#comment:17
https://trac.sagemath.org/ticket/4029#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Ivan Andrus, John Palmieri</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/4029?action=diff&version=17">diff</a>)
</li>
</ul>
<p>
I'm marking this "positive review" so the release manager can close it once <a class="closed ticket" href="https://trac.sagemath.org/ticket/9960" title="defect: require SAGE_CHECK to be "yes" (closed: fixed)">#9960</a> is merged.
</p>
TicketjhpalmieriFri, 25 Mar 2011 17:31:25 GMTdescription changed
https://trac.sagemath.org/ticket/4029#comment:18
https://trac.sagemath.org/ticket/4029#comment:18
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/4029?action=diff&version=18">diff</a>)
</li>
</ul>
TicketjdemeyerSat, 26 Mar 2011 19:46:54 GMTmilestone changed
https://trac.sagemath.org/ticket/4029#comment:19
https://trac.sagemath.org/ticket/4029#comment:19
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7</em> to <em>sage-duplicate/invalid/wontfix</em>
</li>
</ul>
TicketjdemeyerFri, 13 May 2011 06:12:45 GMTstatus, summary changed; resolution set
https://trac.sagemath.org/ticket/4029#comment:20
https://trac.sagemath.org/ticket/4029#comment:20
<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>
<li><strong>summary</strong>
changed from <em>[with patch; needs work] sage-env kills the shell when called from "wrong" directory</em> to <em>sage-env kills the shell when called from "wrong" directory</em>
</li>
</ul>
<p>
See <a class="closed ticket" href="https://trac.sagemath.org/ticket/9960" title="defect: require SAGE_CHECK to be "yes" (closed: fixed)">#9960</a>.
</p>
Ticket