Opened 5 years ago

Closed 5 years ago

#20708 closed enhancement (fixed)

Improve sage-logger

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-7.3
Component: build Keywords:
Cc: embray Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: ffb5922 (Commits, GitHub, GitLab) Commit: ffb59227f6b80c4d938b34d2e3493eeb429323d1
Dependencies: Stopgaps:

Status badges

Description

  1. Don't use the -p option at the top-level which only prints a useless [install] .
  1. Use sed instead of read/echo. This preserves whitespace.

Change History (13)

comment:1 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/improve_sage_logger

comment:2 Changed 5 years ago by jdemeyer

  • Commit set to ffb59227f6b80c4d938b34d2e3493eeb429323d1
  • Status changed from new to needs_review

New commits:

ffb5922Minor improvements to logging

comment:3 Changed 5 years ago by jhpalmieri

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.)

comment:4 Changed 5 years ago by jdemeyer

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

comment:5 Changed 5 years ago by embray

  • Status changed from needs_review to positive_review

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?

comment:6 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:7 Changed 5 years ago by jdemeyer

  • Reviewers set to Erik Bray
  • Status changed from needs_work to positive_review

comment:8 follow-ups: Changed 5 years ago by 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.

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.

comment:9 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

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?

comment:10 Changed 5 years ago by mkoeppe

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

comment:11 in reply to: ↑ 8 Changed 5 years ago by embray

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).

comment:12 Changed 5 years ago by mkoeppe

From man sed(1) on OS X:

     -l      Make output line buffered.

Line buffered is not unbuffered.

comment:13 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/improve_sage_logger to ffb59227f6b80c4d938b34d2e3493eeb429323d1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.