#20897 closed defect (fixed)
Fixes to SAGE_BANNER=bare support throughout
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.3 |
Component: | user interface | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 9e5e9a5 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
#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))
Change History (36)
comment:1 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
comment:3 Changed 6 years ago by
Oops, thanks for pointing that out. No, that's a copy/paste error.
comment:4 Changed 6 years ago by
- Commit changed from 41f98cff8fbc052dd7fd0e3951ffb3a0252f8baf to f89054131b51774eae7ce66acd7bf6383448883d
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.
|
comment:5 Changed 6 years ago by
I fixed the error and rebased.
comment:6 Changed 6 years ago by
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.
comment:7 Changed 6 years ago by
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.
comment:8 Changed 6 years ago by
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 ?
comment:9 Changed 6 years ago by
Let me take a look at that--I hadn't really paid attention to SAGE_RELEASE_DATE
comment:10 Changed 6 years ago by
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.
comment:11 Changed 6 years ago by
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"
comment:12 follow-up: ↓ 13 Changed 6 years ago by
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
).
comment:13 in reply to: ↑ 12 ; follow-up: ↓ 17 Changed 6 years ago by
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..
comment:14 Changed 6 years ago by
It seems that the argument full
of banner_text
is ignored.
banner_text always print the full banner, unless an environnement variable is changed..
comment:15 Changed 6 years ago by
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).
comment:16 Changed 6 years ago by
- Commit changed from f89054131b51774eae7ce66acd7bf6383448883d to 4314e4ab786dbd1120ef143cce518f4cd9fcca11
Branch pushed to git repo; I updated commit sha1. New commits:
4314e4a | Fix the `full` option to `banner_text` to actually work as documented.
|
comment:17 in reply to: ↑ 13 Changed 6 years ago by
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
.
comment:18 Changed 6 years ago by
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.
comment:19 Changed 6 years ago by
A doctest would be good to better define the expectation here.
comment:20 Changed 6 years ago by
- Commit changed from 4314e4ab786dbd1120ef143cce518f4cd9fcca11 to 14dc47e62aec1a944ac4a7b537e19d4016bc60b9
Branch pushed to git repo; I updated commit sha1. New commits:
14dc47e | Added doctest demonstrating banner_text(full=False)
|
comment:21 Changed 6 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, then. This should work as far as I can tell. I hope there will be no unexpected side effect.
comment:22 Changed 6 years ago by
- Status changed from positive_review to needs_work
I need to doublecheck, it seems that some doctests in misc are failing.
comment:23 Changed 6 years ago by
and with the branch, running
sage -bt src/sage/misc/*.py
the banner is displayed without line breaks..
comment:24 Changed 6 years ago by
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
comment:25 Changed 6 years ago by
Oops! I'll fix the line breaks thing. That's just because there's missing quotes in the echo
.
comment:26 Changed 6 years ago by
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.
comment:27 Changed 6 years ago by
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.
comment:28 Changed 6 years ago by
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.
comment:29 Changed 6 years ago by
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 **********************************************************************
comment:30 Changed 6 years ago by
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..
comment:31 Changed 6 years ago by
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.
comment:32 Changed 6 years ago by
- Commit changed from 14dc47e62aec1a944ac4a7b537e19d4016bc60b9 to 9e5e9a57d87f8a9700bd06327b76ec23b4a978d5
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).
|
comment:33 Changed 6 years ago by
- Status changed from needs_work to needs_review
Okay, this should fix all the failing tests. Thanks again chapoton for your careful review.
comment:34 Changed 6 years ago by
- Status changed from needs_review to positive_review
Should be good now.
comment:35 Changed 6 years ago by
- Branch changed from u/embray/sage-banner to 9e5e9a57d87f8a9700bd06327b76ec23b4a978d5
- Resolution set to fixed
- Status changed from positive_review to closed
comment:36 Changed 6 years ago by
- Commit 9e5e9a57d87f8a9700bd06327b76ec23b4a978d5 deleted
Follow-up: #21029
this looks strange:
src/bin/sage-update-version
Do you really want to change the place where VERSION.txt is stored ?