Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#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: sage-7.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:

Status badges

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 Frédéric Chapoton

Authors: Frédéric Chapoton
Branch: u/chapoton/20322
Commit: 465db90b975dbf2c2ada8d8b1c8f33ea4f048fcf
Status: newneeds_review

setting SAGE_BANNER to "bare" should give the expected result


New commits:

465db90trac #20322 quick solution for a shorten banner

comment:2 Changed 7 years ago by Frédéric Chapoton

Cc: Erik Bray Jeroen Demeyer added

comment:3 Changed 7 years ago by Jeroen Demeyer

Component: PLEASE CHANGEuser interface
Status: needs_reviewneeds_work
Type: PLEASE CHANGEenhancement

Can you use consistent indentation?

comment:4 Changed 7 years ago by git

Commit: 465db90b975dbf2c2ada8d8b1c8f33ea4f048fcfc2d0ff3df2b0543fbab75c9c24e067d9bd6d48e4

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

c2d0ff3indentation

comment:5 Changed 7 years ago by Frédéric Chapoton

Status: needs_workneeds_review

comment:6 Changed 7 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer
Status: needs_reviewneeds_work

I am adding a commit to make the bare and full banners more consistent...

comment:7 Changed 7 years ago by Frédéric Chapoton

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 Jeroen Demeyer

Branch: u/chapoton/20322u/jdemeyer/20322

comment:9 Changed 7 years ago by Jeroen Demeyer

Commit: c2d0ff3df2b0543fbab75c9c24e067d9bd6d48e4846e21de218ace07322ac24a64341ec71141578e
Status: needs_workneeds_review

New commits:

846e21dMake full and bare banner consistent

comment:10 Changed 7 years ago by Jeroen Demeyer

Running full doctests now with SAGE_BANNER=bare...

comment:11 Changed 7 years ago by git

Commit: 846e21de218ace07322ac24a64341ec71141578e8b1c3697418043b7c1a6c4547d5e412a2ebe0e5f

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

8b1c369Ensure that SAGE_BANNER=no is not exported

comment:12 Changed 7 years ago by Jeroen Demeyer

Doctests pass: positive_review from my part.

comment:13 Changed 7 years ago by Frédéric Chapoton

Status: needs_reviewpositive_review

ok, let it be

comment:14 Changed 7 years ago by Erik Bray

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 Frédéric Chapoton

@embray

This was just to help you to make the sage-patchbot docker container work.

comment:16 Changed 7 years ago by Erik Bray

I just haven't had this problem specifically since I'm not trying to run the sage-patchbot container on Windows. In the meantime I just disabled the SAGE_BANNER for the docker images in sagemath/docker-images@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 Frédéric Chapoton

This is the disabling of the banner that causes some failing doctests:

http://patchbot.sagemath.org/log/0/Ubuntu/15.10/x86_64/3.13.0-43-generic/62744a2b1a07/2016-03-28%2022:18:21

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 Erik Bray

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 Volker Braun

Status: positive_reviewneeds_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 Frédéric Chapoton

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 Jeroen Demeyer

Status: needs_workpositive_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 git

Commit: 8b1c3697418043b7c1a6c4547d5e412a2ebe0e5f5b87f1aca7a38d0c5ef80b8e12b16822039a7360
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

5b87f1aUpdated SageMath version to 7.2.beta1

comment:23 Changed 7 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

comment:24 Changed 7 years ago by Jeroen Demeyer

The last commit isn't really needed, it's just to make the buildbot happy.

comment:25 Changed 7 years ago by Erik Bray

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 Common-Lisp") >= 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)
Version 0, edited 7 years ago by Erik Bray (next)

comment:26 Changed 7 years ago by Volker Braun

Branch: u/jdemeyer/203225b87f1aca7a38d0c5ef80b8e12b16822039a7360
Resolution: fixed
Status: positive_reviewclosed

comment:27 Changed 6 years ago by Erik Bray

Commit: 5b87f1aca7a38d0c5ef80b8e12b16822039a7360

Incidentally, the patchbot has its own unicode banner that is causing Docker client to crash: https://github.com/robertwb/sage-patchbot/blob/master/src/patchbot.py#L455 I will open a separate issue.

comment:28 Changed 6 years ago by Erik Bray

I have submitted an issue regarding the patchbot's banner to https://github.com/robertwb/sage-patchbot/pull/78

Note: See TracTickets for help on using tickets.