#20322 closed enhancement (fixed)
use SAGE_BANNER to propose a bare banner with no utf8
Reported by:  Frédéric Chapoton  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  user interface  Keywords:  
Cc:  Nicolas M. Thiéry, Erik Bray, Jeroen Demeyer  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  5b87f1a (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
The pretty colored banner for sage uses utf8, and this poses problem for the docker patchbot.
Let us use SAGE_BANNER to propose a shorten banner when needed.
Change History (28)
comment:1 Changed 7 years ago by
Authors:  → Frédéric Chapoton 

Branch:  → u/chapoton/20322 
Commit:  → 465db90b975dbf2c2ada8d8b1c8f33ea4f048fcf 
Status:  new → needs_review 
comment:2 Changed 7 years ago by
Cc:  Erik Bray Jeroen Demeyer added 

comment:3 Changed 7 years ago by
Component:  PLEASE CHANGE → user interface 

Status:  needs_review → needs_work 
Type:  PLEASE CHANGE → enhancement 
Can you use consistent indentation?
comment:4 Changed 7 years ago by
Commit:  465db90b975dbf2c2ada8d8b1c8f33ea4f048fcf → c2d0ff3df2b0543fbab75c9c24e067d9bd6d48e4 

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

comment:5 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:6 Changed 7 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → needs_work 
I am adding a commit to make the bare and full banners more consistent...
comment:7 Changed 7 years ago by
oh well, bare means "without bells and dingles"
all what is needed to have the tests pass in the docker container is that it contains the text of VERSION.txt
comment:8 Changed 7 years ago by
Branch:  u/chapoton/20322 → u/jdemeyer/20322 

comment:9 Changed 7 years ago by
Commit:  c2d0ff3df2b0543fbab75c9c24e067d9bd6d48e4 → 846e21de218ace07322ac24a64341ec71141578e 

Status:  needs_work → needs_review 
New commits:
846e21d  Make full and bare banner consistent

comment:11 Changed 7 years ago by
Commit:  846e21de218ace07322ac24a64341ec71141578e → 8b1c3697418043b7c1a6c4547d5e412a2ebe0e5f 

Branch pushed to git repo; I updated commit sha1. New commits:
8b1c369  Ensure that SAGE_BANNER=no is not exported

comment:14 Changed 7 years ago by
Out of curiosity, in what context did this come up? I reported the following issue upstream to Docker: https://github.com/docker/docker/issues/21323 But AFAICT this only affects the Docker client on Windows.
comment:15 Changed 7 years ago by
@embray
This was just to help you to make the sagepatchbot docker container work.
comment:16 Changed 7 years ago by
I just haven't had this problem specifically since I'm not trying to run the sagepatchbot container on Windows. In the meantime I just disabled the SAGE_BANNER for the docker images in sagemath/dockerimages@2c90e030 . So I was just curious how this came about since I wasn't explicitly asking for it (not that I'm against it).
comment:17 Changed 7 years ago by
This is the disabling of the banner that causes some failing doctests:
the other failing doctest is in sagedev, but not yet understood.
SO PLEASE, once the next release is out, use SAGE_BANNER=bare instead of "no"
comment:18 Changed 7 years ago by
Ah, I see. Though maybe those tests should check the actual value of the environment variable and skip if it's different, or just set it explicitly beforehand.
comment:19 Changed 7 years ago by
Status:  positive_review → needs_work 

sage t long src/sage/tests/cmdline.py ********************************************************************** File "src/sage/tests/cmdline.py", line 106, in sage.tests.cmdline.test_executable Failed example: out.find(version()) >= 0 Expected: True Got: False ********************************************************************** File "src/sage/tests/cmdline.py", line 114, in sage.tests.cmdline.test_executable Failed example: out.find(version()) >= 0 Expected: True Got: False ********************************************************************** 1 item had failures: 2 of 235 in sage.tests.cmdline.test_executable [234 tests, 2 failures, 63.86 s]
comment:20 Changed 7 years ago by
Hum, I think this just needs a banner and VERSION.txt refreshment to work.
So this is a rather special kind of ticket. Maybe to merge just before a beta release ?
comment:21 Changed 7 years ago by
Status:  needs_work → positive_review 

I guess this is yet another "the bug is not in the ticket but in the buildbot".
comment:22 Changed 7 years ago by
Commit:  8b1c3697418043b7c1a6c4547d5e412a2ebe0e5f → 5b87f1aca7a38d0c5ef80b8e12b16822039a7360 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
5b87f1a  Updated SageMath version to 7.2.beta1

comment:23 Changed 7 years ago by
Status:  needs_review → positive_review 

comment:24 Changed 7 years ago by
The last commit isn't really needed, it's just to make the buildbot happy.
comment:25 Changed 7 years ago by
I still think that if a test is expecting something particular about the environment that it should set that environment for the test. For example something like
diff git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py index 534436d..1d0395e 100644  a/src/sage/tests/cmdline.py +++ b/src/sage/tests/cmdline.py @@ 57,7 +57,7 @@ from subprocess import * import os, select def test_executable(args, input="", timeout=100.0, **kwds): +def test_executable(args, input="", timeout=100.0, env={}, **kwds): r""" Run the program defined by ``args`` using the string ``input`` on the standard input. @@ 102,7 +102,7 @@ def test_executable(args, input="", timeout=100.0, **kwds): Run Sage itself with various options::  sage: (out, err, ret) = test_executable(["sage"]) + sage: (out, err, ret) = test_executable(["sage"], env={'SAGE_BANNER': ' sage: out.find(version()) >= 0 True sage: err @@ 110,7 +110,7 @@ def test_executable(args, input="", timeout=100.0, **kwds): ...skipping... + sage: (out, err, ret) = test_executable(["sage", "q"], "3^33\n", env={ sage: out.find(version()) >= 0 False sage: out.find("5559060566555523") >= 0 @@ 489,7 +489,7 @@ def test_executable(args, input="", timeout=100.0, **kwds): ....: os.open(os.ctermid(), os.O_RDONLY) ....: return True ....: except OSError:  ....: return False + ....: return False sage: (out, err, ret) = test_executable(["sage", "ecl"], "(* 12345 54 sage: out.find("Embeddable CommonLisp") >= 0 True @@ 728,6 +728,9 @@ def test_executable(args, input="", timeout=100.0, **kwds): del pexpect_env["TERM"] except KeyError: pass + + pexpect_env.update(env) + p = Popen(args, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=pexpect_env, **kw if input: p.stdin.write(input)
comment:26 Changed 7 years ago by
Branch:  u/jdemeyer/20322 → 5b87f1aca7a38d0c5ef80b8e12b16822039a7360 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:27 Changed 6 years ago by
Commit:  5b87f1aca7a38d0c5ef80b8e12b16822039a7360 

Incidentally, the patchbot has its own unicode banner that is causing Docker client to crash: https://github.com/robertwb/sagepatchbot/blob/master/src/patchbot.py#L455 I will open a separate issue.
comment:28 Changed 6 years ago by
I have submitted an issue regarding the patchbot's banner to https://github.com/robertwb/sagepatchbot/pull/78
setting SAGE_BANNER to "bare" should give the expected result
New commits:
trac #20322 quick solution for a shorten banner