Opened 8 years ago

Closed 8 years ago

#11790 closed defect (fixed)

`sage --sh -c ...` shouldn't print [that many] messages

Reported by: leif Owned by:
Priority: blocker Milestone: sage-5.0
Component: scripts Keywords: subshell commands sage-sage environment batch mode stdout
Cc: iandrus Merged in: sage-5.0.beta9
Authors: John Palmieri, Jeroen Demeyer Reviewers: Jeroen Demeyer, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12647 Stopgaps:

Description (last modified by jdemeyer)

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 and 11790_review.patch to the SAGE_ROOT repository.

Attachments (5)

trac_11790-sage-sh.patch (3.4 KB) - added by jhpalmieri 8 years ago.
scripts repo
trac_11790-sage-sh.v2.patch (3.5 KB) - added by jhpalmieri 8 years ago.
scripts repo
trac_11790-sage-sh.v3.patch (5.1 KB) - added by jhpalmieri 8 years ago.
scripts repo
11790_sage_sh.patch (4.6 KB) - added by jdemeyer 8 years ago.
11790_review.patch (4.5 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (43)

comment:1 Changed 8 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Status changed from new to needs_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 8 years ago by jhpalmieri

  • Dependencies set to #11866

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

comment:3 follow-up: Changed 8 years ago by leif

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 8 years ago by jhpalmieri

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 8 years ago by jhpalmieri

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

comment:6 Changed 8 years ago by leif

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 8 years ago by jhpalmieri

Here's a new patch.

Changed 8 years ago by jhpalmieri

scripts repo

comment:8 Changed 8 years ago by leif

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

comment:9 follow-up: Changed 8 years ago by jhpalmieri

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 8 years ago by jhpalmieri

scripts repo

comment:10 in reply to: ↑ 9 Changed 8 years ago by leif

Replying to jhpalmieri:

Like this?

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

comment:11 follow-ups: Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_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 8 years ago by jdemeyer

  • Reviewers set to 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 8 years ago by leif

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 ; follow-up: Changed 8 years ago by 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.

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

comment:15 Changed 8 years ago by jhpalmieri

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 follow-up: Changed 8 years ago by jhpalmieri

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 8 years ago by jdemeyer

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 8 years ago by jdemeyer

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 8 years ago by jhpalmieri

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 8 years ago by leif

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 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_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 8 years ago by jhpalmieri

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

Changed 8 years ago by jhpalmieri

scripts repo

comment:23 Changed 8 years ago by iandrus

  • Cc iandrus added

comment:24 Changed 8 years ago by jdemeyer

  • Priority changed from minor to blocker

comment:25 Changed 8 years ago by jdemeyer

  • Dependencies #11866 deleted
  • Description modified (diff)

comment:26 Changed 8 years ago by jdemeyer

I think adding a sagerc file warrants its own ticket: #12647.

comment:27 Changed 8 years ago by jdemeyer

  • Dependencies set to #12647
  • Status changed from needs_review to needs_work

Changed 8 years ago by jdemeyer

Changed 8 years ago by jdemeyer

comment:28 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:29 Changed 8 years ago by jdemeyer

Reviewer patch needs review...

comment:30 Changed 8 years ago by jdemeyer

Adding a sagerc script (#12637) needs review.

comment:31 Changed 8 years ago by jdemeyer

I meant #12647.

comment:32 follow-up: Changed 8 years ago by jhpalmieri

  • Authors changed from John Palmieri to John Palmieri, Jeroen Demeyer
  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, John Palmieri
  • Status changed from needs_review to positive_review

This looks good to me.

Regarding the use of "exec", which was suggested by Leif, was in my version of the patch, and is in the current patch: if we want to be able to do something like sage --norc -c ... (as in #11932), then we want to be able to delete the temporary DOTSAGE directory after the Sage command finishes. Using "exec" prevents this, doesn't it? So unless I'm misunderstanding and there is a good way to clean things up after "exec CMD", we should be wary of overusing "exec" in the sage script.

Anyway, I'm willing to give this a positive review.

comment:33 in reply to: ↑ 32 Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

Regarding the use of "exec", which was suggested by Leif, was in my version of the patch, and is in the current patch: if we want to be able to do something like sage --norc -c ... (as in #11932), then we want to be able to delete the temporary DOTSAGE directory after the Sage command finishes. Using "exec" prevents this, doesn't it?

Using exec here doesn't prevent that. It just means that we cannot implement --norc itself using exec, which is fine.

Example: scriptA does

scriptB
cleanup

scriptB does

exec scriptC
cleanup2

The exec in scriptB doesn't prevent the cleanup in scriptA, it only prevents the cleanup2 in scriptB.

comment:34 follow-up: Changed 8 years ago by jhpalmieri

Right, but suppose we do something like this:

if [ "$1" = '--norc' -o "$1" = '--nodotsage' ]; then
    export DOT_SAGE=`mktemp -d ${TMPDIR:-/tmp}/dotsageXXXXXX`
    shift
    sage "$@"
    status=$?
    rm -rf "$DOT_SAGE"
    exit $status
fi

Then if you do sage --norc --sh ... (or any other option like --sh which uses exec), it will never reach the line rm -rf "$DOT_SAGE".

comment:35 in reply to: ↑ 34 Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

Then if you do sage --norc --sh ... (or any other option like --sh which uses exec), it will never reach the line rm -rf "$DOT_SAGE".

That would be very surprising, are you sure about this?

comment:36 Changed 8 years ago by jhpalmieri

No, I'm not sure about this - it seems to work the way we want, and as described in your comment. Never mind.

comment:37 Changed 8 years ago by jdemeyer

See #12698 for a sort-of follow-up.

comment:38 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.