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: |
Description (last modified by )
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)
Change History (22)
comment:1 Changed 13 years ago by
- Milestone set to sage-3.1.3
comment:2 Changed 12 years ago by
- 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
- Milestone changed from sage-3.4.1 to sage-3.3
comment:4 Changed 12 years ago by
- 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
- 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
- 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
Changed 12 years ago by
comment:7 Changed 12 years ago by
- 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
- 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
- Milestone changed from sage-4.1.3 to sage-4.1.2
comment:10 Changed 12 years ago by
- 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
comment:11 Changed 10 years ago by
- 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
Will be obsolete with #10469.
comment:13 Changed 10 years ago by
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
- 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
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
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
- 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
- Description modified (diff)
comment:19 Changed 10 years ago by
- Milestone changed from sage-4.7 to sage-duplicate/invalid/wontfix
comment:20 Changed 10 years ago by
- 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.
Oops, no milestone