Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#20322 closed enhancement (fixed)

use SAGE_BANNER to propose a bare banner with no utf8

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.2
Component: user interface Keywords:
Cc: nthiery, embray, jdemeyer 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 6 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/20322
  • Commit set to 465db90b975dbf2c2ada8d8b1c8f33ea4f048fcf
  • Status changed from new to needs_review

setting SAGE_BANNER to "bare" should give the expected result


New commits:

465db90trac #20322 quick solution for a shorten banner

comment:2 Changed 6 years ago by chapoton

  • Cc embray jdemeyer added

comment:3 Changed 6 years ago by jdemeyer

  • Component changed from PLEASE CHANGE to user interface
  • Status changed from needs_review to needs_work
  • Type changed from PLEASE CHANGE to enhancement

Can you use consistent indentation?

comment:4 Changed 6 years ago by git

  • Commit changed from 465db90b975dbf2c2ada8d8b1c8f33ea4f048fcf to c2d0ff3df2b0543fbab75c9c24e067d9bd6d48e4

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

c2d0ff3indentation

comment:5 Changed 6 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:6 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

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

comment:7 Changed 6 years ago by 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 6 years ago by jdemeyer

  • Branch changed from u/chapoton/20322 to u/jdemeyer/20322

comment:9 Changed 6 years ago by jdemeyer

  • Commit changed from c2d0ff3df2b0543fbab75c9c24e067d9bd6d48e4 to 846e21de218ace07322ac24a64341ec71141578e
  • Status changed from needs_work to needs_review

New commits:

846e21dMake full and bare banner consistent

comment:10 Changed 6 years ago by jdemeyer

Running full doctests now with SAGE_BANNER=bare...

comment:11 Changed 6 years ago by git

  • Commit changed from 846e21de218ace07322ac24a64341ec71141578e to 8b1c3697418043b7c1a6c4547d5e412a2ebe0e5f

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

8b1c369Ensure that SAGE_BANNER=no is not exported

comment:12 Changed 5 years ago by jdemeyer

Doctests pass: positive_review from my part.

comment:13 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

ok, let it be

comment:14 Changed 5 years ago by embray

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 5 years ago by chapoton

@embray

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

comment:16 Changed 5 years ago by embray

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 5 years ago by 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 5 years ago by embray

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 5 years ago by vbraun

  • Status changed from positive_review to 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 5 years ago by 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 5 years ago by jdemeyer

  • Status changed from needs_work to positive_review

I guess this is yet another "the bug is not in the ticket but in the buildbot".

comment:22 Changed 5 years ago by git

  • Commit changed from 8b1c3697418043b7c1a6c4547d5e412a2ebe0e5f to 5b87f1aca7a38d0c5ef80b8e12b16822039a7360
  • Status changed from positive_review to needs_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 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:24 Changed 5 years ago by jdemeyer

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

comment:25 Changed 5 years ago by embray

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

  • src/sage/tests/cmdline.py

    diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
    index 534436d..1d0395e 100644
    a b from subprocess import * 
    5757import os, select
    5858
    5959
    60 def test_executable(args, input="", timeout=100.0, **kwds):
     60def test_executable(args, input="", timeout=100.0, env={}, **kwds):
    6161    r"""
    6262    Run the program defined by ``args`` using the string ``input`` on
    6363    the standard input.
    def test_executable(args, input="", timeout=100.0, **kwds): 
    102102
    103103    Run Sage itself with various options::
    104104
    105         sage: (out, err, ret) = test_executable(["sage"])
     105        sage: (out, err, ret) = test_executable(["sage"], env={'SAGE_BANNER': 'bare'})
    106106        sage: out.find(version()) >= 0
    107107        True
    108108        sage: err
    def test_executable(args, input="", timeout=100.0, **kwds): 
    110110        sage: ret
    111111        0
    112112
    113         sage: (out, err, ret) = test_executable(["sage"], "3^33\n")
     113        sage: (out, err, ret) = test_executable(["sage"], "3^33\n", env={'SAGE_BANNER': 'bare'})
    114114        sage: out.find(version()) >= 0
    115115        True
    116116        sage: out.find("5559060566555523") >= 0
    def test_executable(args, input="", timeout=100.0, **kwds): 
    120120        sage: ret
    121121        0
    122122
    123         sage: (out, err, ret) = test_executable(["sage", "-q"], "3^33\n")
     123        sage: (out, err, ret) = test_executable(["sage", "-q"], "3^33\n", env={'SAGE_BANNER': 'bare'})
    124124        sage: out.find(version()) >= 0
    125125        False
    126126        sage: out.find("5559060566555523") >= 0
    def test_executable(args, input="", timeout=100.0, **kwds): 
    728728        del pexpect_env["TERM"]
    729729    except KeyError:
    730730        pass
     731
     732    pexpect_env.update(env)
     733
    731734    p = Popen(args, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=pexpect_env, **kwds)
    732735    if input:
    733736        p.stdin.write(input)
Last edited 5 years ago by embray (previous) (diff)

comment:26 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/20322 to 5b87f1aca7a38d0c5ef80b8e12b16822039a7360
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 Changed 5 years ago by embray

  • Commit 5b87f1aca7a38d0c5ef80b8e12b16822039a7360 deleted

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 5 years ago by embray

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.