Opened 11 years ago

Last modified 11 years ago

#11790 closed defect

`sage --sh -c ...` shouldn't print [that many] messages — at Version 25

Reported by: Leif Leonhardy Owned by:
Priority: blocker Milestone: sage-5.0
Component: scripts Keywords: subshell commands sage-sage environment batch mode stdout
Cc: Ivan Andrus Merged in:
Authors: John Palmieri Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

Currently, we have

$ ./sage --sh -c "echo Hello"

Starting subshell with Sage environment variables set.
Be sure to exit when you are done and do not do anything
with other copies of Sage!

Bypassing shell configuration files ...

Hello
Exited Sage subshell.
$ 

which is IMHO odd.

And it's inconvenient if one wants to further process the output of some command run in a Sage subshell.

In spkg/bin/sage, we have:

if [ "$1" = '-sh'  -o "$1" = '--sh' ]; then
    # AUTHORS:
    #   Carl Witty and William Stein: initial version
    #   Craig Citro: add options for not loading profile
    cd "$CUR"
    shift
    echo ""
    echo "Starting subshell with Sage environment variables set."
    echo "Be sure to exit when you are done and do not do anything"
    echo "with other copies of Sage!"
    echo ""
    SHELL_NAME=`basename $SHELL`
    echo "Bypassing shell configuration files ..."
    echo

    ...

    $SHELL_NAME $SHELL_OPTS "$@"

    status=$?
    echo "Exited Sage subshell."
    exit $status
fi

So I'd propose to change sage to not print any messages if there are any further arguments to sage --sh.

Alternatively, all (or a reduced set of) messages should at least go to stderr instead of stdout, perhaps regardless of whether -c was specified or not.

In addition, if we dropped the "Exited Sage subshell." (at least in the case of -c), we could (or should) also use

    exec $SHELL_NAME $SHELL_OPTS "$@"

(Actually, exec should be used in a couple of Sage commands which don't need any "post-processing", e.g. by sage.)

Apply 11790_sage_sh.patch to the SAGE_ROOT repository.

Change History (28)

comment:1 Changed 11 years ago by John Palmieri

Authors: John Palmieri
Status: newneeds_review

Here's a first attempt at a patch. This also fixes some of incorrect prompts (taken from #10822 -- the patch there will need to be rebased on this one if this one gets done first).

comment:2 Changed 11 years ago by John Palmieri

Dependencies: #11866

New patch rebased w.r.t. #11866.

comment:3 Changed 11 years ago by Leif Leonhardy

Looks ok, but I cannot tell (or test right now) whether all prompts, and shell options regarding not executing rc files are correct.

The code testing for -c could be moved up, since everything else (in the case ... esac) is useless in this case .

Also, if exec returns, this means an error, so we could print some according message in this case (although hopefullyTM the shell itself did).

comment:4 in reply to:  3 Changed 11 years ago by John Palmieri

Replying to leif:

Looks ok, but I cannot tell (or test right now) whether all prompts, and shell options regarding not executing rc files are correct.

On my Mac, I was able to run "chsh" and then open up new terminal windows and run "sage -sh" to test the prompts.

The code testing for -c could be moved up, since everything else (in the case ... esac) is useless in this case .

Well, if you're running "sage --sh CMD", it should matter whether you're running "bash" or "bash --norc", right? The case ... esac stuff sets shell options like --norc in addition to the prompt, and I think we want this.

Also, if exec returns, this means an error, so we could print some according message in this case (although hopefullyTM the shell itself did).

So are you suggesting not checking the status at all? Like this:

  • sage-sage

    diff --git a/sage-sage b/sage-sage
    a b PS1="SAGE_ROOT=${SAGE_ROOT} 
    518518    esac
    519519    if [ "$1" = '-c' ]; then
    520520       exec $SHELL_NAME $SHELL_OPTS "$@"
    521        status=$?
    522        exit $status
     521       echo "An error seems to occurred." 1>&2
     522       exit 1
    523523    fi
    524524    # -c is not the first option, so print informative messages...
    525525    echo "" 1>&2

I could probably come up with a more cryptic message, though.

comment:5 Changed 11 years ago by John Palmieri

s/to occurred/to have occurred/, obviously.

comment:6 Changed 11 years ago by Leif Leonhardy

Replying to jhpalmieri:

Replying to leif:

The code testing for -c could be moved up, since everything else (in the case ... esac) is useless in this case .

Well, if you're running "sage --sh CMD", it should matter whether you're running "bash" or "bash --norc", right? The case ... esac stuff sets shell options like --norc in addition to the prompt, and I think we want this.

Any shell called with -c must not execute any rc files.


Also, if exec returns, this means an error, so we could print some according message in this case (although hopefullyTM the shell itself did).

So are you suggesting not checking the status at all? Like this:

  • sage-sage

    diff --git a/sage-sage b/sage-sage
    a b PS1="SAGE_ROOT=${SAGE_ROOT} 
    518518    esac
    519519    if [ "$1" = '-c' ]; then
    520520       exec $SHELL_NAME $SHELL_OPTS "$@"
    521        status=$?
    522        exit $status
     521       echo "An error seems to occurred." 1>&2
     522       exit 1
    523523    fi
    524524    # -c is not the first option, so print informative messages...
    525525    echo "" 1>&2

I could probably come up with a more cryptic message, though.

:)

