Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#20897 closed defect (fixed)

Fixes to SAGE_BANNER=bare support throughout

Reported by: embray Owned by:
Priority: major Milestone: sage-7.3
Component: user interface Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 9e5e9a5 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

#20322 fixed the standard sage CLI so that if SAGE_BANNER=bare, the simplified, non-unicode banner is displayed.

However, there were several other places this directive was not obeyed. First sage --help would still display the full banner regardless. Second, the built-in banner() function (from what the banner file is generated) also does not obey SAGE_BANNER=bare.

Because there is code that calls banner() directly, it should still only display the simplified banner if the SAGE_BANNER=bare variable is set in the environment (this is particularly relevant in the Docker container on Windows...)

(This bug caused a live demo to fail! (Mysteriously the same demo did not fail when I tested it minutes earlier....not sure why it suddenly changed))

Change History (36)

comment:1 Changed 5 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by chapoton

this looks strange:

  • src/bin/sage-update-version

    a b EOF 
    4646# Rebuild the Sage library for the change in version.py to take effect
    4747"$SAGE_ROOT/sage" -b
    4848
    49 # Update Sage version file
    50 echo "SageMath version $SAGE_VERSION, Release Date: $SAGE_RELEASE_DATE" \
    51     > "$SAGE_ROOT/VERSION.txt"
     49# Update the simplified Sage banner
     50"$SAGE_ROOT/sage" --python -c \
     51    "import sage.misc.banner; sage.misc.banner.banner(full=False)" \
     52    > "$SAGE_SRC/bin/VERSION.txt"

Do you really want to change the place where VERSION.txt is stored ?

comment:3 Changed 5 years ago by embray

Oops, thanks for pointing that out. No, that's a copy/paste error.

comment:4 Changed 5 years ago by git

  • Commit changed from 41f98cff8fbc052dd7fd0e3951ffb3a0252f8baf to f89054131b51774eae7ce66acd7bf6383448883d

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

f890541Change banner() to obey SAGE_BANNER='bare' universally.

comment:5 Changed 5 years ago by embray

I fixed the error and rebased.

comment:6 Changed 5 years ago by chapoton

You cannot use SAGE_BANNER_TEXT at the beginning of "sage" script, as sage-env is not yet sourced. The banner here needs to come from a file.

comment:7 Changed 5 years ago by embray

It's only used directly in two functions: help_banner and sage_setup. ISTM sage-env is sourced before either of this functions are used anywhere.

comment:8 Changed 5 years ago by chapoton

ok, you are right.

But now I am worried by something else, in sage-update-version

SAGE_RELEASE_DATE was used there and is no longer used anywhere. Are you creating some kind of circular thing, where nobody takes care of doing the real job ?

comment:9 Changed 5 years ago by embray

Let me take a look at that--I hadn't really paid attention to SAGE_RELEASE_DATE

comment:10 Changed 5 years ago by embray

Okay, I'm not sure what you mean by SAGE_RELEASE_DATE is no longer used anywhere. It's used right in sage-update-version to write $SAGE_SRC/bin/sage-version.sh and $SAGE_SRC/sage/version.py. That's where the SAGE_VERSION and SAGE_RELEASE_DATE values can be retrieved from at runtime.

The VERSION.txt file is mostly superfluous and I'm not sure why it's there exactly.

comment:11 Changed 5 years ago by chapoton

I would give a positive review if you would revert this change

-# Update Sage version file
-echo "SageMath version $SAGE_VERSION, Release Date: $SAGE_RELEASE_DATE" \
+# Update the simplified Sage banner
+"$SAGE_ROOT/sage" --python -c \
+    "import sage.misc.banner; sage.misc.banner.banner(full=False)" \
     > "$SAGE_ROOT/VERSION.txt"

comment:12 follow-up: Changed 5 years ago by embray

Why? What's wrong with that change?

