line buffering in sage-logger causes "hang" due to invisible prompt when installing experimental packages

[mkoeppe]

(Observed on Mac OS X.)

As I reported in #20708 comment:8:

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.

CC: @jdemeyer @embray @vbraun @kcrisman @dimpase @videlec @novoselt

Component: build

Author: Matthias Koeppe

Branch: 6888a67

Reviewer: Leif Leonhardy, Volker Braun

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

[vbraun]

comment:1

IMHO we just shouldn't ask questions here, a (potentially) parallel make session is just not an interactive process. Either do it or bail out and require a command line switch --experimental to opt-in.

[mkoeppe]

comment:2

I think so too.

[embray]

comment:3

+1, just keep the message.

[kcrisman]

comment:4

I also think it's fine for experimental packages to just run and fail. As long as there isn't any potential to mess up the rest of Sage (like with messed up runs of make).

[embray]

Author: Erik Bray

[embray]

Branch: u/embray/disable-exp-pkg-prompt

[embray]

comment:5

Here's a patch to remove the prompt entirely.

Might be good to find a fix to the underlying problem, since in principle there could be some other prompt somewhere that would be hidden in the same way. I'm not sure how best to fix this though--would have to disable line-buffering on such prompts somehow.

---

New commits:

ccff005

Disable the y/n prompt when installing experimental packages.

[embray]

Commit: ccff005

[mkoeppe]

comment:6

This would be fine with me. But it could be argued that this warning is too easy to overlook (and comes too late) and so the point of distinguishing between optional and experimental packages is lost.

[dimpase]

comment:10

I don't think simply removing this dialog is the right thing to do.

[embray]

comment:12

This would be fine with me. But it could be argued that this warning is too easy to overlook (and comes too late) and so the point of distinguishing between optional and experimental packages is lost.

I'm not sure what you suggest differently, other than moving the warning a little earlier?

But if someone doesn't read their build output it will be lost on them anyways. That's okay, because it's "merely" a warning.

[embray]

comment:13

Dima has a point that removing the prompt outright is potentially disruptive.

What if we moved the warning so that there's no prompt if running make <packagename> directly, as a package developer might do. But running ./sage -i for an experimental package both displays the warning and the prompt (before ever even running make, much less sage-logger).

[mkoeppe]

comment:14

Replying to @embray:

Dima has a point that removing the prompt outright is potentially disruptive.

What if we moved the warning so that there's no prompt if running make <packagename> directly, as a package developer might do. But running ./sage -i for an experimental package both displays the warning and the prompt (before ever even running make, much less sage-logger).

I think ./sage -i PACKAGENAMES... should display the warning and the prompt before at the very beginning, before installing any package.

As per Volker's initial suggestion, a command-line switch --experimental should override the prompt. This would be useful in a script. I didn't know that make PACKAGENAME works; but in any case it would be better to be able to use "sage -i" also from a script.

[embray]

comment:15

make <packagename> is how all packages are built. sage -i is just a thin front-end over that.

It appears the warning/prompt comes from the sage-spkg (which is the program responsible for unpacking the package, running spkg-install, etc) which is in turn called by make.

I tried reworking this yesterday in a way that didn't change sage-spkg, but to make that prompt happen right away it would have to moved either into the makefile itself, or into the sage command, though I think it's good to have the warning when running sage-spkg too. So I'd hate to have duplicate copies of the warning.

In other words it's not exactly clear where the best place would be for it. Jeroen might have some ideas but he's on holiday right now so we won't bug him about it.

[sagetrac-git]

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

611480d	No matter what I do, even with sed --unbuffered, sed will read either up to the EOF or the EOL, but otherwise buffers until EOL.
632fea7	Add a new -y argument to sage-spkg to make it automatically answer 'y' to all prompts.

[sagetrac-git]

Changed commit from ccff005 to 632fea7

[embray]

comment:17

Welp, this is definitely more complicated than it was before, but I think it's pretty good.

[sagetrac-git]

Changed commit from 632fea7 to 4f7b63f

[sagetrac-git]

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

4f7b63f

