maralorn / nix-output-monitor

Pipe your nix-build output through the nix-output-monitor a.k.a nom to get additional information while building.
GNU Affero General Public License v3.0
835 stars 24 forks source link

Reset console state when terminating through SIGINT/SIGTERM #115

Closed picnoir closed 11 months ago

picnoir commented 11 months ago

Keeping track of the console current internal state through a IORef. It makes the state accessible for both the application and the signals handlers.

In the process of solving this issue, we introduce a custom signal handler in charge to reset the console and plug it to SIGINT and SIGTERM. This signal handler terminates the overall program by killing the main thread with exit code 1. The RTS runtime then kills the other thread for us before exiting itself with code 1.

Fixes https://github.com/maralorn/nix-output-monitor/issues/114

I don't have any nice automated test for this. I manually tested the patch using:

(nix-build test.nix; nix-build test.nix) |& cabal run nom

Where test.nix is just something really long to build:

{ pkgs ? import <nixpkgs> {} }:

pkgs.stdenv.mkDerivation {
  pname = "test";
  version = "1";
  src = ./.;
  postInstall = ''
    sleep 100
  '';
}

I then send sigterm/sigint through htop to the nom process.

Note: I couldn't reproduce the initial issue @mic92 is facing. The cursor was properly reset on my setup when we kill Nom through SIGTERM. The cursor was not reset when killed via SIGINT though. This patch fixes this issue, killing with SIGINT now properly reset the terminal cursor.

Fixes #114

picnoir commented 11 months ago

Hmmm, wait a minute, I think I've been blinded by my initial bogus idea here.

Do we really need to track the console internal state or could we just send the cursorShow escape sequence regardless of the current state? :thinking:

Doing this would greatly reduce the size of this PR.

picnoir commented 11 months ago

Yup, we definitely do not need to keep track of the terminal state. It reduces the diffs from 50+ to 18.

Mic92 commented 11 months ago

Thanks. This solves my issue.

maralorn commented 11 months ago

Seems to work, code looks fine. Thank you <3