If anything it de-duplicates code. The existing code there already outputs the exact same message as sage.misc.banner.banner(full=False). Why have two different implementations of outputting the same text, which ought to be outputting the same text, when they this point isn't well documented and could accidentally diverge? It's also perfectly reasonably symmetric that the two types of "banners" are generated from the same code, with a different options (full=True vs (full=False).

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by chapoton

Replying to embray:

Why? What's wrong with that change?

Maybe it's ok, but it will just take me more time to check that, for something not so important..

comment:14 Changed 5 years ago by chapoton

It seems that the argument full of banner_text is ignored.

banner_text always print the full banner, unless an environnement variable is changed..

comment:15 Changed 5 years ago by embray

Ah, you're right. I completely dropped some logic from that--it shouldn't pay any attention to SAGE_BANNER except by default (i.e. if full=True/False is not explicitly given).

comment:16 Changed 5 years ago by git

  • Commit changed from f89054131b51774eae7ce66acd7bf6383448883d to 4314e4ab786dbd1120ef143cce518f4cd9fcca11

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

4314e4aFix the `full` option to `banner_text` to actually work as documented.

comment:17 in reply to: ↑ 13 Changed 5 years ago by embray

Replying to chapoton:

Replying to embray:

Why? What's wrong with that change?

Maybe it's ok, but it will just take me more time to check that, for something not so important..

There is already code here which prints the exact same thing as the echo "SageMath version $SAGE_VERSION, Release Date: $SAGE_RELEASE_DATE". I see no point in having two things that produce the same output.

The only reason I replace it with banner(full=False) instead of version() is for symmetry.

banner() was updated to wrap version() (by way of full=False) to incorporate consistent handling of the SAGE_BANNER environment variable, as banner() is already called in a few other parts of the code (including by sagenb) without respecting SAGE_BANNER.

comment:18 Changed 5 years ago by chapoton

ok, ok. I am still not very convinced, but this is enough.

Could you please add one doctest for full=False in banner_text ?

Then I will give a positive review.

comment:19 Changed 5 years ago by embray

A doctest would be good to better define the expectation here.

comment:20 Changed 5 years ago by git

  • Commit changed from 4314e4ab786dbd1120ef143cce518f4cd9fcca11 to 14dc47e62aec1a944ac4a7b537e19d4016bc60b9

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

14dc47eAdded doctest demonstrating banner_text(full=False)

comment:21 Changed 5 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, then. This should work as far as I can tell. I hope there will be no unexpected side effect.

comment:22 Changed 5 years ago by chapoton

  • Status changed from positive_review to needs_work

I need to doublecheck, it seems that some doctests in misc are failing.

comment:23 Changed 5 years ago by chapoton

and with the branch, running

sage

the banner is displayed without line breaks..

Last edited 5 years ago by chapoton (previous) (diff)

comment:24 Changed 5 years ago by chapoton

I got

sage -t src/sage/misc/lazy_attribute.pyx  # 1 doctest failed
sage -t src/sage/misc/abstract_method.py  # 1 doctest failed
sage -t src/sage/misc/temporary_file.py  # 1 doctest failed
Last edited 5 years ago by chapoton (previous) (diff)

comment:25 Changed 5 years ago by embray

Oops! I'll fix the line breaks thing. That's just because there's missing quotes in the echo.

comment:26 Changed 5 years ago by embray

For the tests you have failing did you install the new code before running the tests? Otherwise they'll definitely fail since they depend on what python sources are actually being tested.

comment:27 Changed 5 years ago by chapoton

I always use "sage -bt". The 3 lines above are copy-pasted from the output of the command. I think these 3 tests really fail.

comment:28 Changed 5 years ago by embray

You'd have to show me how the tests are failing for you. The one about temporary_file.py seems particularly unrelated. The other two do contain related tests but I updated those tests to reflect these changes, as you can see in the diff.

comment:29 Changed 5 years ago by chapoton

Here they are (plus of course the missing line breaks)

sage -t src/sage/misc/abstract_method.py
**********************************************************************
File "src/sage/misc/abstract_method.py", line 194, in sage.misc.abstract_method.AbstractMethod._sage_src_lines_
Failed example:
    lines
Expected:
    83
Got:
    99
**********************************************************************
**********************************************************************
File "src/sage/misc/temporary_file.py", line 47, in sage.misc.temporary_file.delete_tmpfiles
Failed example:
    os.path.isdir(parent_SAGE_TMP)
Expected:
    True
Got:
    False
**********************************************************************
**********************************************************************
File "src/sage/misc/lazy_attribute.pyx", line 90, in sage.misc.lazy_attribute._lazy_attribute._sage_src_lines_
Failed example:
    lines
Expected:
    83
Got:
    99
**********************************************************************

comment:30 Changed 5 years ago by embray

Oh I see. The number of source lines changed too, and there's a test for that.

The one you're getting from temporary_file.py really doesn't look related..

comment:31 Changed 5 years ago by embray

Okay, I investigated the temporary_file.py test and it is related. It seems the banner is being printed when running ./sage -c which I'm guessing is not meant to be the case.

comment:32 Changed 5 years ago by git

  • Commit changed from 14dc47e62aec1a944ac4a7b537e19d4016bc60b9 to 9e5e9a57d87f8a9700bd06327b76ec23b4a978d5

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

540ddd4Banner text should be quoted
6457c74Restore the logic of not displaying any banner if SAGE_BANNER is explicitly 'no'
9e5e9a5These tests needed to be updated further since they also tested the number of lines in the test function (that happens to be 'banner', the length of which has changed).

comment:33 Changed 5 years ago by embray

  • Status changed from needs_work to needs_review

Okay, this should fix all the failing tests. Thanks again chapoton for your careful review.

comment:34 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

Should be good now.

comment:35 Changed 5 years ago by vbraun

  • Branch changed from u/embray/sage-banner to 9e5e9a57d87f8a9700bd06327b76ec23b4a978d5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:36 Changed 5 years ago by jdemeyer

  • Commit 9e5e9a57d87f8a9700bd06327b76ec23b4a978d5 deleted

Follow-up: #21029

Note: See TracTickets for help on using tickets.