I rather meant something like

    ...
    SHELL_NAME=`basename $SHELL`

    if [ "$1" = "-c" ]; then
        exec "$SHELL_NAME" "$@"
        # If 'exec' returns, an error occurred:
        status=$?
        echo >&2 "Fatal error: 'exec \"$SHELL_NAME\" \"$@\"' failed!"
        exit $status # Always non-zero, but return the code the shell gave.
    fi

    case $SHELL_NAME in
    ...


Btw., any reason to call $SHELL_NAME instead of $SHELL?

The latter would IMHO be correct.

comment:7 Changed 11 years ago by John Palmieri

Here's a new patch.

Changed 11 years ago by John Palmieri

Attachment: trac_11790-sage-sh.patch added

scripts repo

comment:8 Changed 11 years ago by Leif Leonhardy

Both $SHELL and $SHELL_NAME should in principle be quoted... ;-)

comment:9 Changed 11 years ago by John Palmieri

Like this?

  • sage-sage

    diff --git a/sage-sage b/sage-sage
    a b if [ "$1" = '-sh' -o "$1" = '--sh' ]; t 
    473473    cd "$CUR"
    474474    shift
    475475    if [ "$1" = '-c' ]; then
    476        exec $SHELL "$@"
     476       exec "$SHELL" "$@"
    477477        # If 'exec' returns, an error occurred:
    478478        status=$?
    479479        echo >&2 "Fatal error: 'exec \"$SHELL\" \"$@\"' failed!"
    if [ "$1" = '-sh' -o "$1" = '--sh' ]; t 
    489489    echo "" 1>&2
    490490    # We must start a new shell with no .profile or .bashrc files
    491491    # processed, so that we know our path is correct
    492     SHELL_NAME=`basename $SHELL`
    493     case $SHELL_NAME in
     492    SHELL_NAME=`basename "$SHELL"`
     493    case "$SHELL_NAME" in
    494494        bash)
    495495            SHELL_OPTS=" --norc"
    496496            PS1="SAGE_ROOT=${SAGE_ROOT}\n(sage subshell) \h:\W \u\$ "
    PS1="SAGE_ROOT=${SAGE_ROOT} 
    531531            echo >&2 "Aborting."
    532532            exit 1
    533533    esac
    534     $SHELL $SHELL_OPTS "$@"
     534    "$SHELL" $SHELL_OPTS "$@"
    535535    status=$?
    536536    echo "Exited Sage subshell." 1>&2
    537537    exit $status

Let's make this v2 of the patch.

Changed 11 years ago by John Palmieri

Attachment: trac_11790-sage-sh.v2.patch added

scripts repo

comment:10 in reply to:  9 Changed 11 years ago by Leif Leonhardy

Replying to jhpalmieri:

Like this?

Yep. As a MacOS user, you should be familiar with spaces in filenames... ;-)

comment:11 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

I would prefer not to special-case -c and print the "informative" messages only when no arguments are given.

Second, please get rid of the double-line prompt. I find that so confusing. No need to print SAGE_ROOT on every prompt (maybe print it once in the "informative" messages).

comment:12 Changed 11 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer

Third, now that you are working on this: it is absolutely wrong to assume that /bin/sh supports --norc. I would not make any assumptions at all about which shell is /bin/sh, so I would set no options for sh.

comment:13 in reply to:  11 Changed 11 years ago by Leif Leonhardy

Replying to jdemeyer:

I would prefer not to special-case -c and print the "informative" messages only when no arguments are given.

Second, please get rid of the double-line prompt. I find that so confusing. No need to print SAGE_ROOT on every prompt (maybe print it once in the "informative" messages).

+N (N>=1)

I always found that annoying, but hesitated to change it since others might argue that it may be helpful to "less experienced users"TM.

(One reason I hardly ever use the Sage subshell... ;-) )

comment:14 in reply to:  11 ; Changed 11 years ago by Leif Leonhardy

Replying to jdemeyer:

I would prefer not to special-case -c and print the "informative" messages only when no arguments are given.

Well, other options could be passed such that the shell would still be interactive.

In that case, the "informative messages" should probably still get printed.

comment:15 Changed 11 years ago by John Palmieri

I wouldn't mind shortening the prompt to one line, but I'd like to get more feedback, so I asked the question on sage-devel.

