Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sage-logger #20708

Closed
jdemeyer opened this issue May 29, 2016 · 14 comments
Closed

Improve sage-logger #20708

jdemeyer opened this issue May 29, 2016 · 14 comments

Comments

@jdemeyer
Copy link
Contributor

  1. Don't use the -p option at the top-level which only prints a useless [install] .

  2. Use sed instead of read/echo. This preserves whitespace.

CC: @embray

Component: build

Author: Jeroen Demeyer

Branch/Commit: ffb5922

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/20708

@jdemeyer jdemeyer added this to the sage-7.3 milestone May 29, 2016
@jdemeyer
Copy link
Contributor Author

Branch: u/jdemeyer/improve_sage_logger

@jdemeyer
Copy link
Contributor Author

New commits:

ffb5922Minor improvements to logging

@jdemeyer
Copy link
Contributor Author

Commit: ffb5922

@jhpalmieri
Copy link
Member

comment:3

This looks good to me. It works on OS X, for example the early parts of the matplotlib log look better with this, but someone should test it on some linux machines. (I don't think this is doctested, so the patchbot won't help.)

@jdemeyer
Copy link
Contributor Author

comment:4

Well, it works for me (on Gentoo Linux).

@embray
Copy link
Contributor

embray commented May 30, 2016

comment:5

I think this is fine. I didn't mind having [install] on every line, because when you run make not every step is necessarily in install, but most of the time it's clear enough from context that it isn't needed.

I think it's a bit silly to call this a "blocker" though don't you?

@vbraun
Copy link
Member

vbraun commented May 30, 2016

comment:6

Reviewer name

@jdemeyer
Copy link
Contributor Author

Reviewer: Erik Bray

@mkoeppe
Copy link
Contributor

mkoeppe commented May 31, 2016

comment:8

The recent sage-logger changes from #20640 seem to cause a minor problem.

When installing an "experimental" package, Sage warns a lot and then prompts the user.
Because of line buffering, one does not see the prompt, but Sage just waits indefinitely.

sage -f latte_int
...
[latte_int-1.7.3] =========================== WARNING ===========================
[latte_int-1.7.3] You are about to download and install an experimental package.
[latte_int-1.7.3] This probably won't work at all for you! There is no guarantee
[latte_int-1.7.3] that it will build correctly, or behave as expected.
[latte_int-1.7.3] Use at your own risk!
[latte_int-1.7.3] ===============================================================

<--- This is where it asks "[latte_int-1.7.3] Are you sure you want to continue [Y/n]?" but this is line-buffered and not visible to the user.

@jdemeyer
Copy link
Contributor Author

comment:9

Replying to @mkoeppe:

The recent sage-logger changes from #20640 seem to cause a minor problem.

On which operating system is this? And does the problem remain after applying #20708?

@mkoeppe
Copy link
Contributor

mkoeppe commented May 31, 2016

comment:10

This is on Mac OS X, after merging #20708.

@embray
Copy link
Contributor

embray commented May 31, 2016

comment:11

Replying to @mkoeppe:

The recent sage-logger changes from #20640 seem to cause a minor problem.

When installing an "experimental" package, Sage warns a lot and then prompts the user.
Because of line buffering, one does not see the prompt, but Sage just waits indefinitely.

I have noticed this as well. But I think jdmeyer's fix in this ticket should fix it due to the use of unbuffered sed. (The keyboard input is still accepted, you just don't see the prompt until later).

@mkoeppe
Copy link
Contributor

mkoeppe commented May 31, 2016

comment:12

From man sed(1) on OS X:

     -l      Make output line buffered.

Line buffered is not unbuffered.

@vbraun
Copy link
Member

vbraun commented May 31, 2016

Changed branch from u/jdemeyer/improve_sage_logger to ffb5922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants