Opened 13 years ago

Closed 10 years ago

#4029 closed defect (duplicate)

sage-env kills the shell when called from "wrong" directory

Reported by: dphilp Owned by: mabshoff
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: distribution Keywords: sage-env source
Cc: iandrus Merged in:
Authors: Mike Hansen Reviewers: Ivan Andrus, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jhpalmieri)

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


Once #9960 is merged, this can be closed. Don't merge any of the patches here!

Attachments (2)

trac_4029.patch (444 bytes) - added by mhansen 12 years ago.
trac_4029.2.patch (5.0 KB) - added by iandrus 10 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 13 years ago by mabshoff

  • Milestone set to sage-3.1.3

Oops, no milestone

comment:2 Changed 12 years ago by was

  • Summary changed from sage-env kills the shell when called from "wrong" directory to [with patch; needs rievew] sage-env kills the shell when called from "wrong" directory

comment:3 Changed 12 years ago by was

  • Milestone changed from sage-3.4.1 to sage-3.3

comment:4 Changed 12 years ago by mabshoff

  • Summary changed from [with patch; needs rievew] sage-env kills the shell when called from "wrong" directory to [with patch; positive review] sage-env kills the shell when called from "wrong" directory

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.

Cheers,

Michael

comment:5 Changed 12 years ago by mabshoff

  • Summary changed from [with patch; positive review] sage-env kills the shell when called from "wrong" directory to [with patch; positive review, needs rebase] sage-env kills the shell when called from "wrong" directory

The patch does not apply to my merged tree. I believe #22 collides with this, but it should be easy to rebase post 3.3.alpha2.

Cheers,

Michael

comment:6 Changed 12 years ago by mabshoff

  • Summary changed from [with patch; positive review, needs rebase] sage-env kills the shell when called from "wrong" directory to [with patch; needs rebase] sage-env kills the shell when called from "wrong" directory

The rebase needs to make sure that the changes from #22 don't get lost :)

Cheers,

Michael

Changed 12 years ago by mhansen

comment:7 Changed 12 years ago by mhansen

  • Authors set to Mike Hansen
  • Summary changed from [with patch; needs rebase] sage-env kills the shell when called from "wrong" directory to [with patch; needs review] sage-env kills the shell when called from "wrong" directory

I've attached a new patch which implements a cleaner solution using "$(exit 1)". See http://fvue.nl/wiki/Bash:_Return_or_exit%3F

comment:8 Changed 12 years ago by was

  • Summary changed from [with patch; needs review] sage-env kills the shell when called from "wrong" directory to [with patch; positive review] sage-env kills the shell when called from "wrong" directory

merged in sage-4.1.2.rc1....

comment:9 Changed 12 years ago by was

  • Milestone changed from sage-4.1.3 to sage-4.1.2

comment:10 Changed 12 years ago by was

  • Summary changed from [with patch; positive review] sage-env kills the shell when called from "wrong" directory to [with patch; needs work] sage-env kills the shell when called from "wrong" directory

Reopened because wjp pointed out a serious issue -- the rest of the script gets executed.

Some irc discussion about this:

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!"

Changed 10 years ago by iandrus

comment:11 Changed 10 years ago by iandrus

  • Cc iandrus added
  • Report Upstream set to N/A
  • Status changed from needs_work to needs_review

I used the very hacky (but effective)

return 1 2>/dev/null || exit 1

instead of return or exit.

comment:12 Changed 10 years ago by iandrus

Will be obsolete with #10469.

comment:13 Changed 10 years ago by jhpalmieri

I don't think this will be obsolete with #10469: 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

return 1 2>/dev/null || exit 1

This seems to work for me.

comment:14 Changed 10 years ago by iandrus

  • Status changed from needs_review to needs_work

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 $(exit) link I thought we wanted to be able to execute sage-env. I think the correct thing to do is change all of the exit's to return's since sage-env 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 sage-env to exit for it.

I could only find sage-sage, Makefile, sage-spkg, and a start-sage.sh in the Mac app which source sage-env so we will have to change them to check the exit status, and sage-sage and Makefile 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.

If we want/need to keep the current behavior of exiting, we could use a hack like

# 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

to not exit the shell unless it's interactive. Unfortunately I'm not sure how portable it is--I checked bash and zsh.

---

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 #10469) then it would be better to change them all to return, since that's what my hack would do.

comment:15 Changed 10 years ago by jhpalmieri

See #9960; 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.

comment:16 Changed 10 years ago by iandrus

That would definitely obsolete this one, though we still need to change the start-sage.sh 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.

comment:17 Changed 10 years ago by jhpalmieri

  • Description modified (diff)
  • Reviewers set to Ivan Andrus, John Palmieri
  • Status changed from needs_work to positive_review

I'm marking this "positive review" so the release manager can close it once #9960 is merged.

comment:18 Changed 10 years ago by jhpalmieri

  • Description modified (diff)

comment:19 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-duplicate/invalid/wontfix

comment:20 Changed 10 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from positive_review to closed
  • Summary changed from [with patch; needs work] sage-env kills the shell when called from "wrong" directory to sage-env kills the shell when called from "wrong" directory

See #9960.

Note: See TracTickets for help on using tickets.