Fixes to SAGE_BANNER=bare support throughout

[embray]

#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))

[chapoton]

this looks strange:

src/bin/sage-update-version

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

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

[embray]

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

[git]

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

​f890541

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

[embray]

I fixed the error and rebased.

[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.

[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.

[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 ?

[embray]

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

[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.

[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"

[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).

[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..

[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..

[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).

[git]

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

​4314e4a

Fix the `full` option to `banner_text` to actually work as documented.

[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.

[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.

[embray]

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

[git]

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

​14dc47e

Added doctest demonstrating banner_text(full=False)

[chapoton]

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

[chapoton]

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

[chapoton]

and with the branch, running

sage -bt src/sage/misc/*.py

the banner is displayed without line breaks..

[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

[embray]

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

[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.

[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.

[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.

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

[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..

[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.

[git]

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

​540ddd4	Banner text should be quoted
​6457c74	Restore the logic of not displaying any banner if SAGE_BANNER is explicitly 'no'
​9e5e9a5	These 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).

[embray]

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

[chapoton]

Should be good now.

[jdemeyer]

Follow-up: #21029