Opened 6 years ago

Closed 6 years ago

#21029 closed defect (fixed)

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

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-7.3
Component: build Keywords:
Cc: embray, chapoton Merged in:
Authors: Erik Bray, Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 4b869fb (Commits, GitHub, GitLab) Commit: 4b869fbbd008b408a232c651eaa8bf0e955e626d
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (32)

comment:1 Changed 6 years ago by 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?)

comment:2 Changed 6 years ago by jdemeyer

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

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by embray

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

comment:5 Changed 6 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-21029
  • Commit set to 1aae17a7d99011aab96fd098146873578ded170e
  • Status changed from new to needs_review

New commits:

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

comment:6 Changed 6 years ago by 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.

comment:7 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:9 in reply to: ↑ 8 Changed 6 years ago by 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.

comment:10 follow-up: Changed 6 years ago by embray

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

comment:11 Changed 6 years ago by embray

  • Status changed from needs_work to needs_review

comment:12 in reply to: ↑ 10 Changed 6 years ago by 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?

comment:13 Changed 6 years ago by 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?

comment:14 Changed 6 years ago by git

  • Commit changed from 1aae17a7d99011aab96fd098146873578ded170e to 16ad005ed4e94984a26a10e6c65f55f023cd235e

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

16ad005Just use cat

comment:15 Changed 6 years ago by embray

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

Last edited 6 years ago by embray (previous) (diff)

comment:16 Changed 6 years ago by git

  • Commit changed from 16ad005ed4e94984a26a10e6c65f55f023cd235e to e5ee236cb8818cd3d80f64d38c07d6f83ce317fa

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

e5ee236Just use cat

comment:17 Changed 6 years ago by git

  • Commit changed from e5ee236cb8818cd3d80f64d38c07d6f83ce317fa to 1aae17a7d99011aab96fd098146873578ded170e

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

comment:18 Changed 6 years ago by 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.

Last edited 6 years ago by embray (previous) (diff)

comment:19 Changed 6 years ago by 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.

comment:20 Changed 6 years ago by embray

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

comment:21 Changed 6 years ago by jdemeyer

  • Branch changed from u/embray/ticket-21029 to u/jdemeyer/ticket-21029

comment:22 Changed 6 years ago by jdemeyer

  • Authors changed from Erik Bray to Erik Bray, Jeroen Demeyer
  • Commit changed from 1aae17a7d99011aab96fd098146873578ded170e to 0d06b1767b15ad98ad4775f7bb08dc8cf96cd7a3

If you agree, you can set this to positive_review.


New commits:

0d06b17Simplify printing of banner

comment:23 Changed 6 years ago by 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.

comment:24 Changed 6 years ago by jdemeyer

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

comment:25 Changed 6 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

comment:26 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

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
**********************************************************************

comment:27 Changed 6 years ago by embray

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

comment:28 Changed 6 years ago by jdemeyer

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

comment:29 Changed 6 years ago by git

  • Commit changed from 0d06b1767b15ad98ad4775f7bb08dc8cf96cd7a3 to 4b869fbbd008b408a232c651eaa8bf0e955e626d

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

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

comment:30 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:31 Changed 6 years ago by embray

  • Status changed from needs_review to positive_review

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

comment:32 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket-21029 to 4b869fbbd008b408a232c651eaa8bf0e955e626d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.