Fix the Sage banner
jdemeyer opened this issue · 57 comments
Something has broken the Sage banner.
In 8.2.beta8:
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.2.beta8, Release Date: 2018-03-10 │
│ Type "notebook()" for the browser-based notebook interface. │
│ Type "help()" for help. │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable. ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
In 8.2.rc0:
SageMath version 8.2.rc0, Release Date: 2018-03-28
This is despite the fact that printing the banner works:
SageMath version 8.2.rc0, Release Date: 2018-03-28
sage: from sage.misc.banner import banner
sage: banner()
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.2.rc0, Release Date: 2018-03-28 │
│ Type "notebook()" for the browser-based notebook interface. │
│ Type "help()" for help. │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable. ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
CC: @embray
Component: user interface
Author: Jeroen Demeyer, Erik Bray
Branch: u/vbraun/36d438055e2e9bd5399152e510b6d233fc591ffe
Reviewer: Erik Bray, Jeroen Demeyer
Issue created by migration from https://trac.sagemath.org/ticket/25056
Description changed:
---
+++
@@ -18,3 +18,19 @@
```
SageMath version 8.2.rc0, Release Date: 2018-03-28
```
+
+This is despite the fact that printing the banner works:
+
+```
+SageMath version 8.2.rc0, Release Date: 2018-03-28
+sage: from sage.misc.banner import banner_text
+sage: print(banner_text())
+┌────────────────────────────────────────────────────────────────────┐
+│ SageMath version 8.2.rc0, Release Date: 2018-03-28 │
+│ Type "notebook()" for the browser-based notebook interface. │
+│ Type "help()" for help. │
+└────────────────────────────────────────────────────────────────────┘
+┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
+┃ Warning: this is a prerelease version, and it may be unstable. ┃
+┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
+```Description changed:
---
+++
@@ -23,8 +23,8 @@
```
SageMath version 8.2.rc0, Release Date: 2018-03-28
-sage: from sage.misc.banner import banner_text
-sage: print(banner_text())
+sage: from sage.misc.banner import banner
+sage: banner()
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.2.rc0, Release Date: 2018-03-28 │
│ Type "notebook()" for the browser-based notebook interface. │Description changed:
---
+++
@@ -34,3 +34,5 @@
┃ Warning: this is a prerelease version, and it may be unstable. ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
```
+
+This is because the banner is stored in `src/bin/sage-banner` instead of being generated.Author: Jeroen Demeyer
Description changed:
---
+++
@@ -34,5 +34,3 @@
┃ Warning: this is a prerelease version, and it may be unstable. ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
```
-
-This is because the banner is stored in `src/bin/sage-banner` instead of being generated.It seems like the release manager is running sage-update-version in some context where unicode is just broken...
Changed author from Jeroen Demeyer to none
Why is it stored in src/bin anyways? It's not an executable at all...
Branch: u/jdemeyer/fix_the_sage_banner
Author: Jeroen Demeyer
Reviewer: Erik Bray
+1
Let me just make sure that the banner still gets printed pretty quickly on slower platforms (i.e. Cygwin).
On that note I've also been really wanting to add like a spinner or something while it imports sage, but that's another matter...
It does come up noticeably a little more slowly, but it's still subtle. I'm also currently running a debug build which probably accounts for it being slow to begin with, so I'm not too concerned about it right now.
**********************************************************************
File "src/sage/misc/lazy_attribute.pyx", line 88, in sage.misc.lazy_attribute._lazy_attribute._sage_src_lines_
Failed example:
src[0]
Expected:
'def banner(full=None):\n'
Got:
'def banner():\n'
**********************************************************************
File "src/sage/misc/lazy_attribute.pyx", line 90, in sage.misc.lazy_attribute._lazy_attribute._sage_src_lines_
Failed example:
lines
Expected:
87
Got:
81
**********************************************************************
1 item had failures:
2 of 6 in sage.misc.lazy_attribute._lazy_attribute._sage_src_lines_
[130 tests, 2 failures, 2.79 s]
----------------------------------------------------------------------
sage -t --long src/sage/misc/abstract_method.py # 2 doctests failed
sage -t --long src/sage/misc/lazy_attribute.pyx # 2 doctests failed
I suggest those tests be rewritten around a dummy function that is not going to actually be changed...
The banner was broken in #24825, now the output of the full banner can't be redirected
$ sage --python -c "import sage.misc.banner; sage.misc.banner.banner(full=True)" | cat
SageMath version 8.2.rc1, Release Date: 2018-03-31
Pipes that don't lead to the terminal do need to be encoded, check with isatty or always encode.
Also cold starts can be slow (especially over nfs), which is why historically we want to print something before Python starts up.
Well, starting up Python should only a fraction of the time of starting up Sage.
maybe we should just undo #24825, in order to allow the release of 8.2 ?
It's not just a question of whether or not it's a TTY. On Python 3 sys.stdout is still a TextIOWrapper with an encoding regardless whether it's attached to a terminal or not. That's only a problem on Python 2, apparently.
We could make this work more consistently by having the banner() function encode string (e.g. as UTF-8) before printing on Python 2 (but not on Python 3, where this is already fine).
That said, I still prefer Jeroen's solution in general; it just needs a trivial fix to that test (which I still believe should be rewritten...)
Changed branch from u/jdemeyer/fix_the_sage_banner to u/embray/fix_the_sage_banner
Here I fixed the underlying issue on Python 2, and I also fixed the failing test. However, I still kept Jeroen's solution of no longer pre-generating the banner text.
New commits:
670de94 | Fix printing the banner on Python 2 when stdout does not have an encoding (e.g. isatty) |
0bd5fcc | Fix this test to account for changes to 'banner' |
Changed reviewer from Erik Bray to Erik Bray, Jeroen Demeyer
I don't agree with
# sys.stdout does not have a clear encoding associated with it on
# Python 2 (it may have been redirected, for example); so just
# encode as UTF-8 and write the bytes directly to sys.stdout
If we don't know the encoding, shouldn't we play it safe and assume ASCII instead? You are right that encoding=None seems to occur when output has been redirected. In that case, the banner is probably not important anyway, so I don't see the point for your special case.
Changed author from Jeroen Demeyer to Jeroen Demeyer, Erik Bray
The point is that on Python 2 you're writing strs which can contain arbitrary bytes, including UTF-8 encoded text. It doesn't matter. If you did, say, want to dump to a text file for example this is fine. Python 3 already handles text better in this regard. This is just a middle ground (albeit probably not needed in this particular function along with your additional changes).
In other words, whether on Python 2 or on Python 3 the end result of this is writing a stream of bytes--typically UTF-8 encoded text--to a file object. The difference is just who's handling the encoding and when (in Python 3 the higher-level I/O object is handling the encoding).
Replying to @embray:
The point is that on Python 2 you're writing
strs which can contain arbitrary bytes, including UTF-8 encoded text.
But why would you write UTF-8 encoded text when you have no idea whether stdout is actually expecting UTF-8? I would make a lot more sense to say "I don't know the encoding, so let's be safe and assume ASCII".
Python 3 already handles text better in this regard.
Or worse in my opinion. When Python 2 doesn't know the encoding, it honestly says so. With Python 3, saying "I don't know" is not an option, so it just guesses some encoding.
Let me put it another way--what this is doing currently is exactly the same on Python 2 as what it did before I started making changes to it for Python 3 support.
But why would you write UTF-8 encoded text when you have no idea whether stdout is actually expecting UTF-8?
Because that's how Python 2 works--this is the kind of thing Python 3 is trying to fix :)
Or worse in my opinion. When Python 2 doesn't know the encoding, it honestly says so. With Python 3, saying "I don't know" is not an option, so it just guesses some encoding.
I don't get it. Is Python 2 worse or better? Because what you're complaining about just above this is how Python 2 works. Python 3 is not just "guessing some encoding". It's using the encoding from the current locale which is how other applications work (this is, for example, how your editor chooses an encoding to write when you enter non-ASCII text into a document).
Replying to @embray:
what this is doing currently is exactly the same on Python 2 as what it did before I started making changes to it for Python 3 support.
Indeed. And I was happy that this bug (writing UTF-8 regardless of actual encoding) finally got fixed in #24825. And now you suggest to revert that fix.
I don't know what you mean by "actual encoding". There is no "actual encoding". It's a file descriptor. If you want to write some text to a file descriptor it always has to have some encoding and that has to come from somewhere.
How about this: If you want to switch back to your branch (and just add the fix to the tests) I don't mind. My only goal here was to fix a regression. At the same time, as long as Volker agrees to the rest of your changes (getting rid of the pre-written banner files) then it's kind of a moot point.
The rest of the confusion over this mostly stems from the use of "print()" instead of, say, providing an interface explicitly for writing the banner to a text file in some specific encoding (which I agree kind of sucks anyways because it's not necessarily the right encoding for the user's terminal).
I'm not going to play referee, please figure something out that works amongst yourselves :-)
Changed branch from u/embray/fix_the_sage_banner to u/jdemeyer/fix_the_sage_banner
That's fine--I was just trying to restore some "backwards compatibility", but I think we can both agree it was bad to begin with, and that it's a moot point now.
Still get
sage -t --long src/sage/misc/abstract_method.py # 2 doctests failed
That reminds me to change that silly test.
I see; this is a different test that also depends on the source code of that particular function...
Changed branch from u/jdemeyer/fix_the_sage_banner to u/embray/fix_the_sage_banner
Thank you for annoying sage-on-distros by using SAGE_ROOT in the main sage script. SAGE_ROOT outside of the build system is just a pain in the ass. If you have to use a text file to store the version, name it uniquely and ship to /etc, or put it in a folder exclusively belonging to sage.
I guess it is too late at this stage, we'll deal with it later.
Changed branch from u/embray/fix_the_sage_banner to 36d4380
Replying to @kiwifb:
Thank you for annoying sage-on-distros by using
SAGE_ROOTin the main sage script. SAGE_ROOT outside of the build system is just a pain in the ass. If you have to use a text file to store the version, name it uniquely and ship to /etc, or put it in a folder exclusively belonging to sage.I guess it is too late at this stage, we'll deal with it later.
I don't think the sarcasm is necessary--I see your point but it's not completely obvious, and you had the opportunity here to set it back to "needs_work" or something--I wouldn't mind. Please open a new ticket for this and CC me and set it to "blocker". An easy fix for this would be to ensure "VERSION.txt" is installed somewhere sensinble under $SAGE_LOCAL (like, not bin/ :) and use it from there if possible.
Replying to @embray:
Replying to @kiwifb:
Thank you for annoying sage-on-distros by using
SAGE_ROOTin the main sage script. SAGE_ROOT outside of the build system is just a pain in the ass. If you have to use a text file to store the version, name it uniquely and ship to /etc, or put it in a folder exclusively belonging to sage.I guess it is too late at this stage, we'll deal with it later.
I don't think the sarcasm is necessary--I see your point but it's not completely obvious, and you had the opportunity here to set it back to "needs_work" or something--I wouldn't mind. Please open a new ticket for this and CC me and set it to "blocker". An easy fix for this would be to ensure "VERSION.txt" is installed somewhere sensinble under
$SAGE_LOCAL(like, notbin/:) and use it from there if possible.
I noticed this ticket because Volker included it on his branch. Rather than sarcasm, that's my exasperation at some stuff that bleeds out. I spent quite a bit of time over the years reducing the use of SAGE_ROOT. People just forget. For info I ship VERSION.txt as /etc/sage-version.txt which seems sensible but may not be the best place. I should ask what the other do.
I know, that's fair. I also want to do with any and all runtime dependency on $SAGE_ROOT--it's really annoying.
The problem with VERSION.txt looks very similar to COPYING.txt. So maybe we should deal with both in #21571?
Changed branch from 36d4380 to u/vbraun/36d438055e2e9bd5399152e510b6d233fc591ffe