If it is shortened, should we do more to highlight that we're in a subshell? The following shortens "(sage subshell)" to "(Sage)" and puts in reverse video. We could use boldface if reverse video is too obnoxious.

  • sage-sage

    diff --git a/sage-sage b/sage-sage
    a b if [ "$1" = '-sh' -o "$1" = '--sh' ]; t 
    490491    # We must start a new shell with no .profile or .bashrc files
    491492    # processed, so that we know our path is correct
    492493    SHELL_NAME=`basename "$SHELL"`
     494    if [ -x /usr/bin/tput ] && tput setaf 1 >&/dev/null; then
     495        color_prompt=yes
     496    else
     497        color_prompt=
     498    fi
    493499    case "$SHELL_NAME" in
    494500        bash)
    495501            SHELL_OPTS=" --norc"
    496             PS1="SAGE_ROOT=${SAGE_ROOT}\n(sage subshell) \h:\W \u\$ "
     502            if [ "$color_prompt" = yes ] ; then
     503                PS1="\[$(tput rev)\](Sage)\[$(tput sgr0)\] \h:\W \u\$ "
     504            else
     505                PS1="(Sage) \h:\W \u\$ "
     506            fi
    497507            export PS1
    498508            ;;
    499509        csh)
    if [ "$1" = '-sh' -o "$1" = '--sh' ]; t 
    504514            ;;
    505515        ksh)
    506516            SHELL_OPTS=" -p"
    507             PS1="SAGE_ROOT=${SAGE_ROOT}
    508 (sage subshell) `hostname -s`:\${PWD##*/} $USER$ "
     517            if [ "$color_prompt" = yes ] ; then
     518                PS1="\[$(tput rev)\](Sage)\[$(tput sgr0)\] `hostname -s`:\${PWD##*/} $USER$ "
     519            else
     520                PS1="(Sage) `hostname -s`:\${PWD##*/} $USER$ "
     521            fi
    509522            export PS1
    510523            ;;
    511524        sh)
    512             SHELL_OPTS=" --norc"
    513             PS1="SAGE_ROOT=${SAGE_ROOT}
    514 (sage subshell) `hostname -s`:\${PWD##*/} $USER$ "
     525            # If sh is derived from bash, then the following is okay,
     526            # but we shouldn't assume this.
     527            #SHELL_OPTS=" --norc"
     528            PS1="(Sage) `hostname -s`:\${PWD##*/} $USER$ "
    515529            export PS1
    516530            ;;
    517531        tcsh)
    if [ "$1" = '-sh' -o "$1" = '--sh' ]; t 
    521535            SHELL_OPTS=" -f"
    522536            ;;
    523537        zsh)

comment:16 Changed 11 years ago by John Palmieri

Simon King had the interesting suggestion of prepending e.g. "(sage-sh)" to the current prompt. Can we access the current prompt?

comment:17 in reply to:  16 Changed 11 years ago by Jeroen Demeyer

Replying to jhpalmieri:

Simon King had the interesting suggestion of prepending e.g. "(sage-sh)" to the current prompt. Can we access the current prompt?

Well, $PS1 is the current prompt. So you could do

export PS1="(sage-sh) $PS1"

comment:18 in reply to:  14 Changed 11 years ago by Jeroen Demeyer

Replying to leif:

Replying to jdemeyer:

I would prefer not to special-case -c and print the "informative" messages only when no arguments are given.

Well, other options could be passed such that the shell would still be interactive.

On the other hand, other options (e.g. the name of a script) could be passed such that the shell would not be interactive. I would say that any option passed qualifies as "advanced usage" meaning the user knows what he is doing and does not need "informative" messages.

comment:19 Changed 11 years ago by John Palmieri

Replying to jdemeyer:

Well, $PS1 is the current prompt. So you could do

I tried that but it doesn't seem to work: $PS1 is not defined when running sage-sage: add a line echo "current prompt is $PS1" somewhere.

comment:20 Changed 11 years ago by Leif Leonhardy

I don't like playing with "fancy" prompts.

If someone needs such, we could support SAGE_SHELL_PROMPT or whatever.

(Ceterum censeo we should support ~/.sagerc [or $DOT_SAGE/.sagerc or both], or ~/.sageshrc, for common environment settings etc., i.e., this should be a shell script which is sourced by sage-env if present.)

comment:21 Changed 11 years ago by John Palmieri

Status: needs_workneeds_review

Here is version 3 of the patch. This implements Leif's idea about ./sage/.sagerc, in a simple-minded way. It also checks for the environment variable SAGE_SHPROMPT.

My opinion is that the prompt for the Sage shell should indicate very clearly that this is a Sage shell, so the default in this patch is for it to start with "(sage-sh)" in reverse video. If you don't like it, you can override it by setting PS1 in .sagerc or by setting SAGE_SHPROMPT anywhere.

This also doesn't print any warning message or even set the prompt if sage -sh is followed by more arguments. I'm not sure I'm happy about that, in particular the potential lack of a different prompt.

comment:22 Changed 11 years ago by John Palmieri

Okay, I changed it so it sets the prompt regardless of the number of arguments.

Changed 11 years ago by John Palmieri

Attachment: trac_11790-sage-sh.v3.patch added

scripts repo

comment:23 Changed 11 years ago by Ivan Andrus

Cc: Ivan Andrus added

comment:24 Changed 11 years ago by Jeroen Demeyer

Priority: minorblocker

comment:25 Changed 11 years ago by Jeroen Demeyer

Dependencies: #11866
Description: modified (diff)
Note: See TracTickets for help on using tickets.