cat: .../local/bin/sage-banner: No such file or directory

[jdemeyer]

Sage 7.3.beta8 is giving me a lot of errors of the form

[atlas-3.10.2.p2] cat: /usr/local/src/sage-config/local/bin/sage-banner: No such file or directory

The reason is that the file local/bin/sage-banner is installed at some point but it is always read by sage-env.

[embray]

Right...see #20897. I guess that should check if the file exists first before setting that environment variable. It looks like that could happen from a fresh build.

(Do the patch bots really not test fresh builds from scratch?)

[jdemeyer]

For the record, this does not cause the build to fail, just the error message gets printed.

[jdemeyer]

Do we really need SAGE_BANNER_TEXT as variable? I suggest to use a shell function instead, such that the banner text gets computed only when needed, not every time that sage-env is sourced.

[embray]

A shell function would be fine. Thanks for the suggestion.

[embray]

New commits:

​1aae17a

Do away with SAGE_BANNER_TEXT environment variable, replace with a shell function having the same purpose.

[jdemeyer]

I would replace

    if [ -n "$banner_file" -a -f "$banner_file" ]; then
        cat "$banner_file"
    fi

by

    cat "$banner_file"

since we want to see an error message when we ask for the banner but it doesn't exist.

[embray]

I'm not sure about that. I think it's of such minor importance that it should just be silent in that case. The banner files doesn't exist yet when someone first checks out the source, but if they run, for example, ./sage --help (incorrectly, of course) they would get an error. I think it's not worth the panic that any error inevitably causes.

[jdemeyer]

Replying to embray:

I think it's not worth the panic that any error inevitably causes.

In that case, just use cat >&2 file to silence error message because it's simpler.

[embray]

I think this is fine as-is. That looks like line noise to me even though I know what it means.

[jhpalmieri]

Replying to embray:

I think this is fine as-is. That looks like line noise to me even though I know what it means.

I don't understand this comment. There are many instances of using >&2 in Sage scripts; one might even say that it is approved (or at least standard) style for shell scripts in Sage. Why not listen to a conscientious reviewer and make the simple change he requests?

[embray]

Why call an executable just to throw its output away when you can be certain, using shell built-ins, that it's not going to work anyways?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​16ad005

Just use cat

[embray]

Look, this is hardly the hill I want to die on :)

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​e5ee236

Just use cat

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

[embray]

Actually, decided to undo that. I don't know if it's just that I'm tired and keep making mistake or what but cat even with errors suppressed still outputs a pointless newline so that's undesirable.

[jdemeyer]

I agree it's not all that important, but for me 2>/dev/null cat no_such_file outputs nothing, not a newline. I see no reason at all why cat would randomly output newlines, so I guess you must have made a mistake somewhere.

Anyway, leave the branch alone for now, I wanted to make some further simplifications anyway.

[embray]

No, it doesn't output a newline. I had a bit flipped incorrectly in my head somewhere.

[jdemeyer]

If you agree, you can set this to positive_review.

---

New commits:

​0d06b17

Simplify printing of banner

[embray]

+ cat "$SAGE_ROOT/VERSION.txt"

Correct me if I'm wrong, but I think it's possible for $SAGE_ROOT/VERSION.txt to be missing if building for the first time as well. That's why it was done the way I had it.

[jdemeyer]

$SAGE_ROOT/VERSION.txt is tracked by git, so it should always be present.

[vbraun]

Various failures of the form

sage -t --long src/sage/doctest/test.py
**********************************************************************
File "src/sage/doctest/test.py", line 26, in sage.doctest.test
Failed example:
    subprocess.call(["sage", "-t", "--warn-long", "0", "longtime.rst"], **kwds)  # long time
Expected:
    Running doctests...
    Doctesting 1 file.
    sage -t --warn-long 0.0 longtime.rst
    [0 tests, ...s]
    ----------------------------------------------------------------------
    All tests passed!
    ----------------------------------------------------------------------
    ...
    0
Got:
    /Users/buildslave-sage/slave/sage_git/build/src/bin/sage: line 368: sage_banner: command not found
    Running doctests with ID 2016-08-07-23-12-09-44d630e7.
    Git branch: develop
    Using --optional=mpir,python2,sage
    Doctesting 1 file.
    sage -t --warn-long 0.0 longtime.rst
        [0 tests, 0.00 s]
    ----------------------------------------------------------------------
    All tests passed!
    ----------------------------------------------------------------------
    Total time for all tests: 0.0 seconds
        cpu time: 0.0 seconds
        cumulative wall time: 0.0 seconds
    0
**********************************************************************

[embray]

Something seems fishy there. I don't think that would happen unless somehow sage-env weren't updated.

[jdemeyer]

I think I know what's wrong... commit coming up.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​4b869fb

Always define sage_banner function, even if sage-env was already sourced

[embray]

Ah, I see what happened there. Thanks, that makes sense.