Display an additional message when continuing automatically

[dimpase]

comment:20

looks good to me

[dimpase]

Reviewer: Dima Pasechnik, Matthias Koeppe

[sagetrac-git]

Changed commit from e2cf8fb to 035e1d8

[mkoeppe]

comment:90

I just tried sage -f latte_int and I observe the following three things (this is with MAKE='make -j12' in case that matters:

• Sage banner in color, interleaved with environment variables in color due to competing output
• a visible prompt "Are you sure you want to continue [Y/n]? "
• building the dependency 4ti2
• another warning about experimental packages, but no visible prompt.
• "hang" as before

[nexttime]

comment:92

You can avoid SED=cat if you put the pipe into the/a variable (POSTPROCESS, say), leaving it empty if no prefix was given, i.e.

    POSTPROCESS="| $SED 's/^/$prefix/'"
    ...
    ( trap '' SIGINT; eval tee -a "$logfile" $POSTPROCESS )
    ...

(I think.)

[nexttime]

comment:93

You don't have to install packages in a loop; you can download them one-by-one, and afterwards run make on all of them.

In any case, the following is wrong:

        sage-logger "sage-spkg $INSTALL_OPTIONS -d '$PKG'" logs/install.log

        # Then make / install the package with no prompts
        $MAKE SAGE_SPKG="sage-spkg $INSTALL_OPTIONS -y" "$OPT"

("$OPT" should be "$PKG".)

---

Unrelated to this ticket:

# First of all, make sure that the toolchain is up-to-date
# (which is a dependency of every package)
./sage --location
$MAKE all-toolchain

is pretty stupid before we know the package(s) at all exist(s), can be downloaded if needed, whatever. (And IIRC one gets *** ALL ENVIRONMENT VARIABLES BEFORE BUILD: *** etc. twice for no reason, the first time before any relevant processing happens.)

[jdemeyer]

comment:94

Replying to @nexttime:

You don't have to install packages in a loop; you can download them one-by-one, and afterwards run make on all of them.

+1

[mkoeppe]

Changed branch from u/embray/ticket-20884-squashed to u/mkoeppe/ticket-20884-squashed

[mkoeppe]

Changed reviewer from Dima Pasechnik, Matthias Koeppe to none

[mkoeppe]

comment:96

I've implemented a "good enough for next release" solution.
Making it prettier can be on a follow-up ticket.
Needs review.

---

New commits:

0b1acc2

Simple (not pretty) solution for #20884

[mkoeppe]

Changed author from Erik Bray to Matthias Koeppe

[mkoeppe]

Changed commit from 035e1d8 to 0b1acc2

[nexttime]

comment:97

Replying to @mkoeppe:

I've implemented a "good enough for next release" solution.
Making it prettier can be on a follow-up ticket.
Needs review.

---

New commits:

0b1acc2

Simple (not pretty) solution for #20884

Ok for me, modulo indentation... (tabs instead of spaces perhaps?)

[nexttime]

comment:98

(At least in theory, we'd have to make sure the prompting message doesn't vanish due to other output when building in parallel though. But I think in practice this is very unlikely.)

[mkoeppe]

comment:99

Replying to @nexttime:

(At least in theory, we'd have to make sure the prompting message doesn't vanish due to other output when building in parallel though. But I think in practice this is very unlikely.)

Yes, I hope that a follow-up ticket will do a proper solution, which prompts early (or just bails out and requires an extra flag), rather than prompting in the middle of a parallel build.

My simple solution just restores (except for cosmetics) the status quo before #20640, #20708 broke the prompting.

[sagetrac-git]

Changed commit from 0b1acc2 to 6888a67

[sagetrac-git]

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

6888a67

untabify changes

[vbraun]

Reviewer: Leif Leonhardy, Volker Braun

[vbraun]

Changed branch from u/mkoeppe/ticket-20884-squashed to 6888a67

[mkoeppe]

Changed commit from 6888a67 to none

[mkoeppe]

comment:103

Follow-up ticket - #21082. This is where work on the patch by Erik Bray can continue.